net: add destructor for skb data.

PATCH In review Rusty Russell 2 participants 6 messages Apr 5, 2008 – Apr 6, 2008

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Rusty Russell

author

2008-04-05 11:56 UTC #0 lore raw

PATCH 1/1 net: add destructor for skb data.

If we want to notify something when an skb is truly finished (such as for tun vringfd support), we need a destructor on the data. We don't need to add other fields, since we can just allocate extra room at the end.

(I wonder if we could *reduce* the shinfo allocation where no frags are needed?)

Signed-off-by Rusty Russell <[email protected]>

---
 include/linux/skbuff.h |   29 ++++++++++++++++++++++++++---
 net/core/skbuff.c      |   12 +++++++++---
 2 files changed, 35 insertions(+), 6 deletions(-)

include/linux/skbuff.h +5 -3

diff -r 77871c14566e include/linux/skbuff.h
@@ -148,6 +148,7 @@ struct skb_shared_info {
 	__be32          ip6_frag_id;
 	struct sk_buff	*frag_list;
 	skb_frag_t	frags[MAX_SKB_FRAGS];
+	void		(*destructor)(struct skb_shared_info *);
 };
 
 /* We divide dataref into two halves.  The higher 16 bits hold references

@@ -344,17 +346,18 @@ extern void kfree_skb(struct sk_buff *sk
 extern void kfree_skb(struct sk_buff *skb);
 extern void	       __kfree_skb(struct sk_buff *skb);
 extern struct sk_buff *__alloc_skb(unsigned int size,
-				   gfp_t priority, int fclone, int node);
+				   gfp_t priority, int fclone, unsigned extra,
+				   int node);
 static inline struct sk_buff *alloc_skb(unsigned int size,
 					gfp_t priority)
 {
-	return __alloc_skb(size, priority, 0, -1);
+	return __alloc_skb(size, priority, 0, 0, -1);
 }
 
 static inline struct sk_buff *alloc_skb_fclone(unsigned int size,
 					       gfp_t priority)
 {
-	return __alloc_skb(size, priority, 1, -1);
+	return __alloc_skb(size, priority, 1, 0, -1);
 }
 
 extern struct sk_buff *skb_morph(struct sk_buff *dst, struct sk_buff *src);

net/core/skbuff.c +9 -3

diff -r 77871c14566e net/core/skbuff.c
@@ -169,6 +169,7 @@ EXPORT_SYMBOL(skb_truesize_bug);
  *	@gfp_mask: allocation mask
  *	@fclone: allocate from fclone cache instead of head cache
  *		and allocate a cloned (child) skb
+ *	@extra: extra bytes at end of shinfo.
  *	@node: numa node to allocate memory on
  *
  *	Allocate a new &sk_buff. The returned buffer has no headroom and a

@@ -179,7 +180,7 @@ EXPORT_SYMBOL(skb_truesize_bug);
  *	%GFP_ATOMIC.
  */
 struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
