Received-SPF: pass (google.com: domain of linux-kernel+bounces-121611-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <f314f3c5-0641-42e3-be56-3173fdcf0977@gmail.com>
Date: Wed, 27 Mar 2024 17:07:46 +0100
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH net-next v4 4/4] net: gro: move L3 flush checks to
 tcp_gro_receive
To: Paolo Abeni <pabeni@redhat.com>, Eric Dumazet <edumazet@google.com>
Cc: davem@davemloft.net, kuba@kernel.org, willemdebruijn.kernel@gmail.com,
 netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-kselftest@vger.kernel.org
References: <20240325182543.87683-1-richardbgobert@gmail.com>
 <20240325182543.87683-5-richardbgobert@gmail.com>
 <CANn89iKzeTKuBA3NL0DQUmUHmmc0QzZ0X62DUarZ2Q7cKRZvSA@mail.gmail.com>
 <46e0c775-91e7-4bf6-88f3-53ab5e00414f@gmail.com>
 <CANn89iJkDbzLKmUGRHNFpfiaO8z19i44qgqkBA9Updt4QsRkyg@mail.gmail.com>
 <6566fd5f-fcdf-4dc7-b8a2-5e8a182f8c49@gmail.com>
 <d60c6185b8394da02479100981fa3f1306d9c81f.camel@redhat.com>
 <57bf675d-c2f0-4022-845c-166891e336be@gmail.com>
 <9f3509a7134f7e2dfd633ea62d24815e12b1f482.camel@redhat.com>
From: Richard Gobert <richardbgobert@gmail.com>
In-Reply-To: <9f3509a7134f7e2dfd633ea62d24815e12b1f482.camel@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Paolo Abeni wrote:
> On Tue, 2024-03-26 at 18:25 +0100, Richard Gobert wrote:
>> Paolo Abeni wrote:
>>> Hi,
>>>
>>> On Tue, 2024-03-26 at 16:02 +0100, Richard Gobert wrote:
>>>> This patch is meaningful by itself - removing checks against non-relevant
>>>> packets and making the flush/flush_id checks in a single place.
>>>
>>> I'm personally not sure this patch is a win. The code churn is
>>> significant. I understand this is for performance's sake, but I don't
>>> see the benefit??? 
>>>
>>
>> Could you clarify what do you mean by code churn?
> 
> The diffstat of this patch is not negligible and touches very sensitive
> areas.
> 

diff mainly touches flush/flush_id/is_atomic, the new code should be
less complex. I agree this is sensitive as it is part of core GRO -
I checked all relevant flows manually, but I can also create more
tests and ensure that logic remains the same.

>>> he changelog shows that perf reports slightly lower figures for
>>> inet_gro_receive(). That is expected, as this patch move code out of
>>> such functio. What about inet_gro_flush()/tcp_gro_receive() where such
>>> code is moved?
>>>
>>
>> Please consider the following 2 common scenarios:
>>
>> 1) Multiple packets in the GRO bucket - the common case with multiple
>>    packets in the bucket (i.e. running super_netperf TCP_STREAM) - each layer
>>    executes a for loop - going over each packet in the bucket. Specifically,
>>    L3 gro_receive loops over the bucket making flush,flush_id,is_atomic
>>    checks. 
> 
> Only for packets with the same rx hash. 
> 

Right, but there are only 8 GRO buckets, so a collision can still happen
on multiple concurrent streams.

>> For most packets in the bucket, these checks are not
>>    relevant. (possibly also dirtying cache lines with non-relevant p
>>    packets). Removing code in the for loop for this case is significant.
>>
>> 2) UDP/TCP streams which do not coalesce in GRO. This is the common case
>>    for regular UDP connections (i.e. running netperf UDP_STREAM). In this
>>    case, GRO is just overhead. Removing any code from these layers
>>    is good (shown in the first measurement of the commit message).
> 
> If UDP GRO is not enabled, there are no UDP packet staging in the UDP
> gro engine, the bucket list is empty.
> 
>>> Additionally the reported deltas is within noise level according to my
>>> personal experience with similar tests.
>>>
>>
>> I've tested the difference between net-next and this patch repetitively,
>> which showed stable results each time. Is there any specific test you
>> think would be helpful to show the result?
> 
> Anything that show measurable gain. 
> 
> Reporting the CPU utilization in the inet_gro_receive() function alone
> is not enough, as part of the load has been moved into
> gro_network_flush()/tcp_gro_receive().
> 

Got it, the numbers I reported were only relevant to UDP flows (so
measuring perf top with -g flag showed the same improvement). I'll post in v5
numbers relevant to TCP as well.

Thanks