[PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Shirley Ma]

Signed-off-by: Shirley Ma <xma@us.ibm.com>
---

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

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a134d80..2646251 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1066,6 +1066,16 @@ struct net_device {
 #define NETIF_F_NOCACHE_COPY	(1 << 30) /* Use no-cache copyfromuser */
 #define NETIF_F_LOOPBACK	(1 << 31) /* Enable loopback */
 
+/*
+ * Bit 31 is for device to map userspace buffers -- zerocopy
+ * Device can set this flag when it supports HIGHDMA.
+ * Device can't recycle this kind of skb buffers.
+ * There are 256 bytes copied, the rest of buffers are mapped.
+ * The userspace callback should only be called when last reference to this skb
+ * is gone.
+ */
+#define NETIF_F_ZEROCOPY	(1 << 31)
+
 	/* Segmentation offload features */
 #define NETIF_F_GSO_SHIFT	16
 #define NETIF_F_GSO_MASK	0x00ff0000

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Ben Hutchings]

On Mon, 2011-05-16 at 12:28 -0700, Shirley Ma wrote:
> Signed-off-by: Shirley Ma <xma@us.ibm.com>
> ---
> 
>  include/linux/netdevice.h |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index a134d80..2646251 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1066,6 +1066,16 @@ struct net_device {
>  #define NETIF_F_NOCACHE_COPY	(1 << 30) /* Use no-cache copyfromuser */
>  #define NETIF_F_LOOPBACK	(1 << 31) /* Enable loopback */
>  
> +/*
> + * Bit 31 is for device to map userspace buffers -- zerocopy
> + * Device can set this flag when it supports HIGHDMA.
> + * Device can't recycle this kind of skb buffers.
> + * There are 256 bytes copied, the rest of buffers are mapped.
> + * The userspace callback should only be called when last reference to this skb
> + * is gone.
> + */
> +#define NETIF_F_ZEROCOPY	(1 << 31)

Sorry, bit 31 is taken.  You get the job of turning features into a
wider bitmap.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Shirley Ma]

On Mon, 2011-05-16 at 20:35 +0100, Ben Hutchings wrote:
> Sorry, bit 31 is taken.  You get the job of turning features into a
> wider bitmap.

:) will do it.

Thanks
Shirley

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Ben Hutchings]

On Mon, 2011-05-16 at 12:38 -0700, Shirley Ma wrote:
> On Mon, 2011-05-16 at 20:35 +0100, Ben Hutchings wrote:
> > Sorry, bit 31 is taken.  You get the job of turning features into a
> > wider bitmap.
> 
> :) will do it.

Bear in mind that feature masks are manipulated in many different
places.  This is not a simple task.

See previous discussion at:
http://thread.gmane.org/gmane.linux.network/193284
and especially:
http://thread.gmane.org/gmane.linux.network/193284/focus=193332

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michael S. Tsirkin]

On Mon, May 16, 2011 at 08:47:33PM +0100, Ben Hutchings wrote:
> On Mon, 2011-05-16 at 12:38 -0700, Shirley Ma wrote:
> > On Mon, 2011-05-16 at 20:35 +0100, Ben Hutchings wrote:
> > > Sorry, bit 31 is taken.  You get the job of turning features into a
> > > wider bitmap.
> > 
> > :) will do it.
> 
> Bear in mind that feature masks are manipulated in many different
> places.  This is not a simple task.
> 
> See previous discussion at:
> http://thread.gmane.org/gmane.linux.network/193284
> and especially:
> http://thread.gmane.org/gmane.linux.network/193284/focus=193332
> 
> Ben.


IIUC, what is suggested above is something like:

typedef struct net_features {
} net_features_t;

and then

   void netdev_set_feature(net_features_t *net_features, int feature);
   void netdev_clear_feature(net_features_t *net_features, int feature);
   bool netdev_test_feature(net_features_t *net_features, int feature);


I think this might be the easiest way as compiler will catch any direct uses.
It can then be split up nicely.

It looks a bit different from what Dave suggested but I think it's
close enough?

we could also have wrappers that set/clear/test many features to replace
uses of A|B|C that are pretty common.

   static inline void netdev_set_features(net_features_t *net_features, int nfeatures, int *features)
   {
	int i;
	for (i = 0; i < nfeatures; ++i)
		netdev_set_feature(net_features, features[i]);
   }
   void netdev_clear_features(net_features_t *net_features, int nfeatures, int *features)
   {
	int i;
	for (i = 0; i < nfeatures; ++i)
		netdev_clear_feature(net_features, features[i]);
   }
   bool netdev_test_features(net_features_t *net_features, int nfeatures, int *features)
   {
	int i;
	for (i = 0; i < nfeatures; ++i)
		if (netdev_test_feature(net_features, features[i]))
			return true;
	return false;
   }

and possibly macros that get arrays of constants:

#define NETDEV_SET_FEATURES(net_features, feature_array) do { \
	int __NETDEV_SET_FEATURES_F[] = feature_array;
	netdev_set_feature((net_features), \
		ARRAY_SIZE(__NETDEV_SET_FEATURES_F), __NETDEV_SET_FEATURES_F);
} while (0)

etc.

> -- 
> Ben Hutchings, Senior Software Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Shirley Ma]

Hello Michael,

Looks like to use a new flag requires more time/work. I am thinking
whether we can just use HIGHDMA flag to enable zero-copy in macvtap to
avoid the new flag for now since mavctap uses real NICs as lower device?

Thanks
Shirley

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michael S. Tsirkin]

On Mon, May 16, 2011 at 04:32:18PM -0700, Shirley Ma wrote:
> Hello Michael,
> 
> Looks like to use a new flag requires more time/work.
> I am thinking
> whether we can just use HIGHDMA flag to enable zero-copy in macvtap to
> avoid the new flag for now since mavctap uses real NICs as lower device?
> 
> Thanks
> Shirley

Problem is, in your patch there are a set of restrictions on what the
device can do with the skb that we need to enforce somehow.
Also, how do we know it's a 'real NIC' and not a software device?

-- 
MST

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Shirley Ma]

On Tue, 2011-05-17 at 09:21 +0300, Michael S. Tsirkin wrote:
> Problem is, in your patch there are a set of restrictions on what the
> device can do with the skb that we need to enforce somehow.
> Also, how do we know it's a 'real NIC' and not a software device?

I checked macvtap newlink, it doesn't seem to block software device.

So we need to work on the wider feature bit first.

Shirley

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michał Mirosław]

2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
> Hello Michael,
>
> Looks like to use a new flag requires more time/work. I am thinking
> whether we can just use HIGHDMA flag to enable zero-copy in macvtap to
> avoid the new flag for now since mavctap uses real NICs as lower device?

Is there any other restriction besides requiring driver to not recycle
the skb? Are there any drivers that recycle TX skbs?

Best Regards,
Michał Mirosław

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Shirley Ma]

On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
> 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
> > Hello Michael,
> >
> > Looks like to use a new flag requires more time/work. I am thinking
> > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
> to
> > avoid the new flag for now since mavctap uses real NICs as lower
> device?
> 
> Is there any other restriction besides requiring driver to not recycle
> the skb? Are there any drivers that recycle TX skbs? 

Not more other restrictions, skb clone is OK. pskb_expand_head() looks
OK to me from code review.

Currently there is no drivers recycle TX skbs.

Thanks
Shirley

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michał Mirosław]

W dniu 18 maja 2011 00:28 użytkownik Shirley Ma <mashirle@us.ibm.com> napisał:
> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
>> 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
>> > Looks like to use a new flag requires more time/work. I am thinking
>> > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
>> to
>> > avoid the new flag for now since mavctap uses real NICs as lower
>> device?
>>
>> Is there any other restriction besides requiring driver to not recycle
>> the skb? Are there any drivers that recycle TX skbs?
> Not more other restrictions, skb clone is OK. pskb_expand_head() looks
> OK to me from code review.

> Currently there is no drivers recycle TX skbs.

So why do you require the target device to have some flags at all?

Do I understand correctly, that this zero-copy feature is about
packets received from VMs?

Best Regards,
Michał Mirosław

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Shirley Ma]

