From: Richard Gobert <richardbgobert@gmail.com>
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
Subject: Re: [PATCH net-next v4 4/4] net: gro: move L3 flush checks to tcp_gro_receive
Date: Tue, 26 Mar 2024 18:25:02 +0100 [thread overview]
Message-ID: <57bf675d-c2f0-4022-845c-166891e336be@gmail.com> (raw)
In-Reply-To: <d60c6185b8394da02479100981fa3f1306d9c81f.camel@redhat.com>
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?
This patch removes all use of p->flush and flush_id from the
CB. The entire logic for L3 flush_id is scattered in tcp_gro_receive
and {inet,ipv6}_gro_receive with conditionals rewriting ->flush,
->flush_id and ->is_atomic. Moving it to one place (gro_network_flush)
should be more readable. (Personally, it took me a lot of time to
understand the current logic of flush + flush_id + is_atomic)
> The 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. 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).
In the case of a single TCP connection - the amount of checks should be
the same overall not causing any noticeable difference.
> 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?
Thanks
next prev parent reply other threads:[~2024-03-26 17:25 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-25 18:25 [PATCH net-next v4 0/4] net: gro: encapsulation bug fix and flush checks improvements Richard Gobert
2024-03-25 18:25 ` [PATCH net-next v4 1/4] net: gro: add p_off param in *_gro_complete Richard Gobert
2024-03-25 18:25 ` [PATCH net-next v4 2/4] selftests/net: add local address bind in vxlan selftest Richard Gobert
2024-03-25 18:25 ` [PATCH net-next v4 3/4] net: gro: add {inner_}network_offset to napi_gro_cb Richard Gobert
2024-03-25 18:25 ` [PATCH net-next v4 4/4] net: gro: move L3 flush checks to tcp_gro_receive Richard Gobert
2024-03-25 18:53 ` Willem de Bruijn
2024-03-26 12:35 ` Richard Gobert
2024-03-26 13:40 ` Willem de Bruijn
2024-03-26 14:09 ` Richard Gobert
2024-03-26 2:33 ` Jakub Kicinski
2024-03-26 14:14 ` Eric Dumazet
2024-03-26 14:43 ` Richard Gobert
2024-03-26 14:46 ` Eric Dumazet
2024-03-26 15:02 ` Richard Gobert
2024-03-26 15:10 ` Willem de Bruijn
2024-03-26 15:26 ` Richard Gobert
2024-03-26 16:14 ` Paolo Abeni
2024-03-26 17:25 ` Richard Gobert [this message]
2024-03-26 18:29 ` Paolo Abeni
2024-03-27 16:07 ` Richard Gobert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=57bf675d-c2f0-4022-845c-166891e336be@gmail.com \
--to=richardbgobert@gmail.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=willemdebruijn.kernel@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.