copy aside frags with destructors (was [PATCH 7/9] net: add skb_orphan_frags to copy aside frags with destructors)

Quoted 8 lines · Michael S. Tsirkin

Quoted 7 lines · David Miller

From: "Michael S. Tsirkin" <[email protected]>
Date: Fri, 4 May 2012 00:10:24 +0300

Quoted 2 lines

Hmm we orphan skbs when we loop them back so how about reusing the skb->destructor for this?

That's one possibility.

Quoted 5 lines

Quoted 5 lines

But I fear we're about to toss Ian into yet another rabbit hole. :-)

Let's try to converge on something quickly as I think integration of his work has been delayed enough as-is.

Quoted 2 lines

It's weekend here, I'll work on a patch like this
Sunday.

Thanks, I was starting to feel my nose twitching and my ears beginning to elongate ;-)

Ian.

net/core/skbuff.c +2 -0

+		if (unlikely((!uarg && !f->page.destructor)))
+			continue;
 		__skb_fill_page_desc(skb, i-1, head, 0,
 				     skb_shinfo(skb)->frags[i - 1].size);
 		head = (struct page *)head->private;

Review

Quoted 3 lines · Michael S. Tsirkin

(quoted diff) +1 -0

 	/* skb frags point to kernel buffers */
 	for (i = skb_shinfo(skb)->nr_frags; i > 0; i--) {
+		skb_frag_t *f = &skb_shinfo(skb)->frags[i];

This needs to be ....->frags[i - 1]

otherwise you put every new frag one too high and don't do anything to frag 0, which leaves the old destructor pointer in place and leads to a double free.

I think skb_frag_set_destructor and skb_copy_frag_destructor need to clear and propagate respectively (or maybe just clear in both cases) the destructor_arg field since it is otherwise not initialised when we set SKBTX_DEV_ZEROCOPY and that can trigger wrong behaviour in this function.

Ian.

Review

Quoted 5 lines

(quoted diff) +2 -0

+		if (unlikely((!uarg && !f->page.destructor)))
+			continue;
 		__skb_fill_page_desc(skb, i-1, head, 0,
 				     skb_shinfo(skb)->frags[i - 1].size);
 		head = (struct page *)head->private;

Quoted 1 line · Ian Campbell

On Mon, 2012-05-07 at 14:54 +0100, Michael S. Tsirkin wrote:

So the below on top then. I pushed these on
top of my zerocopy branch - can you confirm pls?

---

include/linux/skbuff.h +3 -1

index 930a50e..e52bc8d 100644
@@ -1270,8 +1270,10 @@ static inline void skb_frag_set_destructor(struct sk_buff *skb, int i,
 {
 	skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
 	frag->page.destructor = destroy;
-	if (destroy)
+	if (destroy) {
 		skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
+		skb_shinfo(skb)->destructor_arg = NULL;
+	}
 }
 
 /**

net/core/skbuff.c +2 -3

index b7fc47e..453f621 100644
@@ -753,12 +753,11 @@ int skb_copy_ubufs(struct sk_buff *skb, gfp_t gfp_mask)
 		uarg->callback(uarg);
 
 	/* skb frags point to kernel buffers */
-	for (i = skb_shinfo(skb)->nr_frags; i > 0; i--) {
+	for (i = skb_shinfo(skb)->nr_frags - 1; i >= 0; i--) {
 		skb_frag_t *f = &skb_shinfo(skb)->frags[i];
 		if (unlikely((!uarg && !f->page.destructor)))
 			continue;
-		__skb_fill_page_desc(skb, i-1, head, 0,
-				     skb_shinfo(skb)->frags[i - 1].size);
+		__skb_fill_page_desc(skb, i, head, 0, f->size);
 		head = (struct page *)head->private;
 	}
 

I'm seeing copy_ubufs called in my remote NFS test, which I don't think I expected -- I'll investigate why this is happening today.

Quoted 2 lines · Ian Campbell

I'm seeing copy_ubufs called in my remote NFS test, which I don't think I expected -- I'll investigate why this is happening today.

It's tcp_transmit_skb which can (conditionally) call skb_clone (backtrace below)

Quoted 4 lines · Ian Campbell

Quoted 2 lines · Ian Campbell

I'm seeing copy_ubufs called in my remote NFS test, which I don't think I expected -- I'll investigate why this is happening today.

Quoted 2 lines

It's tcp_transmit_skb which can (conditionally) call skb_clone (backtrace below)

Interesting. I didn't realise we clone skbs on data path:
tcp_write_xmit calls tcp_transmit_skb with clone_it flag. Could someone comment on why we need to clone on good path like this?

Signature · 1 line

MST

Quoted 8 lines · Michael S. Tsirkin

Quoted 6 lines · Ian Campbell

Quoted 2 lines · Ian Campbell

I'm seeing copy_ubufs called in my remote NFS test, which I don't think I expected -- I'll investigate why this is happening today.

It's tcp_transmit_skb which can (conditionally) call skb_clone (backtrace below)

Quoted 4 lines

Interesting. I didn't realise we clone skbs on data path:
tcp_write_xmit calls tcp_transmit_skb with clone_it flag. Could someone comment on why we need to clone on good path like this?

Hmm, it's in case we need to retransmit it later.

Quoted 9 lines · Michael S. Tsirkin

Quoted 8 lines · Michael S. Tsirkin

Quoted 6 lines · Ian Campbell

Quoted 2 lines · Ian Campbell

I'm seeing copy_ubufs called in my remote NFS test, which I don't think I expected -- I'll investigate why this is happening today.

It's tcp_transmit_skb which can (conditionally) call skb_clone (backtrace below)

Quoted 6 lines

Quoted 4 lines

Interesting. I didn't realise we clone skbs on data path:
tcp_write_xmit calls tcp_transmit_skb with clone_it flag. Could someone comment on why we need to clone on good path like this?

Hmm, it's in case we need to retransmit it later.

I wonder if we could avoid the copy_ubuf in this particular clone path and have any subsequent calls to copy_ubufs use skb->fclone to determine if it can safely replace the frags?

If it cannot then could it do a full copy of the skb (including new shinfo, new frag pages etc) as a fallback?

Ian.

Quoted 4 lines · Ian Campbell

Quoted 2 lines

I'm seeing copy_ubufs called in my remote NFS test, which I don't think I expected -- I'll investigate why this is happening today.

Quoted 2 lines

It's tcp_transmit_skb which can (conditionally) call skb_clone (backtrace below)

Interesting. I didn't realise we clone skbs on data path:
tcp_write_xmit calls tcp_transmit_skb with clone_it flag. Could someone comment on why we need to clone on good path like this?

Many places do

       if ((skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY))
		skb_copy_ubufs(skb, gfp_mask);

to copy and invoke frag destructors if necessary.
Add an inline helper for this.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 include/linux/skbuff.h |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

include/linux/skbuff.h +24 -0

index bb78f70..28d842e 100644
@@ -1711,6 +1711,30 @@ static inline void skb_orphan(struct sk_buff *skb)
 }
 
 /**
+ *	skb_orphan_frags - orphan the frags contained in a buffer
+ *	@skb: buffer to orphan frags from
+ *	@gfp_mask: allocation mask for replacement pages
+ *
+ *	For each frag in the SKB which needs a destructor (i.e. has an
+ *	owner) create a copy of that frag and release the original
+ *	page by calling the destructor.
+ */
+static inline int skb_orphan_frags(struct sk_buff *skb, gfp_t gfp_mask)
+{
+	if (likely(!(skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY)))
+		return 0;
+	return skb_copy_ubufs(skb, gfp_mask);
+}
+
+
+static inline void skb_copy_frag_destructor(struct sk_buff *to,
+					    struct sk_buff *from)
+{
+	skb_shinfo(to)->tx_flags |= skb_shinfo(from)->tx_flags &
+		SKBTX_DEV_ZEROCOPY;
+}
+
+/**
  *	__skb_queue_purge - empty a list
  *	@list: list to empty
  *

Signature · 1 line

MST

tun xmit is actually receive of the internal tun socket. Orphan the frags same as we do for normal rx path.

Signed-off-by Michael S. Tsirkin <[email protected]>

zero copy packets are normally sent to the outside network, but bridging, tun etc might loop them back to host networking stack. If this happens destructors will never be called, so orphan the frags immediately on receive.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 net/core/dev.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

net/core/dev.c +2 -0

index a2be59f..c0cdc00 100644
@@ -1630,6 +1630,8 @@ static inline int deliver_skb(struct sk_buff *skb,
 			      struct packet_type *pt_prev,
 			      struct net_device *orig_dev)
 {
+	if (unlikely(skb_orphan_frags(skb, GFP_ATOMIC)))
+		return -ENOMEM;
 	atomic_inc(&skb->users);
 	return pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
 }

Quoted 5 lines · Michael S. Tsirkin

zero copy packets are normally sent to the outside network, but bridging, tun etc might loop them back to host networking stack. If this happens destructors will never be called, so orphan the frags immediately on receive.

I think this deceptively simply patch is actually the meat of the series.
It's been a long time since I dug into the bridging code. Am I right that this function is only reached when an SKB is going to be delivered locally and not when forwarding to another port on the bridge?

Ian.

Quoted 19 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 net/core/dev.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

net/core/dev.c +2 -0

index a2be59f..c0cdc00 100644
@@ -1630,6 +1630,8 @@ static inline int deliver_skb(struct sk_buff *skb,
 			      struct packet_type *pt_prev,
 			      struct net_device *orig_dev)
 {
+	if (unlikely(skb_orphan_frags(skb, GFP_ATOMIC)))
+		return -ENOMEM;
 	atomic_inc(&skb->users);
 	return pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
 }

Quoted 5 lines · Michael S. Tsirkin

zero copy packets are normally sent to the outside network, but bridging, tun etc might loop them back to host networking stack. If this happens destructors will never be called, so orphan the frags immediately on receive.

I think this deceptively simply patch is actually the meat of the series.

set tx flags when adding a frag destructor to force frags to be orphaned as appropriate. copy when copying frags between skbs.
Note: rare false positives (where flag is set with no destructors) are harmless.

Signed-off-by Michael S. Tsirkin <[email protected]>

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

But I fear we're about to toss Ian into yet another rabbit hole. :-)

Let's try to converge on something quickly as I think integration of his work has been delayed enough as-is.

Here's a first stub at a fix. Basically to be able to modify frags on the fly we must make sure the skb isn't cloned, so the moment someone clones the skb we need to trigger the frag copy logic. And this is exactly what happens with SKBTX_DEV_ZEROCOPY so it seems to make sense to reuse that logic.

The below patchset replaces patch 7
([PATCH 7/9] net: add skb_orphan_frags to copy aside frags with destructors) in Ian's patchset and needs to be applied there.

Compiled only but I'd like to hear what people think all the same because it does add a couple of branches on fast path. On the other hand this makes it generic so the same logic will be reusable for packet sockets (which IIRC are currently buggy in the same way as sunrpc) and for adding zero copy support to tun.

Please comment,
Thanks!

Quoted 4 lines · Michael S. Tsirkin

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

But I fear we're about to toss Ian into yet another rabbit hole. :-)

Let's try to converge on something quickly as I think integration of his work has been delayed enough as-is.

...

Quoted 17 lines

Here's a first stub at a fix. Basically to be able to modify frags on the fly we must make sure the skb isn't cloned, so the moment someone clones the skb we need to trigger the frag copy logic. And this is exactly what happens with SKBTX_DEV_ZEROCOPY so it seems to make sense to reuse that logic.

The below patchset replaces patch 7
([PATCH 7/9] net: add skb_orphan_frags to copy aside frags with destructors) in Ian's patchset and needs to be applied there.

Compiled only but I'd like to hear what people think all the same because it does add a couple of branches on fast path. On the other hand this makes it generic so the same logic will be reusable for packet sockets (which IIRC are currently buggy in the same way as sunrpc) and for adding zero copy support to tun.

Quoted 2 lines

Please comment,
Thanks!

Ian, just to stress, need your ack that this does what you wanted.

Quoted 28 lines · Michael S. Tsirkin

Quoted 4 lines · Michael S. Tsirkin

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

But I fear we're about to toss Ian into yet another rabbit hole. :-)

Let's try to converge on something quickly as I think integration of his work has been delayed enough as-is.

...

Quoted 19 lines

Here's a first stub at a fix. Basically to be able to modify frags on the fly we must make sure the skb isn't cloned, so the moment someone clones the skb we need to trigger the frag copy logic. And this is exactly what happens with SKBTX_DEV_ZEROCOPY so it seems to make sense to reuse that logic.

The below patchset replaces patch 7
([PATCH 7/9] net: add skb_orphan_frags to copy aside frags with destructors) in Ian's patchset and needs to be applied there.

Compiled only but I'd like to hear what people think all the same because it does add a couple of branches on fast path. On the other hand this makes it generic so the same logic will be reusable for packet sockets (which IIRC are currently buggy in the same way as sunrpc) and for adding zero copy support to tun.

Please comment,
Thanks!

Quoted 1 line

Ian, just to stress, need your ack that this does what you wanted.

Sure, yesterday was a public holiday and today looks likely to be crazy busy. I'll try and have a look later in the week.

Ian.

Quoted 5 lines · Ian Campbell

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines · David Miller

From: "Michael S. Tsirkin" <[email protected]>
Date: Fri, 4 May 2012 00:10:24 +0300

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

Hmm we orphan skbs when we loop them back so how about reusing the skb->destructor for this?

That's one possibility.

So originally I thought it would work: destructor would wake the original owner which would do data copy and modify the fragments. But then I unfortunately realized that would be racy: the new owner could be

using the old frags and there appears no way for us to make sure it doesn't so that we can put the original page.

And the same logic applies to modifying the frags at any other time if the skb is cloned. So it seems we must copy if we want to clone the skb.

Second option is what macvtap zero copy uses and it already does copy on clone too. So I hacked that to make it support tcp/udp used by sunrpc.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

But I fear we're about to toss Ian into yet another rabbit hole. :-)

Let's try to converge on something quickly as I think integration of his work has been delayed enough as-is.

...

Quoted 7 lines

Quoted 2 lines

It's weekend here, I'll work on a patch like this
Sunday.

Thanks, I was starting to feel my nose twitching and my ears beginning to elongate ;-)

Ian.

Here's a first stub at a fix. Basically to be able to modify frags on the fly we must make sure the skb isn't cloned, so the moment someone clones the skb we need to trigger the frag copy logic. And this is exactly what happens with SKBTX_DEV_ZEROCOPY so it seems to make sense to reuse that logic.

The below patchset replaces patch 7
([PATCH 7/9] net: add skb_orphan_frags to copy aside frags with destructors) in Ian's patchset and needs to be applied there.

Compiled only but I'd like to hear what people think all the same because it does add a couple of branches on fast path. On the other hand this makes it generic so the same logic will be reusable for packet sockets (which IIRC are currently buggy in the same way as sunrpc) and for adding zero copy support to tun.

Please comment,
Thanks!

Signature · 16 lines

MST


Michael S. Tsirkin (6):
  skbuff: support per-page destructors in copy_ubufs
  skbuff: add an api to orphan frags
  skbuff: convert to skb_orphan_frags
  tun: orphan frags on xmit
  net: orphan frags on receive
  skbuff: set zerocopy flag on frag destructor

 drivers/net/tun.c      |    2 ++
 include/linux/skbuff.h |   41 +++++++++++++++++++++++++++++++++++++++++
 net/core/dev.c         |    2 ++
 net/core/skbuff.c      |   43 +++++++++++++++++++++++++------------------
 4 files changed, 70 insertions(+), 18 deletions(-)

Quoted 6 lines · Michael S. Tsirkin

Quoted 5 lines · Ian Campbell

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines · David Miller

From: "Michael S. Tsirkin" <[email protected]>
Date: Fri, 4 May 2012 00:10:24 +0300

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

Hmm we orphan skbs when we loop them back so how about reusing the skb->destructor for this?

That's one possibility.

So originally I thought it would work: destructor would wake the original owner which would do data copy and modify the fragments. But then I unfortunately realized that would be racy: the new owner could be

By "new owner" you mean the owner of some other skb which refernences the same shinfo (so either clone or the original if we currently hold a clone).

Quoted 2 lines

using the old frags and there appears no way for us to make sure it doesn't so that we can put the original page.

Right, the key issue is, I think, that cloning etc take an extra dataref on the shinfo but do not take references on the frags. This is obviously sane but does cause the issue above.

Quoted 3 lines

And the same logic applies to modifying the frags at any other time if the skb is cloned. So it seems we must copy if we want to clone the skb.

Right. I had been hoping that the frag destructors could avoid this but it seems like this is not going to be the case.

Just as a thought experiment would taking a frag ref on clone help us? (lets assume for now that, iff there are destructors, this is cheaper than the copy). I think this doesn't work -- since although the ref will mean that the page doesn't go away under anyone elses feet after we've orphaned it we will have lost the pointer to the original page by the time that other ref is dropped, so freeing it will be tricky.

So following that train of thought a step further -- would creating a new shinfo entirely on frag orphan buy us anything (and at what cost)? Obviously it an extra allocation -- but we are already allocating N pages of new frag. The others skb's referencing the shared shinfo would still continue to see the old shinfo, pages and refs until they themselves needed to orphan the frags (if they do, they might avoid it).

I think that could work, but I'm not sure it is worth the complexity. Basically all it means that if you are bridging + flooding then you would potentially save some number of a copies depending on the types of devices on each port.

[...]

Quoted 3 lines

Second option is what macvtap zero copy uses and it already does copy on clone too. So I hacked that to make it support tcp/udp used by sunrpc.

This does seem like it is the preferable solution.

I haven't read the patches yet, so maybe you already support this, but it would be good to allow the creator of an skb to declare that it doesn't want this behaviour, e.g. because it has some other mechanism for dealing with this copy out for the case where an skb gets held somewhere.

Ian.

Quoted 54 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

But I fear we're about to toss Ian into yet another rabbit hole. :-)

Let's try to converge on something quickly as I think integration of his work has been delayed enough as-is.

...

Quoted 7 lines

Quoted 2 lines

It's weekend here, I'll work on a patch like this
Sunday.

Thanks, I was starting to feel my nose twitching and my ears beginning to elongate ;-)

Ian.

Here's a first stub at a fix. Basically to be able to modify frags on the fly we must make sure the skb isn't cloned, so the moment someone clones the skb we need to trigger the frag copy logic. And this is exactly what happens with SKBTX_DEV_ZEROCOPY so it seems to make sense to reuse that logic.

The below patchset replaces patch 7
([PATCH 7/9] net: add skb_orphan_frags to copy aside frags with destructors) in Ian's patchset and needs to be applied there.

Compiled only but I'd like to hear what people think all the same because it does add a couple of branches on fast path. On the other hand this makes it generic so the same logic will be reusable for packet sockets (which IIRC are currently buggy in the same way as sunrpc) and for adding zero copy support to tun.

Please comment,
Thanks!

Signature · 16 lines

MST


Michael S. Tsirkin (6):
  skbuff: support per-page destructors in copy_ubufs
  skbuff: add an api to orphan frags
  skbuff: convert to skb_orphan_frags
  tun: orphan frags on xmit
  net: orphan frags on receive
  skbuff: set zerocopy flag on frag destructor

 drivers/net/tun.c      |    2 ++
 include/linux/skbuff.h |   41 +++++++++++++++++++++++++++++++++++++++++
 net/core/dev.c         |    2 ++
 net/core/skbuff.c      |   43 +++++++++++++++++++++++++------------------
 4 files changed, 70 insertions(+), 18 deletions(-)

Quoted 41 lines · Ian Campbell

Quoted 6 lines · Michael S. Tsirkin

Quoted 5 lines · Ian Campbell

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines · David Miller

From: "Michael S. Tsirkin" <[email protected]>
Date: Fri, 4 May 2012 00:10:24 +0300

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

Hmm we orphan skbs when we loop them back so how about reusing the skb->destructor for this?

That's one possibility.

So originally I thought it would work: destructor would wake the original owner which would do data copy and modify the fragments. But then I unfortunately realized that would be racy: the new owner could be

By "new owner" you mean the owner of some other skb which refernences the same shinfo (so either clone or the original if we currently hold a clone).

Quoted 2 lines

using the old frags and there appears no way for us to make sure it doesn't so that we can put the original page.

Right, the key issue is, I think, that cloning etc take an extra dataref on the shinfo but do not take references on the frags. This is obviously sane but does cause the issue above.

Quoted 3 lines

And the same logic applies to modifying the frags at any other time if the skb is cloned. So it seems we must copy if we want to clone the skb.

Right. I had been hoping that the frag destructors could avoid this but it seems like this is not going to be the case.

Just as a thought experiment would taking a frag ref on clone help us? (lets assume for now that, iff there are destructors, this is cheaper than the copy). I think this doesn't work -- since although the ref will mean that the page doesn't go away under anyone elses feet after we've orphaned it we will have lost the pointer to the original page by the time that other ref is dropped, so freeing it will be tricky.

Quoted 6 lines

So following that train of thought a step further -- would creating a new shinfo entirely on frag orphan buy us anything (and at what cost)? Obviously it an extra allocation -- but we are already allocating N pages of new frag. The others skb's referencing the shared shinfo would still continue to see the old shinfo, pages and refs until they themselves needed to orphan the frags (if they do, they might avoid it).

So it would address the second problem (cloned skb) but not the first one (original owner racing with the first one).

Quoted 19 lines

I think that could work, but I'm not sure it is worth the complexity. Basically all it means that if you are bridging + flooding then you would potentially save some number of a copies depending on the types of devices on each port.

[...]

Quoted 3 lines

Second option is what macvtap zero copy uses and it already does copy on clone too. So I hacked that to make it support tcp/udp used by sunrpc.

This does seem like it is the preferable solution.

I haven't read the patches yet, so maybe you already support this, but it would be good to allow the creator of an skb to declare that it doesn't want this behaviour, e.g. because it has some other mechanism for dealing with this copy out for the case where an skb gets held somewhere.

Quoted 1 line

Ian.

Sure, if we find a way to do that we just don't set ZEROCOPY skb flag.

So I'll give people a bit more time to review, hope you find the time too.

I took a look at a fairly high level and it all looked sensible, I've not looked closely at the details, not run it yet, I hope I can do that shortly.

Quoted 12 lines · Ian Campbell

Quoted 11 lines · Michael S. Tsirkin

Quoted 9 lines · Ian Campbell

Quoted 8 lines · Michael S. Tsirkin

Quoted 7 lines · David Miller

From: "Michael S. Tsirkin" <[email protected]>
Date: Fri, 4 May 2012 00:10:24 +0300

Quoted 2 lines

Hmm we orphan skbs when we loop them back so how about reusing the skb->destructor for this?

That's one possibility.

Quoted 7 lines

Quoted 3 lines

So originally I thought it would work: destructor would wake the original owner which would do data copy and modify the fragments. But then I unfortunately realized that would be racy: the new owner could be

By "new owner" you mean the owner of some other skb which refernences the same shinfo (so either clone or the original if we currently hold a clone).

Quoted 21 lines

Quoted 2 lines

using the old frags and there appears no way for us to make sure it doesn't so that we can put the original page.

Right, the key issue is, I think, that cloning etc take an extra dataref on the shinfo but do not take references on the frags. This is obviously sane but does cause the issue above.

Quoted 3 lines

And the same logic applies to modifying the frags at any other time if the skb is cloned. So it seems we must copy if we want to clone the skb.

Right. I had been hoping that the frag destructors could avoid this but it seems like this is not going to be the case.

Just as a thought experiment would taking a frag ref on clone help us? (lets assume for now that, iff there are destructors, this is cheaper than the copy). I think this doesn't work -- since although the ref will mean that the page doesn't go away under anyone elses feet after we've orphaned it we will have lost the pointer to the original page by the time that other ref is dropped, so freeing it will be tricky.

Quoted 6 lines

So following that train of thought a step further -- would creating a new shinfo entirely on frag orphan buy us anything (and at what cost)? Obviously it an extra allocation -- but we are already allocating N pages of new frag. The others skb's referencing the shared shinfo would still continue to see the old shinfo, pages and refs until they themselves needed to orphan the frags (if they do, they might avoid it).

So it would address the second problem (cloned skb) but not the first one (original owner racing with the first one).

Quoted 13 lines

I think that could work, but I'm not sure it is worth the complexity. Basically all it means that if you are bridging + flooding then you would potentially save some number of a copies depending on the types of devices on each port.

[...]

Quoted 3 lines

Second option is what macvtap zero copy uses and it already does copy on clone too. So I hacked that to make it support tcp/udp used by sunrpc.

This does seem like it is the preferable solution.

Quoted 7 lines

I haven't read the patches yet, so maybe you already support this, but it would be good to allow the creator of an skb to declare that it doesn't want this behaviour, e.g. because it has some other mechanism for dealing with this copy out for the case where an skb gets held somewhere.

Ian.

Sure, if we find a way to do that we just don't set ZEROCOPY skb flag.

So I'll give people a bit more time to review, hope you find the time too.

Quoted 55 lines

Quoted 54 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

But I fear we're about to toss Ian into yet another rabbit hole. :-)

Let's try to converge on something quickly as I think integration of his work has been delayed enough as-is.

...

Quoted 7 lines

Quoted 2 lines

It's weekend here, I'll work on a patch like this
Sunday.

Thanks, I was starting to feel my nose twitching and my ears beginning to elongate ;-)

Ian.

Here's a first stub at a fix. Basically to be able to modify frags on the fly we must make sure the skb isn't cloned, so the moment someone clones the skb we need to trigger the frag copy logic. And this is exactly what happens with SKBTX_DEV_ZEROCOPY so it seems to make sense to reuse that logic.

The below patchset replaces patch 7
([PATCH 7/9] net: add skb_orphan_frags to copy aside frags with destructors) in Ian's patchset and needs to be applied there.

Compiled only but I'd like to hear what people think all the same because it does add a couple of branches on fast path. On the other hand this makes it generic so the same logic will be reusable for packet sockets (which IIRC are currently buggy in the same way as sunrpc) and for adding zero copy support to tun.

Please comment,
Thanks!

Signature · 16 lines

MST


Michael S. Tsirkin (6):
  skbuff: support per-page destructors in copy_ubufs
  skbuff: add an api to orphan frags
  skbuff: convert to skb_orphan_frags
  tun: orphan frags on xmit
  net: orphan frags on receive
  skbuff: set zerocopy flag on frag destructor

 drivers/net/tun.c      |    2 ++
 include/linux/skbuff.h |   41 +++++++++++++++++++++++++++++++++++++++++
 net/core/dev.c         |    2 ++
 net/core/skbuff.c      |   43 +++++++++++++++++++++++++------------------
 4 files changed, 70 insertions(+), 18 deletions(-)