On Wed, 2011-05-18 at 00:58 +0200, Michał Mirosław wrote:
> W dniu 18 maja 2011 00:28 użytkownik Shirley Ma <mashirle@us.ibm.com>
> napisał:
> > On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
> >> 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
> >> > Looks like to use a new flag requires more time/work. I am
> thinking
> >> > whether we can just use HIGHDMA flag to enable zero-copy in
> macvtap
> >> to
> >> > avoid the new flag for now since mavctap uses real NICs as lower
> >> device?
> >>
> >> Is there any other restriction besides requiring driver to not
> recycle
> >> the skb? Are there any drivers that recycle TX skbs?
> > Not more other restrictions, skb clone is OK. pskb_expand_head()
> looks
> > OK to me from code review.
> 
> > Currently there is no drivers recycle TX skbs.
> 
> So why do you require the target device to have some flags at all?

We could use macvtap to check lower device HIGHDMA to enable zero-copy,
but I am not sure whether it is sufficient. If it's sufficient then we
don't need to use a new flag here. To be safe, it's better to use a new
flag to enable each device who can pass zero-copy test.

> Do I understand correctly, that this zero-copy feature is about
> packets received from VMs?

Yes, packets sent from VMs, and received in local host for TX zero-copy
here.

Thanks
Shirley

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michał Mirosław]

W dniu 18 maja 2011 01:44 użytkownik Shirley Ma <mashirle@us.ibm.com> napisał:
> On Wed, 2011-05-18 at 00:58 +0200, Michał Mirosław wrote:
>> W dniu 18 maja 2011 00:28 użytkownik Shirley Ma <mashirle@us.ibm.com>
>> napisał:
>> > On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
>> >> 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
>> >> > Looks like to use a new flag requires more time/work. I am
>> thinking
>> >> > whether we can just use HIGHDMA flag to enable zero-copy in
>> macvtap
>> >> to
>> >> > avoid the new flag for now since mavctap uses real NICs as lower
>> >> device?
>> >>
>> >> Is there any other restriction besides requiring driver to not
>> recycle
>> >> the skb? Are there any drivers that recycle TX skbs?
>> > Not more other restrictions, skb clone is OK. pskb_expand_head()
>> looks
>> > OK to me from code review.
>>
>> > Currently there is no drivers recycle TX skbs.
>>
>> So why do you require the target device to have some flags at all?
> We could use macvtap to check lower device HIGHDMA to enable zero-copy,
> but I am not sure whether it is sufficient. If it's sufficient then we
> don't need to use a new flag here. To be safe, it's better to use a new
> flag to enable each device who can pass zero-copy test.

>> Do I understand correctly, that this zero-copy feature is about
>> packets received from VMs?
> Yes, packets sent from VMs, and received in local host for TX zero-copy
> here.

What is the zero-copy test? On some arches the HIGHDMA is not needed
at all so might be not enabled on anything. It looks like the correct
test would be per-packet check of !illegal_highdma() or maybe
NETIF_F_SG as returned from harmonize_features(). For virtual devices
or other software forwarding this might lead to skb_linearize() in
some cases, but is it that bad?

Best Regards,
Michał Mirosław

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michael S. Tsirkin]

On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
> > > Hello Michael,
> > >
> > > Looks like to use a new flag requires more time/work. I am thinking
> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
> > to
> > > avoid the new flag for now since mavctap uses real NICs as lower
> > device?
> > 
> > Is there any other restriction besides requiring driver to not recycle
> > the skb? Are there any drivers that recycle TX skbs? 

Not just recycling skbs, keeping reference to any of the pages in the
skb. Another requirement is to invoke the callback
in a timely fashion.  For example virtio-net doesn't limit the time until
that happens (skbs are only freed when some other packet is
transmitted), so we need to avoid zcopy for such (nested-virt)
scenarious, right?
> 
> Not more other restrictions, skb clone is OK. pskb_expand_head() looks
> OK to me from code review.

Hmm. pskb_expand_head calls skb_release_data while keeping
references to pages. How is that ok? What do I miss?

> Currently there is no drivers recycle TX skbs.
> 
> Thanks
> Shirley

Well, with e.g. bridge or veth the skb might enter
the host networking stack. Once that happens, we lose
track of the pages. Or is there anything that
prevents such setups?

-- 
MST

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michał Mirosław]

2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
> On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
>> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
>> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
>> > > Hello Michael,
>> > >
>> > > Looks like to use a new flag requires more time/work. I am thinking
>> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
>> > to
>> > > avoid the new flag for now since mavctap uses real NICs as lower
>> > device?
>> >
>> > Is there any other restriction besides requiring driver to not recycle
>> > the skb? Are there any drivers that recycle TX skbs?
>
> Not just recycling skbs, keeping reference to any of the pages in the
> skb. Another requirement is to invoke the callback
> in a timely fashion.  For example virtio-net doesn't limit the time until
> that happens (skbs are only freed when some other packet is
> transmitted), so we need to avoid zcopy for such (nested-virt)
> scenarious, right?

Hmm. But every hardware driver supporting SG will keep reference to
the pages until the packet is sent (or DMA'd to the device). This can
take a long time if hardware queue happens to stall for some reason.

Is it that you mean keeping a reference after all skbs pointing to the
pages are released?

>> Not more other restrictions, skb clone is OK. pskb_expand_head() looks
>> OK to me from code review.
> Hmm. pskb_expand_head calls skb_release_data while keeping
> references to pages. How is that ok? What do I miss?

It's making copy of the skb_shinfo earlier, so the pages refcount
stays the same.

Best Regards,
Michał Mirosław

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michael S. Tsirkin]

On Wed, May 18, 2011 at 01:10:50PM +0200, Michał Mirosław wrote:
> 2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
> > On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
> >> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
> >> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
> >> > > Hello Michael,
> >> > >
> >> > > Looks like to use a new flag requires more time/work. I am thinking
> >> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
> >> > to
> >> > > avoid the new flag for now since mavctap uses real NICs as lower
> >> > device?
> >> >
> >> > Is there any other restriction besides requiring driver to not recycle
> >> > the skb? Are there any drivers that recycle TX skbs?
> >
> > Not just recycling skbs, keeping reference to any of the pages in the
> > skb. Another requirement is to invoke the callback
> > in a timely fashion.  For example virtio-net doesn't limit the time until
> > that happens (skbs are only freed when some other packet is
> > transmitted), so we need to avoid zcopy for such (nested-virt)
> > scenarious, right?
> 
> Hmm. But every hardware driver supporting SG will keep reference to
> the pages until the packet is sent (or DMA'd to the device). This can
> take a long time if hardware queue happens to stall for some reason.

That's a fundamental property of zero copy transmit.
You can't let the application/guest reuse the memory until
no one looks at it anymore.

> Is it that you mean keeping a reference after all skbs pointing to the
> pages are released?

No one should reference the pages after the callback is invoked, yes.

> >> Not more other restrictions, skb clone is OK. pskb_expand_head() looks
> >> OK to me from code review.
> > Hmm. pskb_expand_head calls skb_release_data while keeping
> > references to pages. How is that ok? What do I miss?
> 
> It's making copy of the skb_shinfo earlier, so the pages refcount
> stays the same.
> 
> Best Regards,
> Michał Mirosław

Exactly. But the callback is invoked so the guest thinks it's ok to
change this memory. If it does a corrupted packet will be sent out.

-- 
MST

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michał Mirosław]

