net: add destructor for skb data (rewritten)
Subsystems (2)
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.
--- include/linux/skbuff.h | 9 +++++++ net/core/skbuff.c | 62 ++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 60 insertions(+), 11 deletions(-)
include/linux/skbuff.h +9 -0
@@ -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
David Miller
maintainerFrom: 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_infoto 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_infoto 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.
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
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.
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 +1000Quoted 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_infoto 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_infoto 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_buffproperly.
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.