Received-SPF: pass (google.com: domain of linux-kernel+bounces-141634-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 11 Apr 2024 17:35:11 -0400
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Richard Gobert <richardbgobert@gmail.com>, 
 Willem de Bruijn <willemdebruijn.kernel@gmail.com>, 
 davem@davemloft.net, 
 edumazet@google.com, 
 kuba@kernel.org, 
 pabeni@redhat.com, 
 shuah@kernel.org, 
 dsahern@kernel.org, 
 aduyck@mirantis.com, 
 netdev@vger.kernel.org, 
 linux-kernel@vger.kernel.org, 
 linux-kselftest@vger.kernel.org
Message-ID: <6618578fc34fa_36e52529429@willemb.c.googlers.com.notmuch>
In-Reply-To: <24daf0f8-1e81-4bdb-81f3-0f95bf4392f4@gmail.com>
References: <20240410153423.107381-1-richardbgobert@gmail.com>
 <20240410153423.107381-6-richardbgobert@gmail.com>
 <66174ec5bbd29_2d6bc629481@willemb.c.googlers.com.notmuch>
 <24daf0f8-1e81-4bdb-81f3-0f95bf4392f4@gmail.com>
Subject: Re: [PATCH net-next v6 5/6] net: gro: move L3 flush checks to
 tcp_gro_receive and udp_gro_receive_segment
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 7bit

Richard Gobert wrote:
> Willem de Bruijn wrote:
> > Richard Gobert wrote:
> >> {inet,ipv6}_gro_receive functions perform flush checks (ttl, flags,
> >> iph->id, ...) against all packets in a loop. These flush checks are used
> >> currently only in tcp flows in GRO.
> >>
> >> These checks need to be done only once in tcp_gro_receive and only against
> >> the found p skb, since they only affect flush and not same_flow.
> > 
> > I don't quite understand where the performance improvements arise.
> > As inet_gro_receive will skip any p that does not match:
> > 
> >       if (!NAPI_GRO_CB(p)->same_flow)
> >               continue;
> > 
> >       iph2 = (struct iphdr *)(p->data + off);
> >       /* The above works because, with the exception of the top
> >        * (inner most) layer, we only aggregate pkts with the same
> >        * hdr length so all the hdrs we'll need to verify will start
> >        * at the same offset.
> >        */
> >       if ((iph->protocol ^ iph2->protocol) |
> >           ((__force u32)iph->saddr ^ (__force u32)iph2->saddr) |
> >           ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) {
> >               NAPI_GRO_CB(p)->same_flow = 0;
> >               continue;
> >       }
> > 
> > So these checks are already only performed against a p that matches.
> >  
> 
> 
> Thanks for the review!
> 
> flush/flush_id is calculated for all other p with same_flow = 1 (which is
> not always determined to be 0 before inet_gro_receive) and same src/dst
> addr in the bucket. Moving it to udp_gro_receive_segment/tcp_gro_receive
> will make it run only once when a matching p is found.

So this optimization is for flows that are the same up to having the
same saddr/daddr. Aside from stress tests, it seems rare to have many
concurrent flows between the same pair of machines?

> 
> In addition, UDP flows where skb_gro_receive_list is called -
> flush/flush_id is not relevant and does not need to be calculated. 

That makes sense

> In these
> cases total CPU time in GRO should drop. I could post perf numbers for
> this flow as well.
> 
> 
> >> Leveraging the previous commit in the series, in which correct network
> >> header offsets are saved for both outer and inner network headers -
> >> allowing these checks to be done only once, in tcp_gro_receive. As a
> > 
> > Comments should be updated to reflect both TCP and L4 UDP. Can
> > generalize to transport callbacks.
> > 
> >> result, NAPI_GRO_CB(p)->flush is not used at all. In addition, flush_id
> >> checks are more declarative and contained in inet_gro_flush, thus removing
> >> the need for flush_id in napi_gro_cb.
> >>
> >> This results in less parsing code for UDP flows and non-loop flush tests
> >> for TCP flows.
> > 
> > This moves network layer tests out of the network layer callbacks into
> > helpers called from the transport layer callback. And then the helper
> > has to look up the network layer header and demultiplex the protocol
> > again:
> > 
> >     +		if (((struct iphdr *)nh)->version == 6)
> >     +			flush |= ipv6_gro_flush(nh, nh2);
> >     +		else
> >     +			flush |= inet_gro_flush(nh, nh2, p, i != encap_mark);
> > 
> > That just seems a bit roundabout.
> 
> IMO this commit could be a part of a larger change, where all
> loops in gro_list_prepare, inet_gro_receive and ipv6_gro_receive can be
> removed, and the logic for finding a matching p will be moved to L4.  This
> means that when p is found, the rest of the gro_list would not need to be
> traversed and thus would not even dirty cache lines at all. I can provide a
> code snippet which would explain it better.

These loops are exactly the mechanism to find a matching p. Though
with all the callbacks perhaps not the most efficient model. The
hashtable should have solved much of that.

Yes, please share a snippet to understand how you would replace this.

In the meantime, I do suggest sending the first two patches to net,
as they have Fixes tags. And then follow up with this for net-next
separately.