W dniu 18 maja 2011 13:17 użytkownik Michael S. Tsirkin
<mst@redhat.com> napisał:
> On Wed, May 18, 2011 at 01:10:50PM +0200, Michał Mirosław wrote:
>> 2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
>> > On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
>> >> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
>> >> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
>> >> > > Hello Michael,
>> >> > >
>> >> > > Looks like to use a new flag requires more time/work. I am thinking
>> >> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
>> >> > to
>> >> > > avoid the new flag for now since mavctap uses real NICs as lower
>> >> > device?
>> >> >
>> >> > Is there any other restriction besides requiring driver to not recycle
>> >> > the skb? Are there any drivers that recycle TX skbs?
>> >
>> > Not just recycling skbs, keeping reference to any of the pages in the
>> > skb. Another requirement is to invoke the callback
>> > in a timely fashion.  For example virtio-net doesn't limit the time until
>> > that happens (skbs are only freed when some other packet is
>> > transmitted), so we need to avoid zcopy for such (nested-virt)
>> > scenarious, right?
>>
>> Hmm. But every hardware driver supporting SG will keep reference to
>> the pages until the packet is sent (or DMA'd to the device). This can
>> take a long time if hardware queue happens to stall for some reason.
>
> That's a fundamental property of zero copy transmit.
> You can't let the application/guest reuse the memory until
> no one looks at it anymore.
>
>> Is it that you mean keeping a reference after all skbs pointing to the
>> pages are released?
> No one should reference the pages after the callback is invoked, yes.

>> >> Not more other restrictions, skb clone is OK. pskb_expand_head() looks
>> >> OK to me from code review.
>> > Hmm. pskb_expand_head calls skb_release_data while keeping
>> > references to pages. How is that ok? What do I miss?
>> It's making copy of the skb_shinfo earlier, so the pages refcount
>> stays the same.
> Exactly. But the callback is invoked so the guest thinks it's ok to
> change this memory. If it does a corrupted packet will be sent out.

Hmm. I tool a quick look at skb_clone(), and it looks like this
sequence will break this scheme:

skb2 = skb_clone(skb...);
kfree_skb(skb) or pskb_expand_head(skb);  /* callback called */
[use skb2, pages still referenced]
kfree_skb(skb); /* callback called again */

This sequence is common in bridge, might be in other places.

Maybe this ubuf thing should just track clones? This will make it work
on all devices then.

Best Regards,
Michał Mirosław

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michael S. Tsirkin]

On Wed, May 18, 2011 at 01:40:29PM +0200, Michał Mirosław wrote:
> W dniu 18 maja 2011 13:17 użytkownik Michael S. Tsirkin
> <mst@redhat.com> napisał:
> > On Wed, May 18, 2011 at 01:10:50PM +0200, Michał Mirosław wrote:
> >> 2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
> >> > On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
> >> >> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
> >> >> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
> >> >> > > Hello Michael,
> >> >> > >
> >> >> > > Looks like to use a new flag requires more time/work. I am thinking
> >> >> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
> >> >> > to
> >> >> > > avoid the new flag for now since mavctap uses real NICs as lower
> >> >> > device?
> >> >> >
> >> >> > Is there any other restriction besides requiring driver to not recycle
> >> >> > the skb? Are there any drivers that recycle TX skbs?
> >> >
> >> > Not just recycling skbs, keeping reference to any of the pages in the
> >> > skb. Another requirement is to invoke the callback
> >> > in a timely fashion.  For example virtio-net doesn't limit the time until
> >> > that happens (skbs are only freed when some other packet is
> >> > transmitted), so we need to avoid zcopy for such (nested-virt)
> >> > scenarious, right?
> >>
> >> Hmm. But every hardware driver supporting SG will keep reference to
> >> the pages until the packet is sent (or DMA'd to the device). This can
> >> take a long time if hardware queue happens to stall for some reason.
> >
> > That's a fundamental property of zero copy transmit.
> > You can't let the application/guest reuse the memory until
> > no one looks at it anymore.
> >
> >> Is it that you mean keeping a reference after all skbs pointing to the
> >> pages are released?
> > No one should reference the pages after the callback is invoked, yes.
> 
> >> >> Not more other restrictions, skb clone is OK. pskb_expand_head() looks
> >> >> OK to me from code review.
> >> > Hmm. pskb_expand_head calls skb_release_data while keeping
> >> > references to pages. How is that ok? What do I miss?
> >> It's making copy of the skb_shinfo earlier, so the pages refcount
> >> stays the same.
> > Exactly. But the callback is invoked so the guest thinks it's ok to
> > change this memory. If it does a corrupted packet will be sent out.
> 
> Hmm. I tool a quick look at skb_clone(), and it looks like this
> sequence will break this scheme:
> 
> skb2 = skb_clone(skb...);
> kfree_skb(skb) or pskb_expand_head(skb);  /* callback called */
> [use skb2, pages still referenced]
> kfree_skb(skb); /* callback called again */
> 
> This sequence is common in bridge, might be in other places.
> 
> Maybe this ubuf thing should just track clones? This will make it work
> on all devices then.
> 
> Best Regards,
> Michał Mirosław

Long term that's a good plan, but it's a lot of work.
pages can also get into weird places like VFS
or devices might hang on to them for a long time.

So I think as a first step, using a flag to white-list
simple devices that don't do any tricks like the
above makes sense.

Just be sure to list all of the restrictions in
the comment where the flag is described.

And hey, we get features extended to 64 bit as a bonus :)

-- 
MST

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Shirley Ma]

On Wed, 2011-05-18 at 13:40 +0200, Michał Mirosław wrote:
> >> >> Not more other restrictions, skb clone is OK. pskb_expand_head()
> looks
> >> >> OK to me from code review.
> >> > Hmm. pskb_expand_head calls skb_release_data while keeping
> >> > references to pages. How is that ok? What do I miss?
> >> It's making copy of the skb_shinfo earlier, so the pages refcount
> >> stays the same.
> > Exactly. But the callback is invoked so the guest thinks it's ok to
> > change this memory. If it does a corrupted packet will be sent out.
> 
> Hmm. I tool a quick look at skb_clone(), and it looks like this
> sequence will break this scheme:
> 
> skb2 = skb_clone(skb...);
> kfree_skb(skb) or pskb_expand_head(skb);  /* callback called */
> [use skb2, pages still referenced]
> kfree_skb(skb); /* callback called again */
> 
> This sequence is common in bridge, might be in other places.
> 
> Maybe this ubuf thing should just track clones? This will make it work
> on all devices then.

The callback was only invoked when last reference of skb was gone.
skb_clone does increase skb refcnt. I tested tcpdump on lower device, it
worked.

For the sequence of:

skb_clone  -> last refcnt + 1
kfree_skb() or pskb_expand_head -> callback not called
kfree_skb() -> callback called

I will check page refcount to see whether it's balanced.

Thanks
shirley

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michael S. Tsirkin]

On Wed, May 18, 2011 at 07:38:27AM -0700, Shirley Ma wrote:
> On Wed, 2011-05-18 at 13:40 +0200, Michał Mirosław wrote:
> > >> >> Not more other restrictions, skb clone is OK. pskb_expand_head()
> > looks
> > >> >> OK to me from code review.
> > >> > Hmm. pskb_expand_head calls skb_release_data while keeping
> > >> > references to pages. How is that ok? What do I miss?
> > >> It's making copy of the skb_shinfo earlier, so the pages refcount
> > >> stays the same.
> > > Exactly. But the callback is invoked so the guest thinks it's ok to
> > > change this memory. If it does a corrupted packet will be sent out.
> > 
> > Hmm. I tool a quick look at skb_clone(), and it looks like this
> > sequence will break this scheme:
> > 
> > skb2 = skb_clone(skb...);
> > kfree_skb(skb) or pskb_expand_head(skb);  /* callback called */
> > [use skb2, pages still referenced]
> > kfree_skb(skb); /* callback called again */
> > 
> > This sequence is common in bridge, might be in other places.
> > 
> > Maybe this ubuf thing should just track clones? This will make it work
> > on all devices then.
> 
> The callback was only invoked when last reference of skb was gone.
> skb_clone does increase skb refcnt. I tested tcpdump on lower device, it
> worked.

Right, it will normally work, but two issues I think you miss:
1. malicious guest can change the memory between when it is sent out by
   device and consumed by tcpdump, so you will see different things
   (not sure how important this is).
2. if tcpdump stops consuming stuff from the packet socket (it's
   userspace, can't be trusted) then we won't get a callback for
   page potentially forever, guest networking will get blocked etc.

> For the sequence of:
> 
> skb_clone  -> last refcnt + 1
> kfree_skb() or pskb_expand_head -> callback not called
> kfree_skb() -> callback called
> 
> I will check page refcount to see whether it's balanced.
> 
> Thanks
> shirley


pskb_expand_head is a problem anyway I think as it
can hang on to pages after it calls release_data.
Then guest will modify these pages and you get trash there.

-- 
MST

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Shirley Ma]

