[PATCH net-next] net: cleanup and document skb fclone layout

[PATCH net-next] net: cleanup and document skb fclone layout

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Lets use a proper structure to clearly document and implement
skb fast clones.

Then, we might experiment more easily alternative layouts.

This patch adds a new skb_fclone_busy() helper, used by tcp and xfrm.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/linux/skbuff.h |   25 ++++++++++++++++++++++
 net/core/skbuff.c      |   44 +++++++++++++++++++--------------------
 net/ipv4/tcp_output.c  |    5 ----
 net/xfrm/xfrm_policy.c |    4 ---
 4 files changed, 49 insertions(+), 29 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 8eaa62400fca..ed5fc2faa074 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -775,6 +775,31 @@ struct sk_buff *alloc_skb_with_frags(unsigned long header_len,
 				     int *errcode,
 				     gfp_t gfp_mask);
 
+/* Layout of fast clones : [skb1][skb2][fclone_ref] */
+struct sk_buff_fclones {
+	struct sk_buff	skb1;
+
+	struct sk_buff	skb2;
+
+	atomic_t	fclone_ref;
+};
+
+/**
+ *	skb_fclone_busy - check if fclone is busy
+ *	@skb: buffer
+ *
+ * Returns true is skb is a fast clone, and its clone is not freed.
+ */
+static inline bool skb_fclone_busy(const struct sk_buff *skb)
+{
+	const struct sk_buff_fclones *fclones;
+
+	fclones = container_of(skb, struct sk_buff_fclones, skb1);
+
+	return skb->fclone == SKB_FCLONE_ORIG &&
+	       fclones->skb2.fclone == SKB_FCLONE_CLONE;
+}
+
 static inline struct sk_buff *alloc_skb_fclone(unsigned int size,
 					       gfp_t priority)
 {
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index d4fdc649112c..460f5f4f12a9 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -257,16 +257,18 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
 	kmemcheck_annotate_variable(shinfo->destructor_arg);
 
 	if (flags & SKB_ALLOC_FCLONE) {
-		struct sk_buff *child = skb + 1;
-		atomic_t *fclone_ref = (atomic_t *) (child + 1);
+		struct sk_buff_fclones *fclones;
 
-		kmemcheck_annotate_bitfield(child, flags1);
-		kmemcheck_annotate_bitfield(child, flags2);
+		fclones = container_of(skb, struct sk_buff_fclones, skb1);
+
+		kmemcheck_annotate_bitfield(&fclones->skb2, flags1);
+		kmemcheck_annotate_bitfield(&fclones->skb2, flags2);
 		skb->fclone = SKB_FCLONE_ORIG;
-		atomic_set(fclone_ref, 1);
 
-		child->fclone = SKB_FCLONE_UNAVAILABLE;
-		child->pfmemalloc = pfmemalloc;
+		atomic_set(&fclones->fclone_ref, 1);
+
+		fclones->skb2.fclone = SKB_FCLONE_UNAVAILABLE;
+		fclones->skb2.pfmemalloc = pfmemalloc;
 	}
 out:
 	return skb;
@@ -525,8 +527,7 @@ static void skb_release_data(struct sk_buff *skb)
  */
 static void kfree_skbmem(struct sk_buff *skb)
 {
-	struct sk_buff *other;
-	atomic_t *fclone_ref;
+	struct sk_buff_fclones *fclones;
 
 	switch (skb->fclone) {
 	case SKB_FCLONE_UNAVAILABLE:
@@ -534,22 +535,21 @@ static void kfree_skbmem(struct sk_buff *skb)
 		break;
 
 	case SKB_FCLONE_ORIG:
-		fclone_ref = (atomic_t *) (skb + 2);
-		if (atomic_dec_and_test(fclone_ref))
-			kmem_cache_free(skbuff_fclone_cache, skb);
+		fclones = container_of(skb, struct sk_buff_fclones, skb1);
+		if (atomic_dec_and_test(&fclones->fclone_ref))
+			kmem_cache_free(skbuff_fclone_cache, fclones);
 		break;
 
 	case SKB_FCLONE_CLONE:
-		fclone_ref = (atomic_t *) (skb + 1);
-		other = skb - 1;
+		fclones = container_of(skb, struct sk_buff_fclones, skb2);
 
 		/* The clone portion is available for
 		 * fast-cloning again.
 		 */
 		skb->fclone = SKB_FCLONE_UNAVAILABLE;
 
-		if (atomic_dec_and_test(fclone_ref))
-			kmem_cache_free(skbuff_fclone_cache, other);
+		if (atomic_dec_and_test(&fclones->fclone_ref))
+			kmem_cache_free(skbuff_fclone_cache, fclones);
 		break;
 	}
 }
@@ -856,17 +856,18 @@ EXPORT_SYMBOL_GPL(skb_copy_ubufs);
 
 struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
 {
-	struct sk_buff *n;
+	struct sk_buff_fclones *fclones = container_of(skb,
+						       struct sk_buff_fclones,
+						       skb1);
+	struct sk_buff *n = &fclones->skb2;
 
 	if (skb_orphan_frags(skb, gfp_mask))
 		return NULL;
 
-	n = skb + 1;
 	if (skb->fclone == SKB_FCLONE_ORIG &&
 	    n->fclone == SKB_FCLONE_UNAVAILABLE) {
-		atomic_t *fclone_ref = (atomic_t *) (n + 1);
 		n->fclone = SKB_FCLONE_CLONE;
-		atomic_inc(fclone_ref);
+		atomic_inc(&fclones->fclone_ref);
 	} else {
 		if (skb_pfmemalloc(skb))
 			gfp_mask |= __GFP_MEMALLOC;
@@ -3238,8 +3239,7 @@ void __init skb_init(void)
 					      SLAB_HWCACHE_ALIGN|SLAB_PANIC,
 					      NULL);
 	skbuff_fclone_cache = kmem_cache_create("skbuff_fclone_cache",
-						(2*sizeof(struct sk_buff)) +
-						sizeof(atomic_t),
+						sizeof(struct sk_buff_fclones),
 						0,
 						SLAB_HWCACHE_ALIGN|SLAB_PANIC,
 						NULL);
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index f173b1c4f815..fd72d381d924 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2091,10 +2091,7 @@ bool tcp_schedule_loss_probe(struct sock *sk)
 static bool skb_still_in_host_queue(const struct sock *sk,
 				    const struct sk_buff *skb)
 {
-	const struct sk_buff *fclone = skb + 1;
-
-	if (unlikely(skb->fclone == SKB_FCLONE_ORIG &&
-		     fclone->fclone == SKB_FCLONE_CLONE)) {
+	if (unlikely(skb_fclone_busy(skb))) {
 		NET_INC_STATS_BH(sock_net(sk),
 				 LINUX_MIB_TCPSPURIOUS_RTX_HOSTQUEUES);
 		return true;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index fdde51f4271a..b31cb8354c97 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1844,10 +1844,8 @@ static int xdst_queue_output(struct sock *sk, struct sk_buff *skb)
 	struct xfrm_dst *xdst = (struct xfrm_dst *) dst;
 	struct xfrm_policy *pol = xdst->pols[0];
 	struct xfrm_policy_queue *pq = &pol->polq;
-	const struct sk_buff *fclone = skb + 1;
 
-	if (unlikely(skb->fclone == SKB_FCLONE_ORIG &&
-		     fclone->fclone == SKB_FCLONE_CLONE)) {
+	if (unlikely(skb_fclone_busy(skb))) {
 		kfree_skb(skb);
 		return 0;
 	}

Re: [PATCH net-next] net: cleanup and document skb fclone layout

[Eric Dumazet]

On Sat, 2014-09-27 at 16:25 -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Lets use a proper structure to clearly document and implement
> skb fast clones.
> 
> Then, we might experiment more easily alternative layouts.
> 
> This patch adds a new skb_fclone_busy() helper, used by tcp and xfrm.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  include/linux/skbuff.h |   25 ++++++++++++++++++++++
>  net/core/skbuff.c      |   44 +++++++++++++++++++--------------------
>  net/ipv4/tcp_output.c  |    5 ----
>  net/xfrm/xfrm_policy.c |    4 ---
>  4 files changed, 49 insertions(+), 29 deletions(-)


I'll rebase this patch and send a v2.

Re: [PATCH net-next] net: cleanup and document skb fclone layout

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 29 Sep 2014 10:23:36 -0700

> On Sat, 2014-09-27 at 16:25 -0700, Eric Dumazet wrote:
>> From: Eric Dumazet <edumazet@google.com>
>> 
>> Lets use a proper structure to clearly document and implement
>> skb fast clones.
>> 
>> Then, we might experiment more easily alternative layouts.
>> 
>> This patch adds a new skb_fclone_busy() helper, used by tcp and xfrm.
>> 
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> ---
>>  include/linux/skbuff.h |   25 ++++++++++++++++++++++
>>  net/core/skbuff.c      |   44 +++++++++++++++++++--------------------
>>  net/ipv4/tcp_output.c  |    5 ----
>>  net/xfrm/xfrm_policy.c |    4 ---
>>  4 files changed, 49 insertions(+), 29 deletions(-)
> 
> 
> I'll rebase this patch and send a v2.

Thanks.

[PATCH v2 net-next] net: cleanup and document skb fclone layout

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Lets use a proper structure to clearly document and implement
skb fast clones.

Then, we might experiment more easily alternative layouts.

This patch adds a new skb_fclone_busy() helper, used by tcp and xfrm,
to stop leaking of implementation details.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
v2: rebased on latest net-next

 include/linux/skbuff.h |   25 +++++++++++++++++++++++
 net/core/skbuff.c      |   41 +++++++++++++++++++--------------------
 net/ipv4/tcp_output.c  |    5 ----
 net/xfrm/xfrm_policy.c |    4 ---
 4 files changed, 47 insertions(+), 28 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 262efdbc346b..d8f7d74d5a4d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -781,6 +781,31 @@ struct sk_buff *alloc_skb_with_frags(unsigned long header_len,
 				     int *errcode,
 				     gfp_t gfp_mask);
 
+/* Layout of fast clones : [skb1][skb2][fclone_ref] */
+struct sk_buff_fclones {
+	struct sk_buff	skb1;
+
+	struct sk_buff	skb2;
+
+	atomic_t	fclone_ref;
+};
+
+/**
+ *	skb_fclone_busy - check if fclone is busy
+ *	@skb: buffer
+ *
+ * Returns true is skb is a fast clone, and its clone is not freed.
+ */
+static inline bool skb_fclone_busy(const struct sk_buff *skb)
+{
+	const struct sk_buff_fclones *fclones;
+
+	fclones = container_of(skb, struct sk_buff_fclones, skb1);
+
+	return skb->fclone == SKB_FCLONE_ORIG &&
+	       fclones->skb2.fclone == SKB_FCLONE_CLONE;
+}
+
 static inline struct sk_buff *alloc_skb_fclone(unsigned int size,
 					       gfp_t priority)
 {
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 4be570a4ab21..a8cebb40699c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -257,15 +257,16 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
 	kmemcheck_annotate_variable(shinfo->destructor_arg);
 
 	if (flags & SKB_ALLOC_FCLONE) {
-		struct sk_buff *child = skb + 1;
-		atomic_t *fclone_ref = (atomic_t *) (child + 1);
+		struct sk_buff_fclones *fclones;
 
-		kmemcheck_annotate_bitfield(child, flags1);
+		fclones = container_of(skb, struct sk_buff_fclones, skb1);
+
+		kmemcheck_annotate_bitfield(&fclones->skb2, flags1);
 		skb->fclone = SKB_FCLONE_ORIG;
-		atomic_set(fclone_ref, 1);
+		atomic_set(&fclones->fclone_ref, 1);
 
-		child->fclone = SKB_FCLONE_UNAVAILABLE;
-		child->pfmemalloc = pfmemalloc;
+		fclones->skb2.fclone = SKB_FCLONE_UNAVAILABLE;
+		fclones->skb2.pfmemalloc = pfmemalloc;
 	}
 out:
 	return skb;
@@ -524,8 +525,7 @@ static void skb_release_data(struct sk_buff *skb)
  */
 static void kfree_skbmem(struct sk_buff *skb)
 {
-	struct sk_buff *other;
-	atomic_t *fclone_ref;
+	struct sk_buff_fclones *fclones;
 
 	switch (skb->fclone) {
 	case SKB_FCLONE_UNAVAILABLE:
@@ -533,22 +533,21 @@ static void kfree_skbmem(struct sk_buff *skb)
 		break;
 
 	case SKB_FCLONE_ORIG:
-		fclone_ref = (atomic_t *) (skb + 2);
-		if (atomic_dec_and_test(fclone_ref))
-			kmem_cache_free(skbuff_fclone_cache, skb);
+		fclones = container_of(skb, struct sk_buff_fclones, skb1);
+		if (atomic_dec_and_test(&fclones->fclone_ref))
+			kmem_cache_free(skbuff_fclone_cache, fclones);
 		break;
 
 	case SKB_FCLONE_CLONE:
-		fclone_ref = (atomic_t *) (skb + 1);
-		other = skb - 1;
+		fclones = container_of(skb, struct sk_buff_fclones, skb2);
 
 		/* The clone portion is available for
 		 * fast-cloning again.
 		 */
 		skb->fclone = SKB_FCLONE_UNAVAILABLE;
 
-		if (atomic_dec_and_test(fclone_ref))
-			kmem_cache_free(skbuff_fclone_cache, other);
+		if (atomic_dec_and_test(&fclones->fclone_ref))
+			kmem_cache_free(skbuff_fclone_cache, fclones);
 		break;
 	}
 }
@@ -859,17 +858,18 @@ EXPORT_SYMBOL_GPL(skb_copy_ubufs);
 
 struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
 {
-	struct sk_buff *n;
+	struct sk_buff_fclones *fclones = container_of(skb,
+						       struct sk_buff_fclones,
+						       skb1);
+	struct sk_buff *n = &fclones->skb2;
 
 	if (skb_orphan_frags(skb, gfp_mask))
 		return NULL;
 
-	n = skb + 1;
 	if (skb->fclone == SKB_FCLONE_ORIG &&
 	    n->fclone == SKB_FCLONE_UNAVAILABLE) {
-		atomic_t *fclone_ref = (atomic_t *) (n + 1);
 		n->fclone = SKB_FCLONE_CLONE;
-		atomic_inc(fclone_ref);
+		atomic_inc(&fclones->fclone_ref);
 	} else {
 		if (skb_pfmemalloc(skb))
 			gfp_mask |= __GFP_MEMALLOC;
@@ -3240,8 +3240,7 @@ void __init skb_init(void)
 					      SLAB_HWCACHE_ALIGN|SLAB_PANIC,
 					      NULL);
 	skbuff_fclone_cache = kmem_cache_create("skbuff_fclone_cache",
-						(2*sizeof(struct sk_buff)) +
-						sizeof(atomic_t),
+						sizeof(struct sk_buff_fclones),
 						0,
 						SLAB_HWCACHE_ALIGN|SLAB_PANIC,
 						NULL);
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index ee567e9e98c3..8d4eac793700 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2110,10 +2110,7 @@ bool tcp_schedule_loss_probe(struct sock *sk)
 static bool skb_still_in_host_queue(const struct sock *sk,
 				    const struct sk_buff *skb)
 {
-	const struct sk_buff *fclone = skb + 1;
-
-	if (unlikely(skb->fclone == SKB_FCLONE_ORIG &&
-		     fclone->fclone == SKB_FCLONE_CLONE)) {
+	if (unlikely(skb_fclone_busy(skb))) {
 		NET_INC_STATS_BH(sock_net(sk),
 				 LINUX_MIB_TCPSPURIOUS_RTX_HOSTQUEUES);
 		return true;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index f623dca6ce30..4c4e457e7888 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1961,10 +1961,8 @@ static int xdst_queue_output(struct sock *sk, struct sk_buff *skb)
 	struct xfrm_dst *xdst = (struct xfrm_dst *) dst;
 	struct xfrm_policy *pol = xdst->pols[0];
 	struct xfrm_policy_queue *pq = &pol->polq;
-	const struct sk_buff *fclone = skb + 1;
 
-	if (unlikely(skb->fclone == SKB_FCLONE_ORIG &&
-		     fclone->fclone == SKB_FCLONE_CLONE)) {
+	if (unlikely(skb_fclone_busy(skb))) {
 		kfree_skb(skb);
 		return 0;
 	}

Re: [PATCH v2 net-next] net: cleanup and document skb fclone layout

[Eric Dumazet]

On Mon, 2014-09-29 at 13:29 -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 

>  struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
>  {
> -	struct sk_buff *n;
> +	struct sk_buff_fclones *fclones = container_of(skb,
> +						       struct sk_buff_fclones,
> +						       skb1);
> +	struct sk_buff *n = &fclones->skb2;
>  
>  	if (skb_orphan_frags(skb, gfp_mask))
>  		return NULL;
>  
> -	n = skb + 1;
>  	if (skb->fclone == SKB_FCLONE_ORIG &&
>  	    n->fclone == SKB_FCLONE_UNAVAILABLE) {
> -		atomic_t *fclone_ref = (atomic_t *) (n + 1);
>  		n->fclone = SKB_FCLONE_CLONE;
> -		atomic_inc(fclone_ref);
> +		atomic_inc(&fclones->fclone_ref);
>  	} else {
>  		if (skb_pfmemalloc(skb))
>  			gfp_mask |= __GFP_MEMALLOC;

Note : 

The "atomic_inc(&fclones->fclone_ref)" here can be replaced by an
atomic_set(&fclones->fclone_ref, 2), to avoid a locked operation.


I'll cook a patch for this, after you merged this cleanup

Thanks

Re: [PATCH v2 net-next] net: cleanup and document skb fclone layout

[Vijay Subramanian]

>         case SKB_FCLONE_CLONE:
> -               fclone_ref = (atomic_t *) (skb + 1);
> -               other = skb - 1;
> +               fclones = container_of(skb, struct sk_buff_fclones, skb2);
>
>                 /* The clone portion is available for
>                  * fast-cloning again.
>                  */
>                 skb->fclone = SKB_FCLONE_UNAVAILABLE;
>

It looks like SKB_FCLONE_UNAVAILABLE has overloaded use depending on
whether we are referring to skb allocated from head_cache or from
fclone_cache. In the former case, it means this skb cannot be cloned
i.e.(clone is unavailable) but in latter case, it means clone is free
to be used.
This overleading is confusing as seen in the comment in snippet above.

Should we add  something like SKB_CLONE_FREE for the second case that
indicates that clone is free for use.?  I can send a patch if it makes
sense..

-vijay

Re: [PATCH v2 net-next] net: cleanup and document skb fclone layout

[Eric Dumazet]

On Wed, 2014-10-01 at 00:38 -0700, Vijay Subramanian wrote:
> >         case SKB_FCLONE_CLONE:
> > -               fclone_ref = (atomic_t *) (skb + 1);
> > -               other = skb - 1;
> > +               fclones = container_of(skb, struct sk_buff_fclones, skb2);
> >
> >                 /* The clone portion is available for
> >                  * fast-cloning again.
> >                  */
> >                 skb->fclone = SKB_FCLONE_UNAVAILABLE;
> >
> 
> It looks like SKB_FCLONE_UNAVAILABLE has overloaded use depending on
> whether we are referring to skb allocated from head_cache or from
> fclone_cache. In the former case, it means this skb cannot be cloned
> i.e.(clone is unavailable) but in latter case, it means clone is free
> to be used.
> This overleading is confusing as seen in the comment in snippet above.
> 
> Should we add  something like SKB_CLONE_FREE for the second case that
> indicates that clone is free for use.?  I can send a patch if it makes
> sense..

FCLONE_UNAVAILABLE always had two meanings, one for a normal skb telling
there is no fclone companion, one for the companion telling he is free.

It would be indeed clear to have SKB_CLONE_FREE for the companion.

Re: [PATCH v2 net-next] net: cleanup and document skb fclone layout

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 29 Sep 2014 13:29:15 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> Lets use a proper structure to clearly document and implement
> skb fast clones.
> 
> Then, we might experiment more easily alternative layouts.
> 
> This patch adds a new skb_fclone_busy() helper, used by tcp and xfrm,
> to stop leaking of implementation details.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> v2: rebased on latest net-next

Applied, thank you.