Skip to contentSkip to conversation

sk_buff: optimize GRO for the common case

PATCH Applied Paolo Abeni 3 participants 10 messages Jul 28, 2021 – Jul 30, 2021

Subsystems (3)
Networking Network drivers Sockets
lore
Share & tools
lore

Paolo Abeni

author maintainer
#0 lore raw

cover sk_buff: optimize GRO for the common case

This is a trimmed down revision of "sk_buff: optimize layout for GRO", specifically dropping the changes to the sk_buff layout[1].

This series tries to accomplish 2 goals:
- optimize the GRO stage for the most common scenario, avoiding a bunch of conditional and some more code
- let owned skbs entering the GRO engine, allowing backpressure in the veth GRO forward path.

A new sk_buff flag (!!!) is introduced and maintained for GRO's sake. Such field uses an existing hole, so there is no change to the sk_buff size.

[1] two main reasons:
- move skb->inner_ field requires some extra care, as some in kernel users access and the fields regardless of skb->encapsulation.
- extending secmark size clash with ct and nft uAPIs

address the all above is possible, I think, but for sure not in a single series.

Paolo Abeni (6):
sk_buff: introduce 'slow_gro' flags
sk_buff: track dst status in slow_gro
sk_buff: track extension status in slow_gro
net: optimize GRO for the common case.
skbuff: allow 'slow_gro' for skb carring sock reference
veth: use skb_prepare_for_gro()

 drivers/net/veth.c     |  2 +-
 include/linux/skbuff.h |  6 ++++++
 include/net/dst.h      |  2 ++
 include/net/sock.h     |  9 +++++++++
 net/core/dev.c         | 32 ++++++++++++++++++++++++--------
 net/core/skbuff.c      | 27 ++++++++++++++++++++-------
 6 files changed, 62 insertions(+), 16 deletions(-)
Signature · 1 line
2.26.3
author maintainer
#1 lore raw

PATCH 1/6 sk_buff: introduce 'slow_gro' flags

The new flag tracks if any state field is set, so that GRO requires 'unusual'/slow prepare steps.

Set such flag when a ct entry is attached to the skb, and never clear it.

The new bit uses an existing hole into the sk_buff struct

RFC -> v1:
- use a single state bit, never clear it
- avoid moving the _nfct field

Signed-off-by Paolo Abeni <[email protected]>
---
 include/linux/skbuff.h | 4 ++++
 1 file changed, 4 insertions(+)
include/linux/skbuff.h +4 -0
index f19190820e63..3ff18300d210 100644
@@ -689,6 +689,7 @@ typedef unsigned char *sk_buff_data_t;
  *		CHECKSUM_UNNECESSARY (max 3)
  *	@dst_pending_confirm: need to confirm neighbour
  *	@decrypted: Decrypted SKB
+ *	@slow_gro: state present at GRO time, slower prepare step required
  *	@napi_id: id of the NAPI struct this skb came from
  *	@sender_cpu: (aka @napi_id) source CPU in XPS
  *	@secmark: security marking

@@ -870,6 +871,7 @@ struct sk_buff {
 #ifdef CONFIG_TLS_DEVICE
 	__u8			decrypted:1;
 #endif
+	__u8			slow_gro:1;
 
 #ifdef CONFIG_NET_SCHED
 	__u16			tc_index;	/* traffic control index */

@@ -4216,6 +4218,7 @@ static inline unsigned long skb_get_nfct(const struct sk_buff *skb)
 static inline void skb_set_nfct(struct sk_buff *skb, unsigned long nfct)
 {
 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
+	skb->slow_gro |= !!nfct;
 	skb->_nfct = nfct;
 #endif
 }

@@ -4375,6 +4378,7 @@ static inline void nf_copy(struct sk_buff *dst, const struct sk_buff *src)
 #if defined(CONFIG_NF_CONNTRACK) || defined(CONFIG_NF_CONNTRACK_MODULE)
 	nf_conntrack_put(skb_nfct(dst));
 #endif
+	dst->slow_gro = src->slow_gro;
 	__nf_copy(dst, src, true);
 }
Signature · 1 line
2.26.3
author maintainer
#2 lore raw

PATCH 2/6 sk_buff: track dst status in slow_gro

In reply to Paolo Abeni

Similar to the previous patch, but covering the dst field:
the slow_gro flag is additionally set when a dst is attached to the skb

RFC -> v1:
- use the existing flag instead of adding a new one

