linux-kernel - Re: [PATCH net-next v6 5/6] net: gro: move L3 flush checks to tcp_gro_receive and udp_gro_receive

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <24daf0f8-1e81-4bdb-81f3-0f95bf4392f4@gmail.com>
Date: Thu, 11 Apr 2024 18:07:13 +0200
From: Richard Gobert <richardbgobert@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, shuah@...nel.org,
 dsahern@...nel.org, aduyck@...antis.com, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v6 5/6] net: gro: move L3 flush checks to
 tcp_gro_receive and udp_gro_receive_segment

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.