-			    int fclone, int node)
+			    int fclone, unsigned extra, int node)
 {
 	struct kmem_cache *cache;
 	struct skb_shared_info *shinfo;

@@ -194,7 +195,8 @@ struct sk_buff *__alloc_skb(unsigned int
 		goto out;
 
 	size = SKB_DATA_ALIGN(size);
-	data = kmalloc_node_track_caller(size + sizeof(struct skb_shared_info),
+	data = kmalloc_node_track_caller(size + sizeof(struct skb_shared_info)
+					 + extra,
 			gfp_mask, node);
 	if (!data)
 		goto nodata;

@@ -218,6 +220,7 @@ struct sk_buff *__alloc_skb(unsigned int
 	shinfo->gso_type = 0;
 	shinfo->ip6_frag_id = 0;
 	shinfo->frag_list = NULL;
+	shinfo->destructor = NULL;
 
 	if (fclone) {
 		struct sk_buff *child = skb + 1;

@@ -255,7 +258,7 @@ struct sk_buff *__netdev_alloc_skb(struc
 	int node = dev->dev.parent ? dev_to_node(dev->dev.parent) : -1;
 	struct sk_buff *skb;
 
-	skb = __alloc_skb(length + NET_SKB_PAD, gfp_mask, 0, node);
+	skb = __alloc_skb(length + NET_SKB_PAD, gfp_mask, 0, 0, node);
 	if (likely(skb)) {
 		skb_reserve(skb, NET_SKB_PAD);
 		skb->dev = dev;

@@ -302,6 +305,9 @@ static void skb_release_data(struct sk_b
 
 		if (skb_shinfo(skb)->frag_list)
 			skb_drop_fraglist(skb);
+
+		if (skb_shinfo(skb)->destructor)
+			skb_shinfo(skb)->destructor(skb_shinfo(skb));
 
 		kfree(skb->head);
 	}

Evgeniy Polyakov

2008-04-05 13:07 UTC #1 lore raw

Hi.

Quoted 4 lines · Rusty Russell

If we want to notify something when an skb is truly finished (such as for tun vringfd support), we need a destructor on the data. We don't need to add other fields, since we can just allocate extra room at the end.

There is skb->destructor already, for your case of vringfd it can be used safely afaics.

Signature · 1 line

	Evgeniy Polyakov

Rusty Russell

author

2008-04-06 03:21 UTC #2 lore raw

Quoted 3 lines · On Saturday 05 April 2008 23:06:48 Evgeniy Polyakov wrote:

Hi.
On Sat, Apr 05, 2008 at 09:56:05PM +1000, Rusty Russell

Quoted 7 lines · ([email protected]) wrote:

Quoted 4 lines
If we want to notify something when an skb is truly finished (such as for tun vringfd support), we need a destructor on the data. We don't need to add other fields, since we can just allocate extra room at the end.
There is skb->destructor already, for your case of vringfd it can be used safely afaics.

Hi Evgeniy,

I don't think so. For a start, the skb destructor is called while the skb
is still in the socket queue (ie. the data is still live). Secondly, the original skb can be freed while clones still reference the data.

Cheers,
Rusty.

Evgeniy Polyakov

2008-04-06 09:20 UTC #3 lore raw

Hi Rusty.

Quoted 3 lines · Rusty Russell

I don't think so. For a start, the skb destructor is called while the skb
is still in the socket queue (ie. the data is still live). Secondly, the original skb can be freed while clones still reference the data.

That is what it is for - to remove data from any queues and free it. One can check if skb was cloned and do not perform some steps, instead call old destructor. Destructor for the last clone will cleanup whatever is needed. Thoughts?

Actually I'm not that opposed agains additional destructor, I just want to bring attention to this topic, since this is second time some steps are going to be setup for the destruction time, so I want a clear solution :)

Signature · 1 line

	Evgeniy Polyakov

Evgeniy Polyakov

2008-04-06 13:32 UTC #4 lore raw

Hi.

Quoted 4 lines · Evgeniy Polyakov

Actually I'm not that opposed agains additional destructor, I just want to bring attention to this topic, since this is second time some steps are going to be setup for the destruction time, so I want a clear solution :)

Actually the question is who is allowed to set that callback? Essentially what I want is to get the same notifications as you propose in patch 4 but for socket layer. AFAICS this will not collide with tun/tap skbs.

Signature · 1 line

	Evgeniy Polyakov

Rusty Russell

author

2008-04-06 21:10 UTC #5 lore raw

In reply to Evgeniy Polyakov

Quoted 3 lines · On Sunday 06 April 2008 19:20:18 Evgeniy Polyakov wrote:

Hi Rusty.
On Sun, Apr 06, 2008 at 01:20:59PM +1000, Rusty Russell

Quoted 9 lines · ([email protected]) wrote:

Quoted 4 lines
I don't think so. For a start, the skb destructor is called while
the skb is still in the socket queue (ie. the data is still live). Secondly, the original skb can be freed while clones still reference the data.
That is what it is for - to remove data from any queues and free it. One can check if skb was cloned and do not perform some steps, instead call old destructor. Destructor for the last clone will cleanup whatever is needed. Thoughts?

The old destructor is in some other skb, you'd have to carry it around. And skb_orphan() calls the destructor early deliberately.

The current skb destructor is for the sk_buff, not the data. It's clearest to keep them separate.

Cheers,
Rusty.