Re: [RFC PATCH 1/2] net: gro: avoid relaying on skb->transport_header at receive time

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Paolo Abeni wrote:
> Currently {tcp,udp}_gro_receive relay on the gro network stage setting
> the correct transport header offset for all the skbs held by the GRO
> engine.
> 
> Such assumption is not necessary, as the code can instead leverage the
> offset already available for the currently processed skb. Add a couple
> of helpers to for readabilty' sake.
> 
> As skb->transport_header lays on a different cacheline wrt skb->data,
> this should save a cacheline access for each packet aggregation.
> Additionally this will make the next patch possible.
> 
> Note that the compiler (gcc 15.2.1) does inline the tcp_gro_lookup()
> call in tcp_gro_receive(), so the additional argument is only relevant
> for the fraglist case.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  include/net/gro.h        | 26 ++++++++++++++++++++++++++
>  include/net/tcp.h        |  3 ++-
>  net/ipv4/tcp_offload.c   | 15 ++++++++-------
>  net/ipv4/udp_offload.c   |  4 ++--
>  net/ipv6/tcpv6_offload.c |  2 +-
>  5 files changed, 39 insertions(+), 11 deletions(-)
> 
> diff --git a/include/net/gro.h b/include/net/gro.h
> index b65f631c521d..fdb9285ab117 100644
> --- a/include/net/gro.h
> +++ b/include/net/gro.h
> @@ -420,6 +420,18 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
>  				struct udphdr *uh, struct sock *sk);
>  int udp_gro_complete(struct sk_buff *skb, int nhoff, udp_lookup_t lookup);
>  
> +/* Return the skb hdr corresponding to the specified skb2 hdr.
> + * skb2 is held in the gro engine, i.e. its headers are in the linear part.

I thought "being held in the gro engine" intended to mean behing held
on the gro_list, i.e., p.

But this is used inverse, where skb2 is the currently arriving packet
and skb == p. Is this intentional and am I just misunderstanding the
intent of this comment? Or is the comment intended to say

"skb is held on the gro list, therefore [..]"

> + */
> +static inline const void *
> +skb_gro_header_from(const struct sk_buff *skb, const struct sk_buff *skb2,
> +		    const void *hdr2)
> +{
> +	size_t offset = (unsigned char *)hdr2 - skb2->data;
> +
> +	return skb->data + offset;
> +}