Signed-off-by Paolo Abeni <[email protected]>
---
 include/linux/skbuff.h | 2 ++
 include/net/dst.h      | 2 ++
 2 files changed, 4 insertions(+)
include/linux/skbuff.h +2 -0
index 3ff18300d210..b1e5bbfcc926 100644
@@ -992,6 +992,7 @@ static inline struct dst_entry *skb_dst(const struct sk_buff *skb)
  */
 static inline void skb_dst_set(struct sk_buff *skb, struct dst_entry *dst)
 {
+	skb->slow_gro |= !!dst;
 	skb->_skb_refdst = (unsigned long)dst;
 }
 

@@ -1008,6 +1009,7 @@ static inline void skb_dst_set(struct sk_buff *skb, struct dst_entry *dst)
 static inline void skb_dst_set_noref(struct sk_buff *skb, struct dst_entry *dst)
 {
 	WARN_ON(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
+	skb->slow_gro = !!dst;
 	skb->_skb_refdst = (unsigned long)dst | SKB_DST_NOREF;
 }
include/net/dst.h +2 -0
index 75b1e734e9c2..a057319aabef 100644
@@ -277,6 +277,7 @@ static inline void skb_dst_drop(struct sk_buff *skb)
 
 static inline void __skb_dst_copy(struct sk_buff *nskb, unsigned long refdst)
 {
+	nskb->slow_gro |= !!refdst;
 	nskb->_skb_refdst = refdst;
 	if (!(nskb->_skb_refdst & SKB_DST_NOREF))
 		dst_clone(skb_dst(nskb));

@@ -316,6 +317,7 @@ static inline bool skb_dst_force(struct sk_buff *skb)
 			dst = NULL;
 
 		skb->_skb_refdst = (unsigned long)dst;
+		skb->slow_gro |= !!dst;
 	}
 
 	return skb->_skb_refdst != 0UL;
Signature · 1 line
2.26.3
maintainer
#3 lore raw

Re: [PATCH net-next 2/6] sk_buff: track dst status in slow_gro

Quoted 13 lines · On Wed, 28 Jul 2021 18:24:00 +0200 Paolo Abeni wrote:
include/linux/skbuff.h +1 -0
index 3ff18300d210..b1e5bbfcc926 100644
@@ -992,6 +992,7 @@ static inline struct dst_entry *skb_dst(const struct sk_buff *skb)
  */
 static inline void skb_dst_set(struct sk_buff *skb, struct dst_entry *dst)
 {
+	skb->slow_gro |= !!dst;
 	skb->_skb_refdst = (unsigned long)dst;
 }

include/linux/skbuff.h static inline void skb_dst_set(struct sk_buff *skb, struct dst_entry *dst) · line 1012

Quoted 4 lines
include/linux/skbuff.h +1 -0
@@ -1008,6 +1009,7 @@ static inline void skb_dst_set(struct sk_buff *skb, struct dst_entry *dst)
 static inline void skb_dst_set_noref(struct sk_buff *skb, struct dst_entry *dst)
 {
 	WARN_ON(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
+	skb->slow_gro = !!dst;

why is this one = and not |= ?

Quoted 2 lines
include/linux/skbuff.h +0 -0
 	skb->_skb_refdst = (unsigned long)dst | SKB_DST_NOREF;
 }
author maintainer
#4 lore raw

Re: [PATCH net-next 2/6] sk_buff: track dst status in slow_gro

Quoted 13 lines · Jakub Kicinski
Quoted 12 lines · On Wed, 28 Jul 2021 18:24:00 +0200 Paolo Abeni wrote:
include/linux/skbuff.h +1 -0
index 3ff18300d210..b1e5bbfcc926 100644
@@ -992,6 +992,7 @@ static inline struct dst_entry *skb_dst(const struct sk_buff *skb)
  */
 static inline void skb_dst_set(struct sk_buff *skb, struct dst_entry *dst)
 {
+	skb->slow_gro |= !!dst;
 	skb->_skb_refdst = (unsigned long)dst;
 }
Quoted 7 lines

include/linux/skbuff.h static inline void skb_dst_set(struct sk_buff *skb, struct dst_entry *dst) · line 1012

Quoted 5 lines
include/linux/skbuff.h +1 -0
@@ -1008,6 +1009,7 @@ static inline void skb_dst_set(struct sk_buff *skb, struct dst_entry *dst)
 static inline void skb_dst_set_noref(struct sk_buff *skb, struct dst_entry *dst)
 {
 	WARN_ON(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
+	skb->slow_gro = !!dst;

why is this one = and not |= ?

Mostly because I'm dumb. Sabrina Dubroca noted that already. I'll send a follow-up ASAP.

Thanks!

Paolo

author maintainer
#5 lore raw

PATCH 3/6 sk_buff: track extension status in slow_gro

In reply to Paolo Abeni

Similar to the previous one, but tracking the
active_extensions field status.

Signed-off-by Paolo Abeni <[email protected]>
---
 net/core/skbuff.c | 1 +
 1 file changed, 1 insertion(+)
net/core/skbuff.c +1 -0
index 517ee2c36425..a990e11c393c 100644
@@ -6455,6 +6455,7 @@ void *skb_ext_add(struct sk_buff *skb, enum skb_ext_id id)
 	new->chunks = newlen;
 	new->offset[id] = newoff;
 set_active:
+	skb->slow_gro = 1;
 	skb->extensions = new;
 	skb->active_extensions |= 1 << id;
 	return skb_ext_get_ptr(new, id);
Signature · 1 line
2.26.3
author maintainer
#6 lore raw

PATCH 4/6 net: optimize GRO for the common case.

In reply to Paolo Abeni

After the previous patches, at GRO time, skb->slow_gro is usually 0, unless the packets comes from some H/W offload slowpath or tunnel.

We can optimize the GRO code assuming !skb->slow_gro is likely. This remove multiple conditionals in the most common path, at the price of an additional one when we hit the above "slow-paths".

Signed-off-by Paolo Abeni <[email protected]>
---
 net/core/dev.c    | 30 ++++++++++++++++++++++--------
 net/core/skbuff.c |  9 ++++++---
 2 files changed, 28 insertions(+), 11 deletions(-)
net/core/dev.c +22 -8
index fb5d12a3d52d..19565f7497ee 100644
@@ -6014,7 +6014,6 @@ static void gro_list_prepare(const struct list_head *head,
 		diffs |= skb_vlan_tag_present(p) ^ skb_vlan_tag_present(skb);
 		if (skb_vlan_tag_present(p))
 			diffs |= skb_vlan_tag_get(p) ^ skb_vlan_tag_get(skb);
-		diffs |= skb_metadata_dst_cmp(p, skb);
 		diffs |= skb_metadata_differs(p, skb);
 		if (maclen == ETH_HLEN)
 			diffs |= compare_ether_header(skb_mac_header(p),

@@ -6024,17 +6023,29 @@ static void gro_list_prepare(const struct list_head *head,
 				       skb_mac_header(skb),
 				       maclen);
 
-		diffs |= skb_get_nfct(p) ^ skb_get_nfct(skb);
+		/* in most common scenarions _state is 0
+		 * otherwise we are already on some slower paths
+		 * either skip all the infrequent tests altogether or
+		 * avoid trying too hard to skip each of them individually
+		 */
+		if (!diffs && unlikely(skb->slow_gro | p->slow_gro)) {
+#if IS_ENABLED(CONFIG_SKB_EXTENSIONS) && IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
+			struct tc_skb_ext *skb_ext;
+			struct tc_skb_ext *p_ext;
+#endif
+
+			diffs |= skb_metadata_dst_cmp(p, skb);
+			diffs |= skb_get_nfct(p) ^ skb_get_nfct(skb);
+
 #if IS_ENABLED(CONFIG_SKB_EXTENSIONS) && IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
-		if (!diffs) {
-			struct tc_skb_ext *skb_ext = skb_ext_find(skb, TC_SKB_EXT);
-			struct tc_skb_ext *p_ext = skb_ext_find(p, TC_SKB_EXT);
+			skb_ext = skb_ext_find(skb, TC_SKB_EXT);
+			p_ext = skb_ext_find(p, TC_SKB_EXT);
 
 			diffs |= (!!p_ext) ^ (!!skb_ext);
 			if (!diffs && unlikely(skb_ext))
 				diffs |= p_ext->chain ^ skb_ext->chain;
-		}
 #endif
+		}
 
 		NAPI_GRO_CB(p)->same_flow = !diffs;
 	}

@@ -6299,8 +6310,11 @@ static void napi_reuse_skb(struct napi_struct *napi, struct sk_buff *skb)
 	skb->encapsulation = 0;
 	skb_shinfo(skb)->gso_type = 0;
 	skb->truesize = SKB_TRUESIZE(skb_end_offset(skb));
-	skb_ext_reset(skb);
-	nf_reset_ct(skb);
+	if (unlikely(skb->slow_gro)) {
+		skb_ext_reset(skb);
+		nf_reset_ct(skb);
+		skb->slow_gro = 0;
+	}
 
 	napi->skb = skb;
 }
net/core/skbuff.c +6 -3
index a990e11c393c..8231cbddb6ed 100644
@@ -954,9 +954,12 @@ void __kfree_skb_defer(struct sk_buff *skb)
 
 void napi_skb_free_stolen_head(struct sk_buff *skb)
 {
-	nf_reset_ct(skb);
-	skb_dst_drop(skb);
-	skb_ext_put(skb);
+	if (unlikely(skb->slow_gro)) {
+		nf_reset_ct(skb);
+		skb_dst_drop(skb);
+		skb_ext_put(skb);
+		skb->slow_gro = 0;
+	}
 	napi_skb_cache_put(skb);
 }
Signature · 1 line
2.26.3
author maintainer
#7 lore raw

PATCH 5/6 skbuff: allow 'slow_gro' for skb carring sock reference

In reply to Paolo Abeni

This change leverages the infrastructure introduced by the previous patches to allow soft devices passing to the GRO engine owned skbs without impacting the fast-path.

It's up to the GRO caller ensuring the slow_gro bit validity before invoking the GRO engine. The new helper skb_prepare_for_gro() is introduced for that goal.

On slow_gro, skbs are aggregated only with equal sk. Additionally, skb truesize on GRO recycle and free is correctly updated so that sk wmem is not changed by the GRO processing.

rfc-> v1:
- fixed bad truesize on dev_gro_receive NAPI_FREE
- use the existing state bit

Signed-off-by Paolo Abeni <[email protected]>
---
 include/net/sock.h |  9 +++++++++
 net/core/dev.c     |  2 ++
 net/core/skbuff.c  | 17 +++++++++++++----
 3 files changed, 24 insertions(+), 4 deletions(-)
include/net/sock.h +9 -0
index f23cb259b0e2..ff1be7e7e90b 100644
@@ -2249,6 +2249,15 @@ static inline __must_check bool skb_set_owner_sk_safe(struct sk_buff *skb, struc
 	return false;
 }
 
+static inline void skb_prepare_for_gro(struct sk_buff *skb)
+{
+	if (skb->destructor != sock_wfree) {
+		skb_orphan(skb);
+		return;
+	}
+	skb->slow_gro = 1;
+}
+
 void sk_reset_timer(struct sock *sk, struct timer_list *timer,
 		    unsigned long expires);
net/core/dev.c +2 -0
index 19565f7497ee..dcc87fcd64ba 100644
@@ -6034,6 +6034,7 @@ static void gro_list_prepare(const struct list_head *head,
 			struct tc_skb_ext *p_ext;
 #endif
 
+			diffs |= p->sk != skb->sk;
 			diffs |= skb_metadata_dst_cmp(p, skb);
 			diffs |= skb_get_nfct(p) ^ skb_get_nfct(skb);
 

@@ -6311,6 +6312,7 @@ static void napi_reuse_skb(struct napi_struct *napi, struct sk_buff *skb)
 	skb_shinfo(skb)->gso_type = 0;
 	skb->truesize = SKB_TRUESIZE(skb_end_offset(skb));
 	if (unlikely(skb->slow_gro)) {
+		skb_orphan(skb);
 		skb_ext_reset(skb);
 		nf_reset_ct(skb);
 		skb->slow_gro = 0;
net/core/skbuff.c +13 -4
index 8231cbddb6ed..9510cb0807bc 100644
@@ -958,6 +958,7 @@ void napi_skb_free_stolen_head(struct sk_buff *skb)
 		nf_reset_ct(skb);
 		skb_dst_drop(skb);
 		skb_ext_put(skb);
+		skb_orphan(skb);
 		skb->slow_gro = 0;
 	}
 	napi_skb_cache_put(skb);

@@ -3898,6 +3899,9 @@ int skb_gro_receive_list(struct sk_buff *p, struct sk_buff *skb)
 	NAPI_GRO_CB(p)->last = skb;
 	NAPI_GRO_CB(p)->count++;
 	p->data_len += skb->len;
+
+	/* sk owenrship - if any - completely transferred to the aggregated packet */
+	skb->destructor = NULL;
 	p->truesize += skb->truesize;
 	p->len += skb->len;
 

@@ -4265,6 +4269,7 @@ int skb_gro_receive(struct sk_buff *p, struct sk_buff *skb)
 	unsigned int headlen = skb_headlen(skb);
 	unsigned int len = skb_gro_len(skb);
 	unsigned int delta_truesize;
+	unsigned int new_truesize;
 	struct sk_buff *lp;
 
 	if (unlikely(p->len + len >= 65536 || NAPI_GRO_CB(skb)->flush))

@@ -4296,10 +4301,10 @@ int skb_gro_receive(struct sk_buff *p, struct sk_buff *skb)
 		skb_frag_size_sub(frag, offset);
 
 		/* all fragments truesize : remove (head size + sk_buff) */
-		delta_truesize = skb->truesize -
-				 SKB_TRUESIZE(skb_end_offset(skb));
+		new_truesize = SKB_TRUESIZE(skb_end_offset(skb));
+		delta_truesize = skb->truesize - new_truesize;
 
-		skb->truesize -= skb->data_len;
+		skb->truesize = new_truesize;
 		skb->len -= skb->data_len;
 		skb->data_len = 0;
 

@@ -4328,12 +4333,16 @@ int skb_gro_receive(struct sk_buff *p, struct sk_buff *skb)
 		memcpy(frag + 1, skbinfo->frags, sizeof(*frag) * skbinfo->nr_frags);
 		/* We dont need to clear skbinfo->nr_frags here */
 
-		delta_truesize = skb->truesize - SKB_DATA_ALIGN(sizeof(struct sk_buff));
+		new_truesize = SKB_TRUESIZE(sizeof(struct sk_buff));
+		delta_truesize = skb->truesize - new_truesize;
+		skb->truesize = new_truesize;
 		NAPI_GRO_CB(skb)->free = NAPI_GRO_FREE_STOLEN_HEAD;
 		goto done;
 	}
 
 merge:
+	/* sk owenrship - if any - completely transferred to the aggregated packet */
+	skb->destructor = NULL;
 	delta_truesize = skb->truesize;
 	if (offset > headlen) {
 		unsigned int eat = offset - headlen;
Signature · 1 line
2.26.3
author maintainer
#8 lore raw

PATCH 6/6 veth: use skb_prepare_for_gro()

In reply to Paolo Abeni

Leveraging the previous patch we can now avoid orphaning the skb in the veth gro path, allowing correct backpressure.

Signed-off-by Paolo Abeni <[email protected]>
---
 drivers/net/veth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/veth.c +1 -1
index 381670c08ba7..50eb43e5bf45 100644
@@ -713,7 +713,7 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
 	int mac_len, delta, off;
 	struct xdp_buff xdp;
 
-	skb_orphan_partial(skb);
+	skb_prepare_for_gro(skb);
 
 	rcu_read_lock();
 	xdp_prog = rcu_dereference(rq->xdp_prog);
Signature · 1 line
2.26.3
CI Applied
#9 lore raw
In reply to Paolo Abeni
This series was applied to netdev/net-next.git (refs/heads/master):

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

Quoted 10 lines · On Wed, 28 Jul 2021 18:23:58 +0200 you wrote:

This is a trimmed down revision of "sk_buff: optimize layout for GRO", specifically dropping the changes to the sk_buff layout[1].

This series tries to accomplish 2 goals:
- optimize the GRO stage for the most common scenario, avoiding a bunch of conditional and some more code
- let owned skbs entering the GRO engine, allowing backpressure in the veth GRO forward path.

[...]

Here is the summary with links:
- [net-next,1/6] sk_buff: introduce 'slow_gro' flags

    https://git.kernel.org/netdev/net-next/c/5fc88f93edf2
  - [net-next,2/6] sk_buff: track dst status in slow_gro
    https://git.kernel.org/netdev/net-next/c/8a886b142bd0
  - [net-next,3/6] sk_buff: track extension status in slow_gro
    https://git.kernel.org/netdev/net-next/c/b0999f385ac3
  - [net-next,4/6] net: optimize GRO for the common case.
    https://git.kernel.org/netdev/net-next/c/9efb4b5baf6c
  - [net-next,5/6] skbuff: allow 'slow_gro' for skb carring sock reference
    https://git.kernel.org/netdev/net-next/c/5e10da5385d2
  - [net-next,6/6] veth: use skb_prepare_for_gro()
    https://git.kernel.org/netdev/net-next/c/d504fff0d14a

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html