From: Paolo Abeni <pabeni@redhat.com>
To: Richard Gobert <richardbgobert@gmail.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 19:29:09 +0100 [thread overview]
Message-ID: <9f3509a7134f7e2dfd633ea62d24815e12b1f482.camel@redhat.com> (raw)
In-Reply-To: <57bf675d-c2f0-4022-845c-166891e336be@gmail.com>
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.
> > 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.
> 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().
Cheers,
Paolo
next prev parent reply other threads:[~2024-03-26 18:29 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
2024-03-26 18:29 ` Paolo Abeni [this message]
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=9f3509a7134f7e2dfd633ea62d24815e12b1f482.camel@redhat.com \
--to=pabeni@redhat.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=richardbgobert@gmail.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.