LinuxLists.cc - [RFC PATCH] net: add dataref destructor to sk

2009-10-02 14:20:09

Subject: [RFC PATCH] net: add dataref destructor to sk_buff

(Applies to davem/net-2.6.git:4fdb78d30)

Hi David, netdevs,

The following is an RFC for an attempt at addressing a zero-copy solution.

To be perfectly honest, I have no idea if this is the best solution, or if
there is truly a problem with skb->destructor that requires an alternate
mechanism. What I do know is that this patch seems to work, and I would
like to see some kind of solution available upstream. So I thought I would
send my hack out as at least a point of discussion. FWIW: This has been
tested heavily in my rig and is technically suitable for inclusion after
review as is, if that is decided to be the optimal path forward here.

Thanks for your review and consideration,

Kind regards,
-Greg

----------------------------------------
From: Gregory Haskins <[email protected]>
Subject: [RFC PATCH] net: add dataref destructor to sk_buff

What: The skb->destructor field is reportedly unreliable for ensuring
that all shinfo users have dropped their references. Therefore, we add
a distinct ->release() method for the shinfo structure which is closely
tied to the underlying page resources we want to protect.

Why: We want to add zero-copy transmit support for AlacrityVM guests.
In order to support this, the host kernel must map guest pages directly
into a paged-skb and send it as normal. put_page() alone is not
sufficient lifetime management since the pages are ultimately allocated
from within the guest. Therefore, we need higher-level notification
when the skb is finally freed on the host so we can then inject a proper
"tx-complete" event into the guest context.

Signed-off-by: Gregory Haskins <[email protected]>
---

include/linux/skbuff.h | 2 ++
net/core/skbuff.c | 9 +++++++++
2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index df7b23a..02cdab6 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -207,6 +207,8 @@ struct skb_shared_info {
/* Intermediate layers must ensure that destructor_arg
* remains valid until skb destructor */
void * destructor_arg;
+ void * priv;
+ void (*release)(struct sk_buff *skb);
};

