[NET]: Get rid of alloc_skb code duplication

[NET]: Get rid of alloc_skb code duplication

[Herbert Xu]

Hi Dave:

[NET]: Get rid of alloc_skb code duplication

The skb initialisation code is shared between alloc_skb and
alloc_skb_from_cache.  Now I don't know what actually uses
the latter so perhaps we could simply get rid of it.

However, if we're to keep it, then let's move the common code
out as they've started diverging.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index fae7f94..db4f526 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -128,6 +128,31 @@ EXPORT_SYMBOL(skb_truesize_bug);
  *
  */
 
+static void init_skb(struct sk_buff *skb, u8 *data, unsigned int size)
+{
+	struct skb_shared_info *shinfo;
+
+	/*
+	 * See comment in sk_buff definition, just before the 'tail' member
+	 */
+	memset(skb, 0, offsetof(struct sk_buff, tail));
+	skb->truesize = size + sizeof(struct sk_buff);
+	atomic_set(&skb->users, 1);
+	skb->head = data;
+	skb->data = data;
+	skb_reset_tail_pointer(skb);
+	skb->end = skb->tail + size;
+	/* make sure we initialize shinfo sequentially */
+	shinfo = skb_shinfo(skb);
+	atomic_set(&shinfo->dataref, 1);
+	shinfo->nr_frags  = 0;
+	shinfo->gso_size = 0;
+	shinfo->gso_segs = 0;
+	shinfo->gso_type = 0;
+	shinfo->ip6_frag_id = 0;
+	shinfo->frag_list = NULL;
+}
+
 /**
  *	__alloc_skb	-	allocate a network buffer
  *	@size: size to allocate
@@ -147,7 +172,6 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
 			    int fclone, int node)
 {
 	struct kmem_cache *cache;
-	struct skb_shared_info *shinfo;
 	struct sk_buff *skb;
 	u8 *data;
 
@@ -164,25 +188,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
 	if (!data)
 		goto nodata;
 
-	/*
-	 * See comment in sk_buff definition, just before the 'tail' member
-	 */
-	memset(skb, 0, offsetof(struct sk_buff, tail));
-	skb->truesize = size + sizeof(struct sk_buff);
-	atomic_set(&skb->users, 1);
-	skb->head = data;
-	skb->data = data;
-	skb_reset_tail_pointer(skb);
-	skb->end = skb->tail + size;
-	/* make sure we initialize shinfo sequentially */
-	shinfo = skb_shinfo(skb);
-	atomic_set(&shinfo->dataref, 1);
-	shinfo->nr_frags  = 0;
-	shinfo->gso_size = 0;
-	shinfo->gso_segs = 0;
-	shinfo->gso_type = 0;
-	shinfo->ip6_frag_id = 0;
-	shinfo->frag_list = NULL;
+	init_skb(skb, data, size);
 
 	if (fclone) {
 		struct sk_buff *child = skb + 1;
@@ -233,23 +239,8 @@ struct sk_buff *alloc_skb_from_cache(struct kmem_cache *cp,
 	data = kmem_cache_alloc(cp, gfp_mask);
 	if (!data)
 		goto nodata;
-	/*
-	 * See comment in sk_buff definition, just before the 'tail' member
-	 */
-	memset(skb, 0, offsetof(struct sk_buff, tail));
-	skb->truesize = size + sizeof(struct sk_buff);
-	atomic_set(&skb->users, 1);
-	skb->head = data;
-	skb->data = data;
-	skb_reset_tail_pointer(skb);
-	skb->end  = skb->tail + size;
 
-	atomic_set(&(skb_shinfo(skb)->dataref), 1);
-	skb_shinfo(skb)->nr_frags  = 0;
-	skb_shinfo(skb)->gso_size = 0;
-	skb_shinfo(skb)->gso_segs = 0;
-	skb_shinfo(skb)->gso_type = 0;
-	skb_shinfo(skb)->frag_list = NULL;
+	init_skb(skb, data, size);
 out:
 	return skb;
 nodata:

Re: [NET]: Get rid of alloc_skb code duplication

[Christoph Hellwig]

On Tue, Apr 17, 2007 at 09:57:46PM +1000, Herbert Xu wrote:
> Hi Dave:
> 
> [NET]: Get rid of alloc_skb code duplication
> 
> The skb initialisation code is shared between alloc_skb and
> alloc_skb_from_cache.  Now I don't know what actually uses
> the latter so perhaps we could simply get rid of it.

It was put in for Xen, but if you as the resident beat the crap out
of Xen networking guru don't know about it we should probably just
kill it.  Especially as we usually kill dead code pretty fast.

Re: [NET]: Get rid of alloc_skb code duplication

[Herbert Xu]

On Tue, Apr 17, 2007 at 02:03:47PM +0100, Christoph Hellwig wrote:
>
> It was put in for Xen, but if you as the resident beat the crap out
> of Xen networking guru don't know about it we should probably just
> kill it.  Especially as we usually kill dead code pretty fast.

Heh, I wasn't touching Xen back then.

That's good news actually.  Xen has now killed their custom alloc_skb
so this is no longer needed.  Besides, even if they needed it they'll
have to patch it in anyway since you've already killed the ifdef that
lets them have a custom alloc_skb :)

[NET]: Get rid of alloc_skb_from_cache

Since this was added originally for Xen, and Xen has recently (~2.6.18)
stopped using this function, we can safely get rid of it.  Good timing
too since this function has started to bit rot.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 691ead8..a9b810d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -342,9 +342,6 @@ static inline struct sk_buff *alloc_skb_fclone(unsigned int size,
 	return __alloc_skb(size, priority, 1, -1);
 }
 
-extern struct sk_buff *alloc_skb_from_cache(struct kmem_cache *cp,
-					    unsigned int size,
-					    gfp_t priority);
 extern void	       kfree_skbmem(struct sk_buff *skb);
 extern struct sk_buff *skb_clone(struct sk_buff *skb,
 				 gfp_t priority);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index fae7f94..2d8707e 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -202,63 +202,6 @@ nodata:
 }
 
 /**
- *	alloc_skb_from_cache	-	allocate a network buffer
- *	@cp: kmem_cache from which to allocate the data area
- *           (object size must be big enough for @size bytes + skb overheads)
- *	@size: size to allocate
- *	@gfp_mask: allocation mask
- *
- *	Allocate a new &sk_buff. The returned buffer has no headroom and
- *	tail room of size bytes. The object has a reference count of one.
- *	The return is the buffer. On a failure the return is %NULL.
- *
- *	Buffers may only be allocated from interrupts using a @gfp_mask of
- *	%GFP_ATOMIC.
- */
-struct sk_buff *alloc_skb_from_cache(struct kmem_cache *cp,
-				     unsigned int size,
-				     gfp_t gfp_mask)
-{
-	struct sk_buff *skb;
-	u8 *data;
-
-	/* Get the HEAD */
-	skb = kmem_cache_alloc(skbuff_head_cache,
-			       gfp_mask & ~__GFP_DMA);
-	if (!skb)
-		goto out;
-
-	/* Get the DATA. */
-	size = SKB_DATA_ALIGN(size);
-	data = kmem_cache_alloc(cp, gfp_mask);
-	if (!data)
-		goto nodata;
-	/*
-	 * See comment in sk_buff definition, just before the 'tail' member
-	 */
-	memset(skb, 0, offsetof(struct sk_buff, tail));
-	skb->truesize = size + sizeof(struct sk_buff);
-	atomic_set(&skb->users, 1);
-	skb->head = data;
-	skb->data = data;
-	skb_reset_tail_pointer(skb);
-	skb->end  = skb->tail + size;
-
-	atomic_set(&(skb_shinfo(skb)->dataref), 1);
-	skb_shinfo(skb)->nr_frags  = 0;
-	skb_shinfo(skb)->gso_size = 0;
-	skb_shinfo(skb)->gso_segs = 0;
-	skb_shinfo(skb)->gso_type = 0;
-	skb_shinfo(skb)->frag_list = NULL;
-out:
-	return skb;
-nodata:
-	kmem_cache_free(skbuff_head_cache, skb);
-	skb = NULL;
-	goto out;
-}
-
-/**
  *	__netdev_alloc_skb - allocate an skbuff for rx on a specific device
  *	@dev: network device to receive on
  *	@length: length to allocate

Re: [NET]: Get rid of alloc_skb code duplication

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 18 Apr 2007 00:17:43 +1000

> On Tue, Apr 17, 2007 at 02:03:47PM +0100, Christoph Hellwig wrote:
> >
> > It was put in for Xen, but if you as the resident beat the crap out
> > of Xen networking guru don't know about it we should probably just
> > kill it.  Especially as we usually kill dead code pretty fast.
> 
> Heh, I wasn't touching Xen back then.
> 
> That's good news actually.  Xen has now killed their custom alloc_skb
> so this is no longer needed.  Besides, even if they needed it they'll
> have to patch it in anyway since you've already killed the ifdef that
> lets them have a custom alloc_skb :)
> 
> [NET]: Get rid of alloc_skb_from_cache
> 
> Since this was added originally for Xen, and Xen has recently (~2.6.18)
> stopped using this function, we can safely get rid of it.  Good timing
> too since this function has started to bit rot.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied.  You missed the arch/x86_64/kernel/functionlist reference
but I took care of that for you :-)

I'm going to push this into 2.6.21 because the sooner this dies
the better, even though it will make for some net-2.6.22 merge
hassles for me.