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.