From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-07 13:53:43
On Fri, May 04, 2012 at 11:51:31AM +0100, Ian Campbell wrote:
On Fri, 2012-05-04 at 11:03 +0100, Michael S. Tsirkin wrote:
quoted
On Fri, May 04, 2012 at 02:54:33AM -0400, David Miller wrote:
quoted
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Fri, 4 May 2012 00:10:24 +0300
quoted
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.
Further, destructor itself can't do the copy because this needs
to allocate memory in atomic context, and destructor
itself can't fail.
For this second problem I see two solutions: either pre-allocate the
copy buffer just in case and track it together with the destructor, or
use an skb flag to make the check for destructors quick (if not
completely free).
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
quoted
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
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!
--
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(-)
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-07 13:54:28
sunrpc wants to use zero copy with tcp which means
some fragments are zero copy while others are not.
This in turn means there's no per skb destructor_arg,
instead some fragments have destructors. Teach
skb_copy_ubufs and skb_release_data to handle such skbs.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/core/skbuff.c | 18 ++++++++++++++----
1 files changed, 14 insertions(+), 4 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-07 13:54:34
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 <mst@redhat.com>
---
include/linux/skbuff.h | 24 ++++++++++++++++++++++++
1 files changed, 24 insertions(+), 0 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-07 13:54:39
Reduce dode duplication a bit using the new helper.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/core/skbuff.c | 22 ++++++++--------------
1 files changed, 8 insertions(+), 14 deletions(-)
@@ -1005,10 +1001,8 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,skb_free_head(skb);}else{/* copy this zero copy skb frags */-if(skb_shinfo(skb)->tx_flags&SKBTX_DEV_ZEROCOPY){-if(skb_copy_ubufs(skb,gfp_mask))-gotonofrags;-}+if(skb_orphan_frags(skb,gfp_mask))+gotonofrags;for(i=0;i<skb_shinfo(skb)->nr_frags;i++)skb_frag_ref(skb,i);
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-07 13:54:45
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 <mst@redhat.com>
---
drivers/net/tun.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
@@ -414,6 +414,8 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)/* Orphan the skb - required as we might hang on to it*forindefinitetime.*/+if(unlikely(skb_orphan_frags(skb,GFP_ATOMIC)))+gotodrop;skb_orphan(skb);/* Enqueue packet */
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-07 13:54:49
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 <mst@redhat.com>
---
net/core/dev.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-07 13:54:55
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 <mst@redhat.com>
---
include/linux/skbuff.h | 19 ++++++++++++++++++-
net/core/skbuff.c | 3 +++
2 files changed, 21 insertions(+), 1 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-08 09:41:16
On Mon, May 07, 2012 at 04:53:46PM +0300, Michael S. Tsirkin wrote:
quoted
quoted
quoted
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!
Ian, just to stress, need your ack that this does what you wanted.
--
MST
From: Ian Campbell <hidden> Date: 2012-05-08 10:54:13
On Tue, 2012-05-08 at 10:41 +0100, Michael S. Tsirkin wrote:
On Mon, May 07, 2012 at 04:53:46PM +0300, Michael S. Tsirkin wrote:
quoted
quoted
quoted
quoted
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
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!
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.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-08 12:46:29
On Mon, May 07, 2012 at 04:54:57PM +0300, Michael S. Tsirkin wrote:
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 <mst@redhat.com>
Ugh. I somehow managed to mangle this last patch and send out
a version that defines skb_copy_frag_destructor twice.
The following is needed on top (not resubmitting everything as it's an
RFC, and needs to be part of Ian's patchset anyway. Sorry about the
noise.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-08 12:47:22
On Mon, May 07, 2012 at 04:54:36PM +0300, Michael S. Tsirkin wrote:
quoted hunk
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 <mst@redhat.com>
---
include/linux/skbuff.h | 24 ++++++++++++++++++++++++
1 files changed, 24 insertions(+), 0 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-08 14:36:27
On Mon, May 07, 2012 at 04:54:46PM +0300, Michael S. Tsirkin wrote:
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 <mst@redhat.com>
If tun is built as a module, skb_copy_ubufs needs to be exported:
---->
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-08 14:49:06
On Tue, May 08, 2012 at 11:54:11AM +0100, Ian Campbell wrote:
On Tue, 2012-05-08 at 10:41 +0100, Michael S. Tsirkin wrote:
quoted
On Mon, May 07, 2012 at 04:53:46PM +0300, Michael S. Tsirkin wrote:
quoted
quoted
quoted
quoted
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
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!
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.
In case you or anyone else has cycles to test this, here's the
updated tree, net-next based - so far I verified that it builds and boots
and regular networking works, but didn't test nfs yet.
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git refs/heads/zerocopy
From: Ian Campbell <hidden> Date: 2012-05-09 13:19:06
On Mon, 2012-05-07 at 14:53 +0100, Michael S. Tsirkin wrote:
On Fri, May 04, 2012 at 11:51:31AM +0100, Ian Campbell wrote:
quoted
On Fri, 2012-05-04 at 11:03 +0100, Michael S. Tsirkin wrote:
quoted
On Fri, May 04, 2012 at 02:54:33AM -0400, David Miller wrote:
quoted
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Fri, 4 May 2012 00:10:24 +0300
quoted
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).
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.
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.
[...]
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
quoted
quoted
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
quoted
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!
--
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(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-09 13:52:40
On Wed, May 09, 2012 at 02:18:49PM +0100, Ian Campbell wrote:
On Mon, 2012-05-07 at 14:53 +0100, Michael S. Tsirkin wrote:
quoted
On Fri, May 04, 2012 at 11:51:31AM +0100, Ian Campbell wrote:
quoted
On Fri, 2012-05-04 at 11:03 +0100, Michael S. Tsirkin wrote:
quoted
On Fri, May 04, 2012 at 02:54:33AM -0400, David Miller wrote:
quoted
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Fri, 4 May 2012 00:10:24 +0300
quoted
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
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
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).
So it would address the second problem (cloned skb) but
not the first one (original owner racing with the first one).
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
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.
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
quoted
quoted
quoted
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
quoted
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!
--
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(-)
From: Ian Campbell <hidden> Date: 2012-05-09 13:54:54
On Mon, 2012-05-07 at 14:54 +0100, Michael S. Tsirkin wrote:
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 hunk
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/core/dev.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
From: Ian Campbell <hidden> Date: 2012-05-09 14:01:50
On Wed, 2012-05-09 at 14:52 +0100, Michael S. Tsirkin wrote:
On Wed, May 09, 2012 at 02:18:49PM +0100, Ian Campbell wrote:
quoted
On Mon, 2012-05-07 at 14:53 +0100, Michael S. Tsirkin wrote:
quoted
On Fri, May 04, 2012 at 11:51:31AM +0100, Ian Campbell wrote:
quoted
On Fri, 2012-05-04 at 11:03 +0100, Michael S. Tsirkin wrote:
quoted
On Fri, May 04, 2012 at 02:54:33AM -0400, David Miller wrote:
quoted
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Fri, 4 May 2012 00:10:24 +0300
quoted
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
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
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).
So it would address the second problem (cloned skb) but
not the first one (original owner racing with the first one).
Right.
quoted
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
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.
Sure, if we find a way to do that we just don't set ZEROCOPY skb flag.
After reading "net: orphan frags on receive" I'm not sure how necessary
it will be, since the main case I was worried about was lots of
copying/orphaning on bridge forwarding, but I convinced myself (I think)
that the series only effects delivery and not forwarding.
Still, no harm in offering the option.
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.
BTW, I never said thanks for looking into this, so thanks ;-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-09 14:56:18
On Wed, May 09, 2012 at 04:52:40PM +0300, Michael S. Tsirkin wrote:
On Wed, May 09, 2012 at 02:18:49PM +0100, Ian Campbell wrote:
quoted
On Mon, 2012-05-07 at 14:53 +0100, Michael S. Tsirkin wrote:
quoted
On Fri, May 04, 2012 at 11:51:31AM +0100, Ian Campbell wrote:
quoted
On Fri, 2012-05-04 at 11:03 +0100, Michael S. Tsirkin wrote:
quoted
On Fri, May 04, 2012 at 02:54:33AM -0400, David Miller wrote:
quoted
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Fri, 4 May 2012 00:10:24 +0300
quoted
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).
Sorry didn't clarify this well enough. We often do something like
the following
orphan
set owner
new owner is what is set afterwards.
quoted
quoted
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
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).
So it would address the second problem (cloned skb) but
not the first one (original owner racing with the first one).
Oh and there's another issue: some code assumes that clones
are always identical. For example tcpdump
might show wrong packet content if the page is modified
while packet gets through networking core, so a malicious
application might confuse anything that relies on
this data to do e.g. security filtering.
I don't know whether anyone cares about tcpdump output
being always correct but it seems nicer to side-step the issue.
quoted
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
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.
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
quoted
quoted
quoted
quoted
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
quoted
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!
--
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(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-09 15:12:01
On Wed, May 09, 2012 at 02:54:52PM +0100, Ian Campbell wrote:
On Mon, 2012-05-07 at 14:54 +0100, Michael S. Tsirkin wrote:
quoted
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.
I think so - bridge uses netdev_rx_handler_unregister
so it does not go through packet handlers.
quoted
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/core/dev.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-09 15:27:09
On Wed, May 09, 2012 at 03:01:47PM +0100, Ian Campbell wrote:
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.
In particular you can trace skb_copy_ubufs to verify that
it's called in the scenario where we want a copybreak
and not called where unexpected e.g. in bridge forwarding.
From: Ian Campbell <hidden> Date: 2012-05-10 17:46:35
On Mon, 2012-05-07 at 14:54 +0100, Michael S. Tsirkin wrote:
/* 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.
+ 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;
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-10 18:42:47
On Thu, May 10, 2012 at 06:46:17PM +0100, Ian Campbell wrote:
On Mon, 2012-05-07 at 14:54 +0100, Michael S. Tsirkin wrote:
quoted
/* 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]
Good catch.
for (i = skb_shinfo(skb)->nr_frags - 1; i >= 0; i--) {
skb_frag_t *f = &skb_shinfo(skb)->frags[i];
would be a bit clearer though.
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.
Agree, let's just clear it.
quoted
+ 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;
So the below on top then. I pushed these on
top of my zerocopy branch - can you confirm pls?
---
From: Ian Campbell <hidden> Date: 2012-05-11 09:00:04
On Thu, 2012-05-10 at 19:42 +0100, Michael S. Tsirkin wrote:
On Thu, May 10, 2012 at 06:46:17PM +0100, Ian Campbell wrote:
quoted
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?
I added these to my test branch:
93772fea6cd66616912101b9e0144dfed645d8fe fix per page destructors in copy ubufs
d72b7ab15f944c5df5f28cce7b5c9a0bca61ff6d clear destructor arg when set zerocopy
I think you also need, as part of the second one:
From: Ian Campbell <hidden> Date: 2012-05-11 10:58:21
On Fri, 2012-05-11 at 10:00 +0100, Ian Campbell wrote:
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)
I suspect this means that the existing SKBTX_DEV_ZEROCOPY semantics are
a superset of what we need to consider for the destructor case. I'm
assuming here that the existing SKBTX_DEV_ZEROCOPY is copying aside
exactly the right amount and isn't conservatively coying more often than
necessary.
shinfo->tx_flags are pretty scarce -- can we afford a new one for this
usecase?
Or perhaps this is actually a function of the callsite not the of
individual skb and we want to have some concept of "deep" and "shallow"
clones combined with SKBTX_DEV_ZEROCOPY to decide when to copy_ubufs or
not? e.g. deep clone => always copy if SKBTX_DEV_ZEROCOPY and shallow
clone => only copy if SKBTX_DEV_ZEROCOPY && destructor_arg!=NULL
(neither copy if !SKBTX_DEV_ZEROCOPY).
Oh, I suppose that reintroduces the copy_ubufs under a (shallow) cloned
skb race if one of those skbs eventually finds itself in a situation
where a skb_frag_orphan is required doesn't it. Hrm :-/
Will have to have a think...
Ian.
[ 109.680828] ------------[ cut here ]------------
[ 109.685440] WARNING: at /local/scratch/ianc/devel/kernels/linux/include/linux/skbuff.h:1732 skb_clone+0xe6/0xf0()
[ 109.695678] Hardware name:
[ 109.699162] ORPHANING
[ 109.701434] Modules linked in:
[ 109.704495] Pid: 10, comm: kworker/0:1 Tainted: G W 3.4.0-rc4-x86_64-native+ #186
[ 109.712830] Call Trace:
[ 109.715278] [<ffffffff8107edfa>] warn_slowpath_common+0x7a/0xb0
[ 109.721273] [<ffffffff8107eed1>] warn_slowpath_fmt+0x41/0x50
[ 109.727007] [<ffffffff8170feea>] ? tcp_transmit_skb+0x9a/0x8f0
[ 109.732914] [<ffffffff8169b2d6>] skb_clone+0xe6/0xf0
[ 109.737957] [<ffffffff8170feea>] tcp_transmit_skb+0x9a/0x8f0
[ 109.743694] [<ffffffff81712d7a>] tcp_write_xmit+0x1ea/0x9c0
[ 109.749343] [<ffffffff8171357b>] tcp_push_one+0x2b/0x40
[ 109.754648] [<ffffffff81705b2b>] tcp_sendpage+0x64b/0x6d0
[ 109.760126] [<ffffffff8172785d>] inet_sendpage+0x4d/0xf0
[ 109.765518] [<ffffffff817afed7>] xs_sendpages+0x117/0x2a0
[ 109.770996] [<ffffffff817ad3f0>] ? xprt_reserve+0x2d0/0x2d0
[ 109.776647] [<ffffffff817b0178>] xs_tcp_send_request+0x58/0x110
[ 109.782644] [<ffffffff817ad5bb>] xprt_transmit+0x6b/0x2d0
[ 109.788123] [<ffffffff817aa9a0>] ? call_transmit_status+0xd0/0xd0
[ 109.794293] [<ffffffff817aab70>] call_transmit+0x1d0/0x290
[ 109.799857] [<ffffffff817aa9a0>] ? call_transmit_status+0xd0/0xd0
[ 109.806029] [<ffffffff817b3725>] __rpc_execute+0x65/0x260
[ 109.811505] [<ffffffff817b3920>] ? __rpc_execute+0x260/0x260
[ 109.817241] [<ffffffff817b3930>] rpc_async_schedule+0x10/0x20
[ 109.823066] [<ffffffff81098fff>] process_one_work+0x11f/0x460
[ 109.828895] [<ffffffff8109b0b3>] worker_thread+0x173/0x3f0
[ 109.834459] [<ffffffff8109af40>] ? manage_workers+0x210/0x210
[ 109.840283] [<ffffffff8109fa26>] kthread+0x96/0xa0
[ 109.845179] [<ffffffff81861654>] kernel_thread_helper+0x4/0x10
[ 109.851092] [<ffffffff8109f990>] ? kthread_freezable_should_stop+0x70/0x70
[ 109.858053] [<ffffffff81861650>] ? gs_change+0xb/0xb
[ 109.863087] ---[ end trace 3e3acdb7cc57c191 ]---
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-11 12:08:38
On Fri, May 11, 2012 at 11:58:12AM +0100, Ian Campbell wrote:
On Fri, 2012-05-11 at 10:00 +0100, Ian Campbell wrote:
quoted
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)
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?
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-11 18:30:30
On Fri, May 11, 2012 at 03:08:36PM +0300, Michael S. Tsirkin wrote:
On Fri, May 11, 2012 at 11:58:12AM +0100, Ian Campbell wrote:
quoted
On Fri, 2012-05-11 at 10:00 +0100, Ian Campbell wrote:
quoted
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)
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?
From: David Miller <davem@davemloft.net> Date: 2012-05-11 21:12:09
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Fri, 11 May 2012 15:08:37 +0300
On Fri, May 11, 2012 at 11:58:12AM +0100, Ian Campbell wrote:
quoted
On Fri, 2012-05-11 at 10:00 +0100, Ian Campbell wrote:
quoted
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)
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?
We can't send the original SKB that's linked into the retransmit
queue. It's linkage must stay secure in that queue.
From: Ian Campbell <hidden> Date: 2012-05-12 06:01:26
On Fri, 2012-05-11 at 17:30 +0100, Michael S. Tsirkin wrote:
On Fri, May 11, 2012 at 03:08:36PM +0300, Michael S. Tsirkin wrote:
quoted
On Fri, May 11, 2012 at 11:58:12AM +0100, Ian Campbell wrote:
quoted
On Fri, 2012-05-11 at 10:00 +0100, Ian Campbell wrote:
quoted
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)
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.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-13 10:10:29
On Sat, May 12, 2012 at 07:01:24AM +0100, Ian Campbell wrote:
On Fri, 2012-05-11 at 17:30 +0100, Michael S. Tsirkin wrote:
quoted
On Fri, May 11, 2012 at 03:08:36PM +0300, Michael S. Tsirkin wrote:
quoted
On Fri, May 11, 2012 at 11:58:12AM +0100, Ian Campbell wrote:
quoted
On Fri, 2012-05-11 at 10:00 +0100, Ian Campbell wrote:
quoted
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)
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.
Yes I think we should call a variant of clone that avoids copy_ubuf on
the first transmit. But need to be careful we don't access the frag
list while it is being modified.
For example very roughly, maybe we could have copy_ubuf detect
packet clone is queued and take some lock?
On retransmit we could check and if we are not the only clone left
(which should be uncommon) trigger copy ubuf then.
Thoughts?
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2012-05-14 19:18:44
On Wed, May 09, 2012 at 02:54:52PM +0100, Ian Campbell wrote:
On Mon, 2012-05-07 at 14:54 +0100, Michael S. Tsirkin wrote:
quoted
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.
Eric, does this patch look ok to you?
It's key to both my and ian's zerocopy work.
--
MST