Messages in this thread | | | Date | Mon, 30 Jan 2023 16:40:48 +0100 | Subject | Re: [PATCH 2/2] gro: optimise redundant parsing of packets | From | Alexander Lobakin <> |
| |
From: Richard Gobert <richardbgobert@gmail.com> Date: Mon, 30 Jan 2023 14:07:55 +0100
> 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...
> > Signed-off-by: Richard Gobert <richardbgobert@gmail.com> > --- > include/net/gro.h | 8 ++++++-- > net/ethernet/eth.c | 11 +++++++++-- > net/ipv4/af_inet.c | 8 +++++++- > net/ipv6/ip6_offload.c | 15 ++++++++++++--- > 4 files changed, 34 insertions(+), 8 deletions(-)
[...]
> @@ -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.
> struct packet_offload *ptype; > int err = -ENOSYS; > > - if (skb->encapsulation) > + if (skb->encapsulation) { > skb_set_inner_mac_header(skb, nhoff); > + type = eh->h_proto; > + } else { > + type = NAPI_GRO_CB(skb)->network_proto; > + } > > ptype = gro_find_complete_by_type(type); > if (ptype != NULL) > diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c > index 6c0ec2789943..4401af7b3a15 100644 > --- a/net/ipv4/af_inet.c > +++ b/net/ipv4/af_inet.c > @@ -1551,6 +1551,9 @@ struct sk_buff *inet_gro_receive(struct list_head *head, struct sk_buff *skb) > * immediately following this IP hdr. > */ > > + if (!NAPI_GRO_CB(skb)->encap_mark) > + NAPI_GRO_CB(skb)->transport_proto = proto; > + > /* Note : No need to call skb_gro_postpull_rcsum() here, > * as we already checked checksum over ipv4 header was 0 > */ > @@ -1621,12 +1624,15 @@ int inet_gro_complete(struct sk_buff *skb, int nhoff) > __be16 newlen = htons(skb->len - nhoff); > struct iphdr *iph = (struct iphdr *)(skb->data + nhoff); > const struct net_offload *ops; > - int proto = iph->protocol; > + int proto;
(same)
> int err = -ENOSYS; > > if (skb->encapsulation) { > skb_set_inner_protocol(skb, cpu_to_be16(ETH_P_IP)); > skb_set_inner_network_header(skb, nhoff); > + proto = iph->protocol; > + } else { > + proto = NAPI_GRO_CB(skb)->transport_proto; > } > > csum_replace2(&iph->check, iph->tot_len, newlen);
[...]
> @@ -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).
> + } else { > + nhoff += sizeof(*iph) + ipv6_exthdrs_len(iph, &ops); > + } > + > if (WARN_ON(!ops || !ops->callbacks.gro_complete)) > goto out; >
Thanks, Olek
| |