On Wed, 2011-05-18 at 18:47 +0300, Michael S. Tsirkin wrote:
> On Wed, May 18, 2011 at 07:38:27AM -0700, Shirley Ma wrote:
> > On Wed, 2011-05-18 at 13:40 +0200, Michał Mirosław wrote:
> > > >> >> Not more other restrictions, skb clone is OK.
> pskb_expand_head()
> > > looks
> > > >> >> OK to me from code review.
> > > >> > Hmm. pskb_expand_head calls skb_release_data while keeping
> > > >> > references to pages. How is that ok? What do I miss?
> > > >> It's making copy of the skb_shinfo earlier, so the pages
> refcount
> > > >> stays the same.
> > > > Exactly. But the callback is invoked so the guest thinks it's ok
> to
> > > > change this memory. If it does a corrupted packet will be sent
> out.
> > > 
> > > Hmm. I tool a quick look at skb_clone(), and it looks like this
> > > sequence will break this scheme:
> > > 
> > > skb2 = skb_clone(skb...);
> > > kfree_skb(skb) or pskb_expand_head(skb);  /* callback called */
> > > [use skb2, pages still referenced]
> > > kfree_skb(skb); /* callback called again */
> > > 
> > > This sequence is common in bridge, might be in other places.
> > > 
> > > Maybe this ubuf thing should just track clones? This will make it
> work
> > > on all devices then.
> > 
> > The callback was only invoked when last reference of skb was gone.
> > skb_clone does increase skb refcnt. I tested tcpdump on lower
> device, it
> > worked.
> 
> Right, it will normally work, but two issues I think you miss:
> 1. malicious guest can change the memory between when it is sent out
> by
>    device and consumed by tcpdump, so you will see different things
>    (not sure how important this is).
> 2. if tcpdump stops consuming stuff from the packet socket (it's
>    userspace, can't be trusted) then we won't get a callback for
>    page potentially forever, guest networking will get blocked etc.
> > For the sequence of:
> > 
> > skb_clone  -> last refcnt + 1
> > kfree_skb() or pskb_expand_head -> callback not called
> > kfree_skb() -> callback called
> > 
> > I will check page refcount to see whether it's balanced.
> > 
> > Thanks
> > shirley
> 
> 
> pskb_expand_head is a problem anyway I think as it
> can hang on to pages after it calls release_data.
> Then guest will modify these pages and you get trash there. 

This can be avoid by allowing pskb_expand_head in fastpath only, I
think. But not sure whether tcpdump can still work with this.

Thanks
Shirley

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michael S. Tsirkin]

On Wed, May 18, 2011 at 09:07:37AM -0700, Shirley Ma wrote:
> On Wed, 2011-05-18 at 18:47 +0300, Michael S. Tsirkin wrote:
> > On Wed, May 18, 2011 at 07:38:27AM -0700, Shirley Ma wrote:
> > > On Wed, 2011-05-18 at 13:40 +0200, Michał Mirosław wrote:
> > > > >> >> Not more other restrictions, skb clone is OK.
> > pskb_expand_head()
> > > > looks
> > > > >> >> OK to me from code review.
> > > > >> > Hmm. pskb_expand_head calls skb_release_data while keeping
> > > > >> > references to pages. How is that ok? What do I miss?
> > > > >> It's making copy of the skb_shinfo earlier, so the pages
> > refcount
> > > > >> stays the same.
> > > > > Exactly. But the callback is invoked so the guest thinks it's ok
> > to
> > > > > change this memory. If it does a corrupted packet will be sent
> > out.
> > > > 
> > > > Hmm. I tool a quick look at skb_clone(), and it looks like this
> > > > sequence will break this scheme:
> > > > 
> > > > skb2 = skb_clone(skb...);
> > > > kfree_skb(skb) or pskb_expand_head(skb);  /* callback called */
> > > > [use skb2, pages still referenced]
> > > > kfree_skb(skb); /* callback called again */
> > > > 
> > > > This sequence is common in bridge, might be in other places.
> > > > 
> > > > Maybe this ubuf thing should just track clones? This will make it
> > work
> > > > on all devices then.
> > > 
> > > The callback was only invoked when last reference of skb was gone.
> > > skb_clone does increase skb refcnt. I tested tcpdump on lower
> > device, it
> > > worked.
> > 
> > Right, it will normally work, but two issues I think you miss:
> > 1. malicious guest can change the memory between when it is sent out
> > by
> >    device and consumed by tcpdump, so you will see different things
> >    (not sure how important this is).
> > 2. if tcpdump stops consuming stuff from the packet socket (it's
> >    userspace, can't be trusted) then we won't get a callback for
> >    page potentially forever, guest networking will get blocked etc.
> > > For the sequence of:
> > > 
> > > skb_clone  -> last refcnt + 1
> > > kfree_skb() or pskb_expand_head -> callback not called
> > > kfree_skb() -> callback called
> > > 
> > > I will check page refcount to see whether it's balanced.
> > > 
> > > Thanks
> > > shirley
> > 
> > 
> > pskb_expand_head is a problem anyway I think as it
> > can hang on to pages after it calls release_data.
> > Then guest will modify these pages and you get trash there. 
> 
> This can be avoid by allowing pskb_expand_head in fastpath only, I
> think. But not sure whether tcpdump can still work with this.
> 
> Thanks
> Shirley

Yes, I agree.  I think for tcpdump, we really need to copy the data
anyway, to avoid guest changing it in between.  So we do that and then
use the copy everywhere, release the old one. Hmm?

-- 
MST

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Shirley Ma]

On Wed, 2011-05-18 at 19:36 +0300, Michael S. Tsirkin wrote:
> On Wed, May 18, 2011 at 09:07:37AM -0700, Shirley Ma wrote:
> > On Wed, 2011-05-18 at 18:47 +0300, Michael S. Tsirkin wrote:
> > > On Wed, May 18, 2011 at 07:38:27AM -0700, Shirley Ma wrote:
> > > > On Wed, 2011-05-18 at 13:40 +0200, Michał Mirosław wrote:
> > > > > >> >> Not more other restrictions, skb clone is OK.
> > > pskb_expand_head()
> > > > > looks
> > > > > >> >> OK to me from code review.
> > > > > >> > Hmm. pskb_expand_head calls skb_release_data while
> keeping
> > > > > >> > references to pages. How is that ok? What do I miss?
> > > > > >> It's making copy of the skb_shinfo earlier, so the pages
> > > refcount
> > > > > >> stays the same.
> > > > > > Exactly. But the callback is invoked so the guest thinks
> it's ok
> > > to
> > > > > > change this memory. If it does a corrupted packet will be
> sent
> > > out.
> > > > > 
> > > > > Hmm. I tool a quick look at skb_clone(), and it looks like
> this
> > > > > sequence will break this scheme:
> > > > > 
> > > > > skb2 = skb_clone(skb...);
> > > > > kfree_skb(skb) or pskb_expand_head(skb);  /* callback called
> */
> > > > > [use skb2, pages still referenced]
> > > > > kfree_skb(skb); /* callback called again */
> > > > > 
> > > > > This sequence is common in bridge, might be in other places.
> > > > > 
> > > > > Maybe this ubuf thing should just track clones? This will make
> it
> > > work
> > > > > on all devices then.
> > > > 
> > > > The callback was only invoked when last reference of skb was
> gone.
> > > > skb_clone does increase skb refcnt. I tested tcpdump on lower
> > > device, it
> > > > worked.
> > > 
> > > Right, it will normally work, but two issues I think you miss:
> > > 1. malicious guest can change the memory between when it is sent
> out
> > > by
> > >    device and consumed by tcpdump, so you will see different
> things
> > >    (not sure how important this is).
> > > 2. if tcpdump stops consuming stuff from the packet socket (it's
> > >    userspace, can't be trusted) then we won't get a callback for
> > >    page potentially forever, guest networking will get blocked
> etc.
> > > > For the sequence of:
> > > > 
> > > > skb_clone  -> last refcnt + 1
> > > > kfree_skb() or pskb_expand_head -> callback not called
> > > > kfree_skb() -> callback called
> > > > 
> > > > I will check page refcount to see whether it's balanced.
> > > > 
> > > > Thanks
> > > > shirley
> > > 
> > > 
> > > pskb_expand_head is a problem anyway I think as it
> > > can hang on to pages after it calls release_data.
> > > Then guest will modify these pages and you get trash there. 
> > 
> > This can be avoid by allowing pskb_expand_head in fastpath only, I
> > think. But not sure whether tcpdump can still work with this.
> > 
> > Thanks
> > Shirley
> 
> Yes, I agree.  I think for tcpdump, we really need to copy the data
> anyway, to avoid guest changing it in between.  So we do that and then
> use the copy everywhere, release the old one. Hmm? 

Yes. Old one use zerocopy, new one use copy data.

Thanks
Shirley

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michael S. Tsirkin]

