On Thu, Nov 30, 2023 at 10:32 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote: > > On 11/30/23 2:55 PM, Toke Høiland-Jørgensen wrote: > > Daniel Borkmann <daniel@xxxxxxxxxxxxx> writes > >> On 11/29/23 10:52 PM, Toke Høiland-Jørgensen wrote: > >>> Edward Cree <ecree.xilinx@xxxxxxxxx> writes: > >>>> On 28/11/2023 14:39, Toke Høiland-Jørgensen wrote: > >>>>> I'm not quite sure what should be the semantics of that, though. I.e., > >>>>> if you are trying to aggregate two packets that have the flag set, which > >>>>> packet do you take the value from? What if only one packet has the flag > >> > >> It would probably make sense if both packets have it set. > > > > Right, so "aggregate only if both packets have the flag set, keeping the > > metadata area from the first packet", then? > > Yes, sgtm. > There is one flaw for TCP in current implementation (before adding the flag), which we experienced earlier in production: when metadata differs on TCP packets, it not only disables GRO, but also reorder all PSH packets. This happens because when metadata differs, the new packet will be linked as a different node on the GRO merge list, since NAPI_GRO_CB->same_flow is set to 0 for all previous packets. However, packets with flags like PSH will be immediately flushed to the upper stack, while its predecessor packets might still be waiting on the merge list. I think it might make sense to first delay metadata comparison before skb_gro_receive, then add the flag to determine if the difference really matters. Yan
