Received-SPF: pass (google.com: domain of linux-kernel+bounces-141279-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <24daf0f8-1e81-4bdb-81f3-0f95bf4392f4@gmail.com>
Date: Thu, 11 Apr 2024 18:07:13 +0200
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH net-next v6 5/6] net: gro: move L3 flush checks to
 tcp_gro_receive and udp_gro_receive_segment
To: 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
References: <20240410153423.107381-1-richardbgobert@gmail.com>
 <20240410153423.107381-6-richardbgobert@gmail.com>
 <66174ec5bbd29_2d6bc629481@willemb.c.googlers.com.notmuch>
From: Richard Gobert <richardbgobert@gmail.com>
In-Reply-To: <66174ec5bbd29_2d6bc629481@willemb.c.googlers.com.notmuch>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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.

In addition, UDP flows where skb_gro_receive_list is called -
flush/flush_id is not relevant and does not need to be calculated. 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.