On Wed, May 18, 2011 at 09:45:40AM -0700, Shirley Ma wrote:
> On Wed, 2011-05-18 at 19:36 +0300, Michael S. Tsirkin wrote:
> > On Wed, May 18, 2011 at 09:07:37AM -0700, Shirley Ma wrote:
> > > On Wed, 2011-05-18 at 18:47 +0300, Michael S. Tsirkin wrote:
> > > > On Wed, May 18, 2011 at 07:38:27AM -0700, Shirley Ma wrote:
> > > > > On Wed, 2011-05-18 at 13:40 +0200, Michał Mirosław wrote:
> > > > > > >> >> Not more other restrictions, skb clone is OK.
> > > > pskb_expand_head()
> > > > > > looks
> > > > > > >> >> OK to me from code review.
> > > > > > >> > Hmm. pskb_expand_head calls skb_release_data while
> > keeping
> > > > > > >> > references to pages. How is that ok? What do I miss?
> > > > > > >> It's making copy of the skb_shinfo earlier, so the pages
> > > > refcount
> > > > > > >> stays the same.
> > > > > > > Exactly. But the callback is invoked so the guest thinks
> > it's ok
> > > > to
> > > > > > > change this memory. If it does a corrupted packet will be
> > sent
> > > > out.
> > > > > > 
> > > > > > Hmm. I tool a quick look at skb_clone(), and it looks like
> > this
> > > > > > sequence will break this scheme:
> > > > > > 
> > > > > > skb2 = skb_clone(skb...);
> > > > > > kfree_skb(skb) or pskb_expand_head(skb);  /* callback called
> > */
> > > > > > [use skb2, pages still referenced]
> > > > > > kfree_skb(skb); /* callback called again */
> > > > > > 
> > > > > > This sequence is common in bridge, might be in other places.
> > > > > > 
> > > > > > Maybe this ubuf thing should just track clones? This will make
> > it
> > > > work
> > > > > > on all devices then.
> > > > > 
> > > > > The callback was only invoked when last reference of skb was
> > gone.
> > > > > skb_clone does increase skb refcnt. I tested tcpdump on lower
> > > > device, it
> > > > > worked.
> > > > 
> > > > Right, it will normally work, but two issues I think you miss:
> > > > 1. malicious guest can change the memory between when it is sent
> > out
> > > > by
> > > >    device and consumed by tcpdump, so you will see different
> > things
> > > >    (not sure how important this is).
> > > > 2. if tcpdump stops consuming stuff from the packet socket (it's
> > > >    userspace, can't be trusted) then we won't get a callback for
> > > >    page potentially forever, guest networking will get blocked
> > etc.
> > > > > For the sequence of:
> > > > > 
> > > > > skb_clone  -> last refcnt + 1
> > > > > kfree_skb() or pskb_expand_head -> callback not called
> > > > > kfree_skb() -> callback called
> > > > > 
> > > > > I will check page refcount to see whether it's balanced.
> > > > > 
> > > > > Thanks
> > > > > shirley
> > > > 
> > > > 
> > > > pskb_expand_head is a problem anyway I think as it
> > > > can hang on to pages after it calls release_data.
> > > > Then guest will modify these pages and you get trash there. 
> > > 
> > > This can be avoid by allowing pskb_expand_head in fastpath only, I
> > > think. But not sure whether tcpdump can still work with this.
> > > 
> > > Thanks
> > > Shirley
> > 
> > Yes, I agree.  I think for tcpdump, we really need to copy the data
> > anyway, to avoid guest changing it in between.  So we do that and then
> > use the copy everywhere, release the old one. Hmm? 
> 
> Yes. Old one use zerocopy, new one use copy data.
> 
> Thanks
> Shirley

No, that's wrong, as they might become different with a
malicious guest. As long as we copied already, lets realease
the data and have everyone use the copy.

-- 
MST

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Shirley Ma]

On Wed, 2011-05-18 at 19:51 +0300, Michael S. Tsirkin wrote:
> > > Yes, I agree.  I think for tcpdump, we really need to copy the
> data
> > > anyway, to avoid guest changing it in between.  So we do that and
> then
> > > use the copy everywhere, release the old one. Hmm? 
> > 
> > Yes. Old one use zerocopy, new one use copy data.
> > 
> > Thanks
> > Shirley
> 
> No, that's wrong, as they might become different with a
> malicious guest. As long as we copied already, lets realease
> the data and have everyone use the copy. 

Ok, I will patch pskb_expand_head to test it out.

Shirley

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Shirley Ma]

On Wed, 2011-05-18 at 10:00 -0700, Shirley Ma wrote:
> On Wed, 2011-05-18 at 19:51 +0300, Michael S. Tsirkin wrote:
> > > > Yes, I agree.  I think for tcpdump, we really need to copy the
> > data
> > > > anyway, to avoid guest changing it in between.  So we do that
> and
> > then
> > > > use the copy everywhere, release the old one. Hmm? 
> > > 
> > > Yes. Old one use zerocopy, new one use copy data.
> > > 
> > > Thanks
> > > Shirley
> > 
> > No, that's wrong, as they might become different with a
> > malicious guest. As long as we copied already, lets realease
> > the data and have everyone use the copy. 
> 
> Ok, I will patch pskb_expand_head to test it out. 

I am patching skb_copy, skb_clone, pskb_copy, pskb_expand_head to
convert a zero-copy skb to a copy skb to avoid this kind of issue.

This overhead won't impact macvtap/vhost TX zero-copy normally.

Shirley

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michael S. Tsirkin]

On Thu, May 19, 2011 at 12:42:49PM -0700, Shirley Ma wrote:
> On Wed, 2011-05-18 at 10:00 -0700, Shirley Ma wrote:
> > On Wed, 2011-05-18 at 19:51 +0300, Michael S. Tsirkin wrote:
> > > > > Yes, I agree.  I think for tcpdump, we really need to copy the
> > > data
> > > > > anyway, to avoid guest changing it in between.  So we do that
> > and
> > > then
> > > > > use the copy everywhere, release the old one. Hmm? 
> > > > 
> > > > Yes. Old one use zerocopy, new one use copy data.
> > > > 
> > > > Thanks
> > > > Shirley
> > > 
> > > No, that's wrong, as they might become different with a
> > > malicious guest. As long as we copied already, lets realease
> > > the data and have everyone use the copy. 
> > 
> > Ok, I will patch pskb_expand_head to test it out. 
> 
> I am patching skb_copy, skb_clone, pskb_copy, pskb_expand_head to
> convert a zero-copy skb to a copy skb to avoid this kind of issue.
> 
> This overhead won't impact macvtap/vhost TX zero-copy normally.
> 
> Shirley

OK, that will handle packet socket at least in that it won't crash :)

So the requirements are
- data must be released in a timely fashion (e.g. unlike virtio-net
  tun or bridge)
- no filtering based on data (data is mapped in guest)
- SG support
- HIGHDMA support (on arches where this makes sense)
- on fast path no calls to skb_copy, skb_clone, pskb_copy,
  pskb_expand_head as these are slow

First 2 requirements are a must, all other requirements
are just dependencies to make sure zero copy will be faster
than non zero copy.

Using a new feature bit is probably the simplest approach to
this. macvtap on top of most physical NICs most likely works
correctly so it seems a bit more work than it needs to be,
but it's also the safest one I think ...

