Skip to contentSkip to conversation

net: add destructor for skb data (rewritten)

PATCH In review Rusty Russell 3 participants 5 messages Apr 18, 2008 – Apr 19, 2008

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Rusty Russell

author
#0 lore raw

PATCH 1/1 net: add destructor for skb data (rewritten)

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.

This turns out to be slightly non-trivial as fragments from one skb get copied to another skb: if the first skb has a destructor (or its parent does) we need to keep a reference to it and destroy it only when (all the) children are destroyed. We add an 'orig' pointer to the skb_shared_info to do this.

But there's currently no way to get from the shinfo to the head (to kfree it), so we add a 'len' field. A better alternative to this might be to move the skb_shared_info to before the head of the skb data.

Note that the destructor is responsible for calling kfree: for the tun device, this is critical since the destructor can be called from any context and it has to do a copy_to_user, so it queues the skb.

Signed-off-by Rusty Russell <[email protected]>
---
 include/linux/skbuff.h |    9 +++++++
 net/core/skbuff.c      |   62 ++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 60 insertions(+), 11 deletions(-)
include/linux/skbuff.h +9 -0
diff -r 78decd088c5d include/linux/skbuff.h
@@ -146,8 +146,12 @@ struct skb_shared_info {
 	unsigned short	gso_segs;
 	unsigned short  gso_type;
 	__be32          ip6_frag_id;
+	unsigned int	len; /* Subtract from this shinfo to find skb->head */
 	struct sk_buff	*frag_list;
 	skb_frag_t	frags[MAX_SKB_FRAGS];
+	struct skb_shared_info *orig;
+	/* This is responsible for kfree() of header. */
+	void		(*destructor)(struct skb_shared_info *);
 };
 
 /* We divide dataref into two halves.  The higher 16 bits hold references

@@ -852,6 +856,11 @@ static inline void skb_fill_page_desc(st
 #define SKB_FRAG_ASSERT(skb) 	BUG_ON(skb_shinfo(skb)->frag_list)
 #define SKB_LINEAR_ASSERT(skb)  BUG_ON(skb_is_nonlinear(skb))
 
+static inline unsigned char *skb_shinfo_to_head(struct skb_shared_info *shinfo)
+{
+	return (unsigned char *)shinfo - shinfo->len;
+}
+
 #ifdef NET_SKBUFF_DATA_USES_OFFSET
 static inline unsigned char *skb_tail_pointer(const struct sk_buff *skb)
 {
net/core/skbuff.c +51 -11

108 lines not on this page · raw message

maintainer
#1 lore raw

From: Rusty Russell <[email protected]>
Date: Fri, 18 Apr 2008 14:21:25 +1000

Quoted 13 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.

This turns out to be slightly non-trivial as fragments from one skb get copied to another skb: if the first skb has a destructor (or its parent does) we need to keep a reference to it and destroy it only when (all the) children are destroyed. We add an 'orig' pointer to the skb_shared_info to do this.

But there's currently no way to get from the shinfo to the head (to kfree it), so we add a 'len' field. A better alternative to this might be to move the skb_shared_info to before the head of the skb data.

Quoted 5 lines

Note that the destructor is responsible for calling kfree: for the tun device, this is critical since the destructor can be called from any context and it has to do a copy_to_user, so it queues the skb.

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

I'm mostly ambivalent but I will say I'm not happy about all of this extra state you're adding even though it's "only" to the SKB data shared-info struct and not sk_buff properly.

Does this handle SKB frags of arbitrary depth? SKB's can be nested to arbitrary depths via the frag mechanism.

author
#2 lore raw
Quoted 20 lines · On Saturday 19 April 2008 19:35:24 David Miller wrote:

From: Rusty Russell <[email protected]>
Date: Fri, 18 Apr 2008 14:21:25 +1000

Quoted 17 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.

This turns out to be slightly non-trivial as fragments from one skb get copied to another skb: if the first skb has a destructor (or its parent does) we need to keep a reference to it and destroy it only when (all the) children are destroyed. We add an 'orig' pointer to the skb_shared_info to do this.

But there's currently no way to get from the shinfo to the head (to kfree it), so we add a 'len' field. A better alternative to this might be to move the skb_shared_info to before the head of the skb data.

Note that the destructor is responsible for calling kfree: for the tun device, this is critical since the destructor can be called from any context and it has to do a copy_to_user, so it queues the skb.

Quoted 5 lines
Quoted 1 line
Signed-off-by Rusty Russell <[email protected]>

I'm mostly ambivalent but I will say I'm not happy about all of this extra state you're adding even though it's "only" to the SKB data shared-info struct and not sk_buff properly.

Me neither. Moving the shared_info to the front of the data would reduce it to two fields for me (removing len and destructor arg), but I held off for now because this is a lesser change and possible for 2.6.26.

Quoted 2 lines

Does this handle SKB frags of arbitrary depth? SKB's can be nested to arbitrary depths via the frag mechanism.

It doesn't matter in this case. It's the skb creator who sets the destructor, and wants it called when all pages in shinfo->frags[] are done with. If it wanted to also include the frag_list in this lifetime, it would simply set ->orig on those skbs's shinfo to point back to the head shinfo, and adjust dataref accordingly.

As long as the anyone referencing a frags page from an skb into another sets the orig ptr & bumps dataref, this will work. If someone kept a reference to a page and then freed the skb it game from, we're already broken.

You could think of it as a 'struct request' for networking. Or not.

Cheers,
Rusty.

#3 lore raw
In reply to Rusty Russell
Quoted 2 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.

Can we put its invokation before pages are freed?

Quoted 11 lines
net/core/skbuff.c +11 -0
+static void shinfo_put(struct skb_shared_info *shinfo, bool nohdr)
+{
+	struct skb_shared_info *orig;
+
+	do {
+		if (atomic_sub_return(nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1,
+				      &shinfo->dataref))
+			return;
+
+		if (shinfo->nr_frags) {
+			int i;

net/core/skbuff.c static void skb_clone_fraglist(struct sk · line 315

Quoted 10 lines
net/core/skbuff.c +10 -0
+			for (i = 0; i < shinfo->nr_frags; i++)
+				put_page(shinfo->frags[i].page);
+		}
+
+		if (shinfo->frag_list)
+			skb_drop_list(&shinfo->frag_list);
+
+		orig = shinfo->orig;
+		if (shinfo->destructor)
+			shinfo->destructor(shinfo);

If it is first, we can process handle pages inside destructor, otherwise they can be out of our control.

Quoted 7 lines
net/core/skbuff.c +7 -0
+		else
+			kfree(skb_shinfo_to_head(shinfo));
+
+		/* We hold a payload reference to our parent. */
+		nohdr = true;
+	} while ((shinfo = orig) != NULL);
+}
Signature · 1 line
	Evgeniy Polyakov
author
#4 lore raw
Quoted 1 line · On Saturday 19 April 2008 19:46:01 Evgeniy Polyakov wrote:

On Fri, Apr 18, 2008 at 02:21:25PM +1000, Rusty Russell

Quoted 4 lines · ([email protected]) wrote:
Quoted 2 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.

Can we put its invokation before pages are freed?

Sure. I don't need that, but it's easy to change.

Cheers,
Rusty.