netdev - Re: [PATCH 2/2] gro: optimise redundant parsing of packets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 22 Feb 2023 15:47:46 +0100
From:   Richard Gobert <richardbgobert@...il.com>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, yoshfuji@...ux-ipv6.org, dsahern@...nel.org,
        steffen.klassert@...unet.com, lixiaoyan@...gle.com,
        alexanderduyck@...com, leon@...nel.org, ye.xingchen@....com.cn,
        iwienand@...hat.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] gro: optimise redundant parsing of packets

> > Currently, the IPv6 extension headers are parsed twice: first in
> > ipv6_gro_receive, and then again in ipv6_gro_complete.
> > 
> > The field NAPI_GRO_CB(skb)->proto is used by GRO to hold the layer 4
> > protocol type that comes after the IPv6 layer. I noticed that it is set
> > in ipv6_gro_receive, but isn't used anywhere. By using this field, and
> > also storing the size of the network header, we can avoid parsing
> > extension headers a second time in ipv6_gro_complete.
> > 
> > The implementation had to handle both inner and outer layers in case of
> > encapsulation (as they can't use the same field).
> > 
> > I've applied this optimisation to all base protocols (IPv6, IPv4,
> > Ethernet). Then, I benchmarked this patch on my machine, using ftrace to
> > measure ipv6_gro_complete's performance, and there was an improvement.
> 
> Would be nice to see some perf numbers. "there was an improvement"
> doesn't say a lot TBH...
> 

I just posted raw performance numbers as a reply to Eric's message. Take a
look there.

> > @@ -456,12 +459,16 @@ EXPORT_SYMBOL(eth_gro_receive);
> >  int eth_gro_complete(struct sk_buff *skb, int nhoff)
> >  {
> >  	struct ethhdr *eh = (struct ethhdr *)(skb->data + nhoff);
> > -	__be16 type = eh->h_proto;
> > +	__be16 type;
> 
> Please don't break RCT style when shortening/expanding variable
> declaration lines.

Will be fixed in v2.

> > @@ -358,7 +361,13 @@ INDIRECT_CALLABLE_SCOPE int ipv6_gro_complete(struct sk_buff *skb, int nhoff)
> >  		iph->payload_len = htons(payload_len);
> >  	}
> >  
> > -	nhoff += sizeof(*iph) + ipv6_exthdrs_len(iph, &ops);
> > +	if (!skb->encapsulation) {
> > +		ops = rcu_dereference(inet6_offloads[NAPI_GRO_CB(skb)->transport_proto]);
> > +		nhoff += NAPI_GRO_CB(skb)->network_len;
> 
> Why not use the same skb_network_header_len() here? Both
> skb->network_header and skb->transport_header must be set and correct at
> this point (if not, you can always fix that).
> 

When processing packets with encapsulation the network_header field is
overwritten when processing the inner IP header, so skb_network_header_len won't
return the correct value.