-- 
MST

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Shirley Ma]

On Fri, 2011-05-20 at 02:41 +0300, Michael S. Tsirkin wrote:
> So the requirements are
> - data must be released in a timely fashion (e.g. unlike virtio-net
>   tun or bridge)
The current patch doesn't enable tun zero-copy. tun will copy data It's
not an issue now. We can disallow macvtap attach to bridge when
zero-copy is enabled.

> - SG support
> - HIGHDMA support (on arches where this makes sense)

This can be checked by device flags.

> - no filtering based on data (data is mapped in guest)

> - on fast path no calls to skb_copy, skb_clone, pskb_copy,
>   pskb_expand_head as these are slow

Any calls to skb_copy, skb_clone, pskb_copy, pskb_expand_head will do a
copy. The performance should be the same as none zero-copy case before.
I have done/tested the patch V6, will send it out for review tomorrow.

I am looking at where there are some cases, skb remains the same for
filtering.

> First 2 requirements are a must, all other requirements
> are just dependencies to make sure zero copy will be faster
> than non zero copy.
> Using a new feature bit is probably the simplest approach to
> this. macvtap on top of most physical NICs most likely works
> correctly so it seems a bit more work than it needs to be,
> but it's also the safest one I think ... 

For "macvtap/vhost zero-copy" we can use SG & HIGHDMA to enable it, it
looks safe to me once patching skb_copy, skb_clone, pskb_copy,
pskb_expand_head.

To extend zero-copy in other usages, we can have a new feature bit
later.

Is that reasonable?

Thanks
Shirley

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michael S. Tsirkin]

On Wed, May 25, 2011 at 03:49:40PM -0700, Shirley Ma wrote:
> On Fri, 2011-05-20 at 02:41 +0300, Michael S. Tsirkin wrote:
> > So the requirements are
> > - data must be released in a timely fashion (e.g. unlike virtio-net
> >   tun or bridge)
> The current patch doesn't enable tun zero-copy. tun will copy data It's
> not an issue now.
> We can disallow macvtap attach to bridge when
> zero-copy is enabled.

Attach macvtap to a tun device though. Or e.g. veth device ...
So there should be so generic way to disable zerocopy.
It can either be a whitelist or a blacklist.

> 
> > - SG support
> > - HIGHDMA support (on arches where this makes sense)
> 
> This can be checked by device flags.

OK, but pls note that SG can get turned off dynamically.

> > - no filtering based on data (data is mapped in guest)
> 
> > - on fast path no calls to skb_copy, skb_clone, pskb_copy,
> >   pskb_expand_head as these are slow
> 
> Any calls to skb_copy, skb_clone, pskb_copy, pskb_expand_head will do a
> copy. The performance should be the same as none zero-copy case before.

I'm guessing a copy is cheaper than get_user_pages+copy+put_page.
But maybe not by much. Care checking that?

> I have done/tested the patch V6, will send it out for review tomorrow.
> 
> I am looking at where there are some cases, skb remains the same for
> filtering.

To reliably filter on data I think we'll need to copy it first, otherwise
guest can change it. Most filters only look at the header though.

> > First 2 requirements are a must, all other requirements
> > are just dependencies to make sure zero copy will be faster
> > than non zero copy.
> > Using a new feature bit is probably the simplest approach to
> > this. macvtap on top of most physical NICs most likely works
> > correctly so it seems a bit more work than it needs to be,
> > but it's also the safest one I think ... 
> 
> For "macvtap/vhost zero-copy" we can use SG & HIGHDMA to enable it, it
> looks safe to me once patching skb_copy, skb_clone, pskb_copy,
> pskb_expand_head.
> 
> To extend zero-copy in other usages, we can have a new feature bit
> later.
> 
> Is that reasonable?
> 
> Thanks
> Shirley

Is the problem is extra work needed to extend feature bits?

-- 
MST

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Shirley Ma]

On Thu, 2011-05-26 at 11:49 +0300, Michael S. Tsirkin wrote:
> On Wed, May 25, 2011 at 03:49:40PM -0700, Shirley Ma wrote:
> > On Fri, 2011-05-20 at 02:41 +0300, Michael S. Tsirkin wrote:
> > > So the requirements are
> > > - data must be released in a timely fashion (e.g. unlike
> virtio-net
> > >   tun or bridge)
> > The current patch doesn't enable tun zero-copy. tun will copy data
> It's
> > not an issue now.
> > We can disallow macvtap attach to bridge when
> > zero-copy is enabled.
> 
> Attach macvtap to a tun device though. Or e.g. veth device ...
> So there should be so generic way to disable zerocopy.
> It can either be a whitelist or a blacklist.
> > 
> > > - SG support
> > > - HIGHDMA support (on arches where this makes sense)
> > 
> > This can be checked by device flags.
> 
> OK, but pls note that SG can get turned off dynamically.
> 
> > > - no filtering based on data (data is mapped in guest)
> > 
> > > - on fast path no calls to skb_copy, skb_clone, pskb_copy,
> > >   pskb_expand_head as these are slow
> > 
> > Any calls to skb_copy, skb_clone, pskb_copy, pskb_expand_head will
> do a
> > copy. The performance should be the same as none zero-copy case
> before.
> 
> I'm guessing a copy is cheaper than get_user_pages+copy+put_page.
> But maybe not by much. Care checking that?

That's I have done already. Patch is going out for review.

> > I have done/tested the patch V6, will send it out for review
> tomorrow.
> > 
> > I am looking at where there are some cases, skb remains the same for
> > filtering.
> 
> To reliably filter on data I think we'll need to copy it first,
> otherwise
> guest can change it. Most filters only look at the header though.
> 
> > > First 2 requirements are a must, all other requirements
> > > are just dependencies to make sure zero copy will be faster
> > > than non zero copy.
> > > Using a new feature bit is probably the simplest approach to
> > > this. macvtap on top of most physical NICs most likely works
> > > correctly so it seems a bit more work than it needs to be,
> > > but it's also the safest one I think ... 
> > 
> > For "macvtap/vhost zero-copy" we can use SG & HIGHDMA to enable it,
> it
> > looks safe to me once patching skb_copy, skb_clone, pskb_copy,
> > pskb_expand_head.
> > 
> > To extend zero-copy in other usages, we can have a new feature bit
> > later.
> > 
> > Is that reasonable?
> > 
> > Thanks
> > Shirley
> 
> Is the problem is extra work needed to extend feature bits?

There is no problem to use it, Mahesh is working on this patch. I just
want to remove macvtap/vhost zero-copy patch dependency.

Thanks
Shirley

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Shirley Ma]

On Thu, 2011-05-26 at 11:49 +0300, Michael S. Tsirkin wrote:
> > 
> > > - SG support
> > > - HIGHDMA support (on arches where this makes sense)
> > 
> > This can be checked by device flags.
> 
> OK, but pls note that SG can get turned off dynamically.

Tested the patch w/i SG dynmically on/off and tcpdump suspended.

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Shirley Ma]

On Wed, 2011-05-18 at 07:38 -0700, Shirley Ma wrote:
> On Wed, 2011-05-18 at 13:40 +0200, Michał Mirosław wrote:
> > >> >> Not more other restrictions, skb clone is OK.
> pskb_expand_head()
> > looks
> > >> >> OK to me from code review.
> > >> > Hmm. pskb_expand_head calls skb_release_data while keeping
> > >> > references to pages. How is that ok? What do I miss?
> > >> It's making copy of the skb_shinfo earlier, so the pages refcount
> > >> stays the same.
> > > Exactly. But the callback is invoked so the guest thinks it's ok
> to
> > > change this memory. If it does a corrupted packet will be sent
> out.
> > 
> > Hmm. I tool a quick look at skb_clone(), and it looks like this
> > sequence will break this scheme:
> > 
> > skb2 = skb_clone(skb...);
> > kfree_skb(skb) or pskb_expand_head(skb);  /* callback called */
> > [use skb2, pages still referenced]
> > kfree_skb(skb); /* callback called again */
> > 
> > This sequence is common in bridge, might be in other places.
> > 
> > Maybe this ubuf thing should just track clones? This will make it
> work
> > on all devices then.
> 
> The callback was only invoked when last reference of skb was gone.
> skb_clone does increase skb refcnt. I tested tcpdump on lower device,
> it
> worked.
> 
> For the sequence of:
> 
> skb_clone  -> last refcnt + 1
> kfree_skb() or pskb_expand_head -> callback not called
> kfree_skb() -> callback called
> 
> I will check page refcount to see whether it's balanced. 

