Messages in this thread | | | From | Alexander Duyck <> | Date | Mon, 30 Jan 2023 08:17:58 -0800 | Subject | Re: [net PATCH] skb: Do mix page pool and page referenced frags in GRO |
| |
On Mon, Jan 30, 2023 at 12:50 AM Paolo Abeni <pabeni@redhat.com> wrote: > > On Sat, 2023-01-28 at 08:08 +0100, Eric Dumazet wrote: > > On Sat, Jan 28, 2023 at 6:26 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > > > On Sat, 28 Jan 2023 10:37:47 +0800 Yunsheng Lin wrote: > > > > If we are not allowing gro for the above case, setting NAPI_GRO_CB(p)->flush > > > > to 1 in gro_list_prepare() seems to be making more sense so that the above > > > > case has the same handling as skb_has_frag_list() handling? > > > > https://elixir.bootlin.com/linux/v6.2-rc4/source/net/core/gro.c#L503 > > > > > > > > As it seems to avoid some unnecessary operation according to comment > > > > in tcp4_gro_receive(): > > > > https://elixir.bootlin.com/linux/v6.2-rc4/source/net/ipv4/tcp_offload.c#L322 > > > > > > The frag_list case can be determined with just the input skb. > > > For pp_recycle we need to compare input skb's pp_recycle with > > > the pp_recycle of the skb already held by GRO. > > > > > > I'll hold off with applying a bit longer tho, in case Eric > > > wants to chime in with an ack or opinion. > > > > We can say that we are adding in the fast path an expensive check > > about an unlikely condition. > > > > GRO is by far the most expensive component in our stack. > > Slightly related to the above: currently the GRO engine performs the > skb metadata check for every packet. My understanding is that even with > XDP enabled and ebpf running on the given packet, the skb should > usually have meta_len == 0. > > What about setting 'skb->slow_gro' together with meta_len and moving > the skb_metadata_differs() check under the slow_gro guard?
Makes sense to me, especially since we have to do a pointer chase to get the metadata length out of the shared info.
Looking at the code one thing I was wondering about is if we should be flagging frames where one is slow_gro and one is not as having a diff and just skipping the checks since we know the slow_gro checks are expensive and if they differ based on that flag odds are one will have a field present that the other doesn't.
| |