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.