The page refcounts are balanced too. 

In macvtap/vhost Real NIC zerocopy case, it always goes to fastpath in
pskb_expand_head, so I didn't hit any issue.

But rethinking about pskb_expand_head(), it calls skb_release_data() to
free old skb head when it's not in the fastpath (pskb_expand_head is not
the last reference of this skb); And it's impossible to track which skb
head (old one or new one) will be the last one to free. So better to
return error for zero-copy skbs when not using fastpath. Does it make
sense? 

Besides this, any other issue?


Thanks
Shirley

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michael S. Tsirkin]

On Wed, May 18, 2011 at 09:02:23AM -0700, Shirley Ma wrote:
> On Wed, 2011-05-18 at 07:38 -0700, Shirley Ma wrote:
> > On Wed, 2011-05-18 at 13:40 +0200, Michał Mirosław wrote:
> > > >> >> Not more other restrictions, skb clone is OK.
> > pskb_expand_head()
> > > looks
> > > >> >> OK to me from code review.
> > > >> > Hmm. pskb_expand_head calls skb_release_data while keeping
> > > >> > references to pages. How is that ok? What do I miss?
> > > >> It's making copy of the skb_shinfo earlier, so the pages refcount
> > > >> stays the same.
> > > > Exactly. But the callback is invoked so the guest thinks it's ok
> > to
> > > > change this memory. If it does a corrupted packet will be sent
> > out.
> > > 
> > > Hmm. I tool a quick look at skb_clone(), and it looks like this
> > > sequence will break this scheme:
> > > 
> > > skb2 = skb_clone(skb...);
> > > kfree_skb(skb) or pskb_expand_head(skb);  /* callback called */
> > > [use skb2, pages still referenced]
> > > kfree_skb(skb); /* callback called again */
> > > 
> > > This sequence is common in bridge, might be in other places.
> > > 
> > > Maybe this ubuf thing should just track clones? This will make it
> > work
> > > on all devices then.
> > 
> > The callback was only invoked when last reference of skb was gone.
> > skb_clone does increase skb refcnt. I tested tcpdump on lower device,
> > it
> > worked.
> > 
> > For the sequence of:
> > 
> > skb_clone  -> last refcnt + 1
> > kfree_skb() or pskb_expand_head -> callback not called
> > kfree_skb() -> callback called
> > 
> > I will check page refcount to see whether it's balanced. 
> 
> The page refcounts are balanced too. 
> 
> In macvtap/vhost Real NIC zerocopy case, it always goes to fastpath in
> pskb_expand_head, so I didn't hit any issue.
> 
> But rethinking about pskb_expand_head(), it calls skb_release_data() to
> free old skb head when it's not in the fastpath (pskb_expand_head is not
> the last reference of this skb); And it's impossible to track which skb
> head (old one or new one) will be the last one to free. So better to
> return error for zero-copy skbs when not using fastpath. Does it make
> sense? 

I'm not sure it does. Look e.g. at tg3 - if expand_head fails
packet gets dropped. No crash but unlikely to perform well :).

> Besides this, any other issue?
> 
> 
> Thanks
> Shirley

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michael S. Tsirkin]

On Wed, May 18, 2011 at 01:40:29PM +0200, Michał Mirosław wrote:
> W dniu 18 maja 2011 13:17 użytkownik Michael S. Tsirkin
> <mst@redhat.com> napisał:
> > On Wed, May 18, 2011 at 01:10:50PM +0200, Michał Mirosław wrote:
> >> 2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
> >> > On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
> >> >> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
> >> >> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
> >> >> > > Hello Michael,
> >> >> > >
> >> >> > > Looks like to use a new flag requires more time/work. I am thinking
> >> >> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
> >> >> > to
> >> >> > > avoid the new flag for now since mavctap uses real NICs as lower
> >> >> > device?
> >> >> >
> >> >> > Is there any other restriction besides requiring driver to not recycle
> >> >> > the skb? Are there any drivers that recycle TX skbs?
> >> >
> >> > Not just recycling skbs, keeping reference to any of the pages in the
> >> > skb. Another requirement is to invoke the callback
> >> > in a timely fashion.  For example virtio-net doesn't limit the time until
> >> > that happens (skbs are only freed when some other packet is
> >> > transmitted), so we need to avoid zcopy for such (nested-virt)
> >> > scenarious, right?
> >>
> >> Hmm. But every hardware driver supporting SG will keep reference to
> >> the pages until the packet is sent (or DMA'd to the device). This can
> >> take a long time if hardware queue happens to stall for some reason.
> >
> > That's a fundamental property of zero copy transmit.
> > You can't let the application/guest reuse the memory until
> > no one looks at it anymore.
> >
> >> Is it that you mean keeping a reference after all skbs pointing to the
> >> pages are released?
> > No one should reference the pages after the callback is invoked, yes.
> 
> >> >> Not more other restrictions, skb clone is OK. pskb_expand_head() looks
> >> >> OK to me from code review.
> >> > Hmm. pskb_expand_head calls skb_release_data while keeping
> >> > references to pages. How is that ok? What do I miss?
> >> It's making copy of the skb_shinfo earlier, so the pages refcount
> >> stays the same.
> > Exactly. But the callback is invoked so the guest thinks it's ok to
> > change this memory. If it does a corrupted packet will be sent out.
> 
> Hmm. I tool a quick look at skb_clone(), and it looks like this
> sequence will break this scheme:
> 
> skb2 = skb_clone(skb...);
> kfree_skb(skb) or pskb_expand_head(skb);  /* callback called */
> [use skb2, pages still referenced]
> kfree_skb(skb); /* callback called again */
> This sequence is common in bridge, might be in other places.
> 
> Maybe this ubuf thing should just track clones? This will make it work
> on all devices then.
> 
> Best Regards,
> Michał Mirosław

Well bridge has the problem that packet might get anywhere and it's
really hard to track. Same for tun - it can get queued forever.
veth, loopback are all a problem I think.

IOW we really want to limit this to real physical NICs
which mostly all DTRT. Whitelisting them with a new flag
is likely the most concervative approach, no?

-- 
MST

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michał Mirosław]

W dniu 18 maja 2011 13:17 użytkownik Michael S. Tsirkin
<mst@redhat.com> napisał:
> On Wed, May 18, 2011 at 01:10:50PM +0200, Michał Mirosław wrote:
>> 2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
>> > On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
>> >> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
>> >> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
>> >> > > Hello Michael,
>> >> > >
>> >> > > Looks like to use a new flag requires more time/work. I am thinking
>> >> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
>> >> > to
>> >> > > avoid the new flag for now since mavctap uses real NICs as lower
>> >> > device?
>> >> >
>> >> > Is there any other restriction besides requiring driver to not recycle
>> >> > the skb? Are there any drivers that recycle TX skbs?
>> > Not just recycling skbs, keeping reference to any of the pages in the
>> > skb. Another requirement is to invoke the callback
>> > in a timely fashion.  For example virtio-net doesn't limit the time until
>> > that happens (skbs are only freed when some other packet is
>> > transmitted), so we need to avoid zcopy for such (nested-virt)
>> > scenarious, right?
>> Hmm. But every hardware driver supporting SG will keep reference to
>> the pages until the packet is sent (or DMA'd to the device). This can
>> take a long time if hardware queue happens to stall for some reason.
> That's a fundamental property of zero copy transmit.
> You can't let the application/guest reuse the memory until
> no one looks at it anymore.

One more question: is userspace (or whatever is sending those packets)
denied from modifying passed pages? I assume it is, but just want to
be sure.

Best Regards,
Michał Mirosław

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michael S. Tsirkin]