/* We divide dataref into two halves. The higher 16 bits hold references
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 80a9616..a7e40a9 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -219,6 +219,8 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
shinfo->tx_flags.flags = 0;
skb_frag_list_init(skb);
memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
+ shinfo->release = NULL;
+ shinfo->priv = NULL;

if (fclone) {
struct sk_buff *child = skb + 1;
@@ -350,6 +352,9 @@ static void skb_release_data(struct sk_buff *skb)
if (skb_has_frags(skb))
skb_drop_fraglist(skb);

+ if (skb_shinfo(skb)->release)
+ skb_shinfo(skb)->release(skb);
+
kfree(skb->head);
}
}
@@ -514,6 +519,8 @@ int skb_recycle_check(struct sk_buff *skb, int skb_size)
shinfo->tx_flags.flags = 0;
skb_frag_list_init(skb);
memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
+ shinfo->release = NULL;
+ shinfo->priv = NULL;

memset(skb, 0, offsetof(struct sk_buff, tail));
skb->data = skb->head + NET_SKB_PAD;
@@ -856,6 +863,8 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
skb->hdr_len = 0;
skb->nohdr = 0;
atomic_set(&skb_shinfo(skb)->dataref, 1);
+ skb_shinfo(skb)->release = NULL;
+ skb_shinfo(skb)->priv = NULL;
return 0;

nodata:

2009-11-06 05:07:49

by David Miller

[permalink] [raw]

Subject: Re: [RFC PATCH] net: add dataref destructor to sk_buff

From: Gregory Haskins <[email protected]>
Date: Fri, 02 Oct 2009 10:20:00 -0400

> The following is an RFC for an attempt at addressing a zero-copy solution.
>
> To be perfectly honest, I have no idea if this is the best solution, or if
> there is truly a problem with skb->destructor that requires an alternate
> mechanism. What I do know is that this patch seems to work, and I would
> like to see some kind of solution available upstream. So I thought I would
> send my hack out as at least a point of discussion. FWIW: This has been
> tested heavily in my rig and is technically suitable for inclusion after
> review as is, if that is decided to be the optimal path forward here.
>
> Thanks for your review and consideration,

I have no fundamental objections, but when you submit this for
real can you show the code that uses it so we can get a better
idea about things?

Thanks.

2009-11-06 16:08:51

[permalink] [raw]

Subject: Re: [RFC PATCH] net: add dataref destructor to sk_buff

Stephen Hemminger wrote:
> On Fri, 13 Nov 2009 21:27:57 -0500
> Gregory Haskins <[email protected]> wrote:
>
>> Herbert Xu wrote:
>>> On Fri, Nov 13, 2009 at 08:33:35PM -0500, Gregory Haskins wrote:
>>>> Well, not with respect to the overall protocol, of course not. But with
>>>> respect to the buffer in question, it _has_ to be. Or am I missing
>>>> something?
>>> sendfile() has never guaranteed that the kernel is finished with
>>> the underlying pages when it returns.
>>>
>>> Cheers,
>> Clearly there must be _some_ mechanism to synchronize (e.g.
>> flush/barrier) though, right? Otherwise, that interface would seem to
>> be quite prone to races and would likely be unusable. So what does
>> said flush use to know when the buffer is free?
>
> No all the interfaces require a copy.

I'm sorry, but I do not think that is correct. As others have pointed
out, that would not appear to be true for at least sendfile.

> Actually, sendfile makes no guarantee about synchronization
> because the receiver of said file could be arbitrarily slow, and any attempt at locking down
> current contents of file is a denial of service exposure.

I think you are inverting the problem space. It is fully expected that
changing the "file", or the pages that represent the file before the
packet is queued would constitute the modification of the stream on the
wire.

I am more thinking about the applications of mmap+sendfile to implement
a zero-copy interface. As David mentions in another mail, it seems that
there is no sync mechanism available, so this would not appear to be a
viable use case today, unfortunately.

>
> People have tried doing copy-less send by page flipping, but the overhead of the IPI to
> invalidate the TLB exceeded the overhead of the copy. There was an Intel paper on this in
> at Linux Symposium (Ottawa) several years ago.

I think you are confusing copy-less tx with copy-less rx. You can try
to do copy-less rx with page flipping, which has the IPI/TLB thrashing
properties you mention, and I agree is problematic. We are talking
about copy-less tx here, however, and therefore no page-flipping is
involved. Rather, we are just posting SG lists of pages directly to the
NIC (assuming the nic supports HIGH_DMA, etc). You do not need to flip
the page, or invalidate the TLB (and thus IPI the other cores) to do
this to my knowledge.

Kind Regards,
-Greg

Attachments:

signature.asc (267.00 B)
OpenPGP digital signature

2009-11-16 14:23:00

Stephen Hemminger wrote:
> On Sat, 14 Nov 2009 00:27:46 -0500
> Gregory Haskins <[email protected]> wrote:
>
>> Stephen Hemminger wrote:

>
>>> People have tried doing copy-less send by page flipping, but the overhead of the IPI to
>>> invalidate the TLB exceeded the overhead of the copy. There was an Intel paper on this in
>>> at Linux Symposium (Ottawa) several years ago.
>> I think you are confusing copy-less tx with copy-less rx. You can try
>> to do copy-less rx with page flipping, which has the IPI/TLB thrashing
>> properties you mention, and I agree is problematic. We are talking
>> about copy-less tx here, however, and therefore no page-flipping is
>> involved. Rather, we are just posting SG lists of pages directly to the
>> NIC (assuming the nic supports HIGH_DMA, etc). You do not need to flip
>> the page, or invalidate the TLB (and thus IPI the other cores) to do
>> this to my knowledge.
>>
>
> If you want to do copy-less tx for all applications, you have to
> do COW to handle the trivial case of :
>
> while (cc = read(infd, buffer, sizeof buffer)) {
> send(outsock, buffer, cc);
> }
>
>

You certainly _could_ implement this as a COW I suppose, but that would
be insane. If someone did do this, you are right: you need TLB
invalidation.

However, if I were going to actually propose the changeover of the
system calls to use zero-copy (note that I am not), it would be based on
the concept in this patch. That is: the send() would block until the
NIC completes the DMA and the shinfo block is freed. Alternate
implementations would be AIO based, where the shinfo destructor
signifies the generation of the completion event.

FWIW: The latter is conceptually similar to how this is being used in
AlacrityVM.

HTH

Kind Regards,
-Greg

Attachments:

signature.asc (267.00 B)
OpenPGP digital signature

2009-11-17 01:02:43

by Herbert Xu

[permalink] [raw]

Subject: Re: [RFC PATCH] net: add dataref destructor to sk_buff

On Mon, Nov 16, 2009 at 09:22:57AM -0500, Gregory Haskins wrote:
>
> But really, this is somewhat orthogonal to the original problem, so let
> me see if we can bring it back on topic. Michael stated that this patch
> in question may be problematic because there are places in the stack
> that can get_page() without also maintaining a reference to the shinfo
> object. Evgeniy seems to say the opposite. I am not sure who is right,
> or if I misunderstood one or both of them. Any thoughts?

There are loads of places where this can happen. Start with
pskb_expand_head.

Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2009-11-17 12:33:47

by Gregory Haskins

[permalink] [raw]

Subject: Re: [RFC PATCH] net: add dataref destructor to sk_buff

Herbert Xu wrote:
> On Mon, Nov 16, 2009 at 09:22:57AM -0500, Gregory Haskins wrote:
>> But really, this is somewhat orthogonal to the original problem, so let
>> me see if we can bring it back on topic. Michael stated that this patch
>> in question may be problematic because there are places in the stack
>> that can get_page() without also maintaining a reference to the shinfo
>> object. Evgeniy seems to say the opposite. I am not sure who is right,
>> or if I misunderstood one or both of them. Any thoughts?
>
> There are loads of places where this can happen. Start with
> pskb_expand_head.

Indeed, I see your point. Looks like we can potentially solve that with
an extra level of indirection. E.g. skb->shinfo->owner, with a
ref-count+callback. Thoughts?

Kind Regards,
-Greg

Attachments:

signature.asc (267.00 B)
OpenPGP digital signature