On Wed, May 18, 2011 at 01:47:33PM +0200, Michał Mirosław wrote:
> W dniu 18 maja 2011 13:17 użytkownik Michael S. Tsirkin
> <mst@redhat.com> napisał:
> > On Wed, May 18, 2011 at 01:10:50PM +0200, Michał Mirosław wrote:
> >> 2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
> >> > On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
> >> >> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
> >> >> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
> >> >> > > Hello Michael,
> >> >> > >
> >> >> > > Looks like to use a new flag requires more time/work. I am thinking
> >> >> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
> >> >> > to
> >> >> > > avoid the new flag for now since mavctap uses real NICs as lower
> >> >> > device?
> >> >> >
> >> >> > Is there any other restriction besides requiring driver to not recycle
> >> >> > the skb? Are there any drivers that recycle TX skbs?
> >> > Not just recycling skbs, keeping reference to any of the pages in the
> >> > skb. Another requirement is to invoke the callback
> >> > in a timely fashion.  For example virtio-net doesn't limit the time until
> >> > that happens (skbs are only freed when some other packet is
> >> > transmitted), so we need to avoid zcopy for such (nested-virt)
> >> > scenarious, right?
> >> Hmm. But every hardware driver supporting SG will keep reference to
> >> the pages until the packet is sent (or DMA'd to the device). This can
> >> take a long time if hardware queue happens to stall for some reason.
> > That's a fundamental property of zero copy transmit.
> > You can't let the application/guest reuse the memory until
> > no one looks at it anymore.
> 
> One more question: is userspace (or whatever is sending those packets)
> denied from modifying passed pages? I assume it is, but just want to
> be sure.
> 
> Best Regards,
> Michał Mirosław

Good point.

It's not denied in the sense that it still can modify them if it's
buggy (the pages might not be read-only).
But well-behaved userspace won't modify them until the callback
is invoked.

That would be a problem if the underlying device is
a bridge where we might try to e.g. filter these packets -
data can get modified after the filter. We'd have to copy
whatever the filter accesses and use the copy - it's rarely
the data itself.

That's not normally a problem for macvtap connected to a physical NIC,
as that already bypasses any and all filtering.

But that's another limitation we should note in the comment,
and another reason to limit to specific devices.

-- 
MST

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michał Mirosław]

W dniu 18 maja 2011 13:56 użytkownik Michael S. Tsirkin
<mst@redhat.com> napisał:
> On Wed, May 18, 2011 at 01:47:33PM +0200, Michał Mirosław wrote:
>> W dniu 18 maja 2011 13:17 użytkownik Michael S. Tsirkin
>> <mst@redhat.com> napisał:
>> > On Wed, May 18, 2011 at 01:10:50PM +0200, Michał Mirosław wrote:
>> >> 2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
>> >> > On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
>> >> >> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
>> >> >> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
>> >> >> > > Hello Michael,
>> >> >> > >
>> >> >> > > Looks like to use a new flag requires more time/work. I am thinking
>> >> >> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
>> >> >> > to
>> >> >> > > avoid the new flag for now since mavctap uses real NICs as lower
>> >> >> > device?
>> >> >> >
>> >> >> > Is there any other restriction besides requiring driver to not recycle
>> >> >> > the skb? Are there any drivers that recycle TX skbs?
>> >> > Not just recycling skbs, keeping reference to any of the pages in the
>> >> > skb. Another requirement is to invoke the callback
>> >> > in a timely fashion.  For example virtio-net doesn't limit the time until
>> >> > that happens (skbs are only freed when some other packet is
>> >> > transmitted), so we need to avoid zcopy for such (nested-virt)
>> >> > scenarious, right?
>> >> Hmm. But every hardware driver supporting SG will keep reference to
>> >> the pages until the packet is sent (or DMA'd to the device). This can
>> >> take a long time if hardware queue happens to stall for some reason.
>> > That's a fundamental property of zero copy transmit.
>> > You can't let the application/guest reuse the memory until
>> > no one looks at it anymore.
>>
>> One more question: is userspace (or whatever is sending those packets)
>> denied from modifying passed pages? I assume it is, but just want to
>> be sure.
>>
>> Best Regards,
>> Michał Mirosław
>
> Good point.
>
> It's not denied in the sense that it still can modify them if it's
> buggy (the pages might not be read-only).
> But well-behaved userspace won't modify them until the callback
> is invoked.
>
> That would be a problem if the underlying device is
> a bridge where we might try to e.g. filter these packets -
> data can get modified after the filter. We'd have to copy
> whatever the filter accesses and use the copy - it's rarely
> the data itself.
>
> That's not normally a problem for macvtap connected to a physical NIC,
> as that already bypasses any and all filtering.
>
> But that's another limitation we should note in the comment,
> and another reason to limit to specific devices.

It looks like this feature can be used only in very strict circumstances.

What about tcpdump listening on the device or lowerdev? This path
might clone the skb for any device.

Best Regards,
Michał Mirosław

Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

[Michael S. Tsirkin]

On Wed, May 18, 2011 at 02:48:24PM +0200, Michał Mirosław wrote:
> W dniu 18 maja 2011 13:56 użytkownik Michael S. Tsirkin
> <mst@redhat.com> napisał:
> > On Wed, May 18, 2011 at 01:47:33PM +0200, Michał Mirosław wrote:
> >> W dniu 18 maja 2011 13:17 użytkownik Michael S. Tsirkin
> >> <mst@redhat.com> napisał:
> >> > On Wed, May 18, 2011 at 01:10:50PM +0200, Michał Mirosław wrote:
> >> >> 2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
> >> >> > On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
> >> >> >> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
> >> >> >> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
> >> >> >> > > Hello Michael,
> >> >> >> > >
> >> >> >> > > Looks like to use a new flag requires more time/work. I am thinking
> >> >> >> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
> >> >> >> > to
> >> >> >> > > avoid the new flag for now since mavctap uses real NICs as lower
> >> >> >> > device?
> >> >> >> >
> >> >> >> > Is there any other restriction besides requiring driver to not recycle
> >> >> >> > the skb? Are there any drivers that recycle TX skbs?
> >> >> > Not just recycling skbs, keeping reference to any of the pages in the
> >> >> > skb. Another requirement is to invoke the callback
> >> >> > in a timely fashion.  For example virtio-net doesn't limit the time until
> >> >> > that happens (skbs are only freed when some other packet is
> >> >> > transmitted), so we need to avoid zcopy for such (nested-virt)
> >> >> > scenarious, right?
> >> >> Hmm. But every hardware driver supporting SG will keep reference to
> >> >> the pages until the packet is sent (or DMA'd to the device). This can
> >> >> take a long time if hardware queue happens to stall for some reason.
> >> > That's a fundamental property of zero copy transmit.
> >> > You can't let the application/guest reuse the memory until
> >> > no one looks at it anymore.
> >>
> >> One more question: is userspace (or whatever is sending those packets)
> >> denied from modifying passed pages? I assume it is, but just want to
> >> be sure.
> >>
> >> Best Regards,
> >> Michał Mirosław
> >
> > Good point.
> >
> > It's not denied in the sense that it still can modify them if it's
> > buggy (the pages might not be read-only).
> > But well-behaved userspace won't modify them until the callback
> > is invoked.
> >
> > That would be a problem if the underlying device is
> > a bridge where we might try to e.g. filter these packets -
> > data can get modified after the filter. We'd have to copy
> > whatever the filter accesses and use the copy - it's rarely
> > the data itself.
> >
> > That's not normally a problem for macvtap connected to a physical NIC,
> > as that already bypasses any and all filtering.
> >
> > But that's another limitation we should note in the comment,
> > and another reason to limit to specific devices.
> 
> It looks like this feature can be used only in very strict circumstances.

True. I think it's reasonable to try and start with something
restricted and then add features though - past attempts to solve the problem
generally right away did not bear fruit.

> What about tcpdump listening on the device or lowerdev? This path
> might clone the skb for any device.
> 
> Best Regards,
> Michał Mirosław

Thanks for bringing this up: taps do need to be fixed as they can hang
on to a page for unlimited time. Further, as a malicious guest can
change the packet at any time, data that taps get wouldn't be correct.
We can either linearize the problematic skbs or disable
zero copy if there are any taps for the given device.

-- 
MST