[Qemu-devel] [PATCH v1 0/1] Allow TAP to unnegotiate virtio-net header

[Qemu-devel] [PATCH v1 0/1] Allow TAP to unnegotiate virtio-net header

[Vincenzo Maffione]

Hi,
  I was doing experiments with a custom paravirtualized net device,
and I ran into a limitation of the TAP backend. I see from the kernel
code that it is not possible to set the TAP virtio-net header
length to something different from 10 or 12, which means that it
is not possible to set it to 0. That's fine.
The QEMU implementation of the TAP backend already supports
the case where virtio-net header is set to 10 or 12 in kernel,
but the emulated/paravirtualized net device is not using that:
the TAP backend will just strip/prepend a zeroed header in this case
(e.g., see the if statement in tap_receive_iov()).
However, the tap_using_vnet_hdr() has an assert() that prevents
this situation to happen, and I don't understand why. Maybe it
is a leftover? I tried to remove the assert and by doing that
my paravirtualized device was able to stop using the virtio-net
header.

Vincenzo Maffione (1):
  net: tap: allow net frontends to un-negotiate virtio-net header

 net/tap.c | 1 -
 1 file changed, 1 deletion(-)

-- 
2.20.1

[Qemu-devel] [PATCH v1 1/1] net: tap: allow net frontends to un-negotiate virtio-net header

[Vincenzo Maffione]

The implementation of the TAP net backend already supports the case
(s->host_vnet_hdr_len && !s->using_vnet_hdr), which means that
the TAP device is expecting the header, while the net frontend
(emulated device) is not aware of it. This case is handled by
stripping or prepending the (zeroed) header on the fly.
However, the function tap_using_vnet_hdr() has an assert() that
explicitly prevents this situation to happen.
This patch removes the assert(), so that net frontends are
free to un-negotiate the header.

Signed-off-by: Vincenzo Maffione <v.maffione@gmail.com>
---
 net/tap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/tap.c b/net/tap.c
index cc8525f154..6f2aca0396 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -264,7 +264,6 @@ static void tap_using_vnet_hdr(NetClientState *nc, bool using_vnet_hdr)
     TAPState *s = DO_UPCAST(TAPState, nc, nc);
 
     assert(nc->info->type == NET_CLIENT_DRIVER_TAP);
-    assert(!!s->host_vnet_hdr_len == using_vnet_hdr);
 
     s->using_vnet_hdr = using_vnet_hdr;
 }
-- 
2.20.1

Re: [Qemu-devel] [PATCH v1 0/1] Allow TAP to unnegotiate virtio-net header

[Jason Wang]

On 2019/2/16 上午1:46, Vincenzo Maffione wrote:
> Hi,
>    I was doing experiments with a custom paravirtualized net device,
> and I ran into a limitation of the TAP backend. I see from the kernel
> code that it is not possible to set the TAP virtio-net header
> length to something different from 10 or 12, which means that it
> is not possible to set it to 0. That's fine.
> The QEMU implementation of the TAP backend already supports
> the case where virtio-net header is set to 10 or 12 in kernel,
> but the emulated/paravirtualized net device is not using that:
> the TAP backend will just strip/prepend a zeroed header in this case
> (e.g., see the if statement in tap_receive_iov()).
> However, the tap_using_vnet_hdr() has an assert() that prevents
> this situation to happen, and I don't understand why. Maybe it
> is a leftover? I tried to remove the assert and by doing that
> my paravirtualized device was able to stop using the virtio-net
> header.


Hi:

If  I understand this correctly, your PV device is API compatible with 
TAP? Then you may just adding code to call TUNSETIFF without IFF_VNET_HDR?

Thanks


>
> Vincenzo Maffione (1):
>    net: tap: allow net frontends to un-negotiate virtio-net header
>
>   net/tap.c | 1 -
>   1 file changed, 1 deletion(-)
>

Re: [Qemu-devel] [PATCH v1 0/1] Allow TAP to unnegotiate virtio-net header

[Vincenzo Maffione]

Hi Jason,
  Thanks for the quick reply.

My PV device (to be open sourced soon) uses the QEMU net backend interface,
in a way similar to virtio-net.
For example it uses qemu_set_offload(), qemu_has_vnet_hdr_len(),
qemu_using_vnet_hdr(), qemu_send_packet(), etc.
This means that the device itself does not know which net backend is using,
of course.
In addition to TAP, also the netmap backend supports the virtio-net header,
and so the PV device will work with both.
Regarding the virtio-net header, the netmap backend is more flexible
because you can change the virtio-net header length as you wish (choosing
between 0, 10 and 12 bytes).

My problem cannot be solved by making sure that TAPs are opened without
IFF_VNET_HDR. What I want instead
is to dynamically change the length of the virtio-net header that the
backend accepts, switching between 12 bytes (virtio 1.0 header) and 0 (no
header). By "dynamically" I mean while the guest (and the device) is
running. This is possible and safe if we get rid of that assert().
The virtio-net device does not need that flexibility (once you enable the
vtnet header, you never disable that again), so I guess that's why you have
that assert().
My PV device can be reconfigured on the fly in such a way it may or not may
be aware of the virtio-net header, and as a consequence it may need to call
qemu_using_vnet_hdr() with true or false (multiple times).

Does it make sense?

Cheers,
  Vincenzo


Il giorno lun 18 feb 2019 alle ore 05:19 Jason Wang <jasowang@redhat.com>
ha scritto:

>
> On 2019/2/16 上午1:46, Vincenzo Maffione wrote:
> > Hi,
> >    I was doing experiments with a custom paravirtualized net device,
> > and I ran into a limitation of the TAP backend. I see from the kernel
> > code that it is not possible to set the TAP virtio-net header
> > length to something different from 10 or 12, which means that it
> > is not possible to set it to 0. That's fine.
> > The QEMU implementation of the TAP backend already supports
> > the case where virtio-net header is set to 10 or 12 in kernel,
> > but the emulated/paravirtualized net device is not using that:
> > the TAP backend will just strip/prepend a zeroed header in this case
> > (e.g., see the if statement in tap_receive_iov()).
> > However, the tap_using_vnet_hdr() has an assert() that prevents
> > this situation to happen, and I don't understand why. Maybe it
> > is a leftover? I tried to remove the assert and by doing that
> > my paravirtualized device was able to stop using the virtio-net
> > header.
>
>
> Hi:
>
> If  I understand this correctly, your PV device is API compatible with
> TAP? Then you may just adding code to call TUNSETIFF without IFF_VNET_HDR?
>
> Thanks
>
>
> >
> > Vincenzo Maffione (1):
> >    net: tap: allow net frontends to un-negotiate virtio-net header
> >
> >   net/tap.c | 1 -
> >   1 file changed, 1 deletion(-)
> >
>


-- 
Vincenzo

Re: [Qemu-devel] [PATCH v1 0/1] Allow TAP to unnegotiate virtio-net header

[Jason Wang]

On 2019/2/18 下午7:53, Vincenzo Maffione wrote:
> Hi Jason,
>    Thanks for the quick reply.
>
> My PV device (to be open sourced soon) uses the QEMU net backend interface,
> in a way similar to virtio-net.
> For example it uses qemu_set_offload(), qemu_has_vnet_hdr_len(),
> qemu_using_vnet_hdr(), qemu_send_packet(), etc.
> This means that the device itself does not know which net backend is using,
> of course.
> In addition to TAP, also the netmap backend supports the virtio-net header,
> and so the PV device will work with both.
> Regarding the virtio-net header, the netmap backend is more flexible
> because you can change the virtio-net header length as you wish (choosing
> between 0, 10 and 12 bytes).


Interesting, actually, there's case that vnet header is not used even 
for virtio-net, e.g for the case of XDP withouth csum support. If we 
don't pass vnet header in this case, we may get even higher PPS.


>
> My problem cannot be solved by making sure that TAPs are opened without
> IFF_VNET_HDR. What I want instead
> is to dynamically change the length of the virtio-net header that the
> backend accepts, switching between 12 bytes (virtio 1.0 header) and 0 (no
> header). By "dynamically" I mean while the guest (and the device) is
> running. This is possible and safe if we get rid of that assert().
> The virtio-net device does not need that flexibility (once you enable the
> vtnet header, you never disable that again), so I guess that's why you have
> that assert().
> My PV device can be reconfigured on the fly in such a way it may or not may
> be aware of the virtio-net header, and as a consequence it may need to call
> qemu_using_vnet_hdr() with true or false (multiple times).
>
> Does it make sense?


Yes, it is. But I think maybe it's better to send this patch with your 
new PV device implementation.

Thanks


>
> Cheers,
>    Vincenzo
>
>
> Il giorno lun 18 feb 2019 alle ore 05:19 Jason Wang <jasowang@redhat.com>
> ha scritto:
>
>> On 2019/2/16 上午1:46, Vincenzo Maffione wrote:
>>> Hi,
>>>     I was doing experiments with a custom paravirtualized net device,
>>> and I ran into a limitation of the TAP backend. I see from the kernel
>>> code that it is not possible to set the TAP virtio-net header
>>> length to something different from 10 or 12, which means that it
>>> is not possible to set it to 0. That's fine.
>>> The QEMU implementation of the TAP backend already supports
>>> the case where virtio-net header is set to 10 or 12 in kernel,
>>> but the emulated/paravirtualized net device is not using that:
>>> the TAP backend will just strip/prepend a zeroed header in this case
>>> (e.g., see the if statement in tap_receive_iov()).
>>> However, the tap_using_vnet_hdr() has an assert() that prevents
>>> this situation to happen, and I don't understand why. Maybe it
>>> is a leftover? I tried to remove the assert and by doing that
>>> my paravirtualized device was able to stop using the virtio-net
>>> header.
>>
>> Hi:
>>
>> If  I understand this correctly, your PV device is API compatible with
>> TAP? Then you may just adding code to call TUNSETIFF without IFF_VNET_HDR?
>>
>> Thanks
>>
>>
>>> Vincenzo Maffione (1):
>>>     net: tap: allow net frontends to un-negotiate virtio-net header
>>>
>>>    net/tap.c | 1 -
>>>    1 file changed, 1 deletion(-)
>>>
>

Re: [Qemu-devel] [PATCH v1 0/1] Allow TAP to unnegotiate virtio-net header

[Vincenzo Maffione]

Il giorno mar 19 feb 2019 alle ore 07:48 Jason Wang <jasowang@redhat.com>
ha scritto:

>
> On 2019/2/18 下午7:53, Vincenzo Maffione wrote:
> > Hi Jason,
> >    Thanks for the quick reply.
> >
> > My PV device (to be open sourced soon) uses the QEMU net backend
> interface,
> > in a way similar to virtio-net.
> > For example it uses qemu_set_offload(), qemu_has_vnet_hdr_len(),
> > qemu_using_vnet_hdr(), qemu_send_packet(), etc.
> > This means that the device itself does not know which net backend is
> using,
> > of course.
> > In addition to TAP, also the netmap backend supports the virtio-net
> header,
> > and so the PV device will work with both.
> > Regarding the virtio-net header, the netmap backend is more flexible
> > because you can change the virtio-net header length as you wish (choosing
> > between 0, 10 and 12 bytes).
>
>
> Interesting, actually, there's case that vnet header is not used even
> for virtio-net, e.g for the case of XDP withouth csum support. If we
> don't pass vnet header in this case, we may get even higher PPS.
>

I guess in this case we simply pass a zeroed header. According to my
experience I think it's unlikely that PPS are affected by having 12
additional bytes to copy around, as long as no processing is done on the
header. I may be wrong of course.

>
>
> >
> > My problem cannot be solved by making sure that TAPs are opened without
> > IFF_VNET_HDR. What I want instead
> > is to dynamically change the length of the virtio-net header that the
> > backend accepts, switching between 12 bytes (virtio 1.0 header) and 0 (no
> > header). By "dynamically" I mean while the guest (and the device) is
> > running. This is possible and safe if we get rid of that assert().
> > The virtio-net device does not need that flexibility (once you enable the
> > vtnet header, you never disable that again), so I guess that's why you
> have
> > that assert().
> > My PV device can be reconfigured on the fly in such a way it may or not
> may
> > be aware of the virtio-net header, and as a consequence it may need to
> call
> > qemu_using_vnet_hdr() with true or false (multiple times).
> >
> > Does it make sense?
>
>
> Yes, it is. But I think maybe it's better to send this patch with your
> new PV device implementation.
>
> Sounds good, thanks.

Cheers,
  Vincenzo


> Thanks
>
>
> >
> > Cheers,
> >    Vincenzo
> >
> >
> > Il giorno lun 18 feb 2019 alle ore 05:19 Jason Wang <jasowang@redhat.com
> >
> > ha scritto:
> >
> >> On 2019/2/16 上午1:46, Vincenzo Maffione wrote:
> >>> Hi,
> >>>     I was doing experiments with a custom paravirtualized net device,
> >>> and I ran into a limitation of the TAP backend. I see from the kernel
> >>> code that it is not possible to set the TAP virtio-net header
> >>> length to something different from 10 or 12, which means that it
> >>> is not possible to set it to 0. That's fine.
> >>> The QEMU implementation of the TAP backend already supports
> >>> the case where virtio-net header is set to 10 or 12 in kernel,
> >>> but the emulated/paravirtualized net device is not using that:
> >>> the TAP backend will just strip/prepend a zeroed header in this case
> >>> (e.g., see the if statement in tap_receive_iov()).
> >>> However, the tap_using_vnet_hdr() has an assert() that prevents
> >>> this situation to happen, and I don't understand why. Maybe it
> >>> is a leftover? I tried to remove the assert and by doing that
> >>> my paravirtualized device was able to stop using the virtio-net
> >>> header.
> >>
> >> Hi:
> >>
> >> If  I understand this correctly, your PV device is API compatible with
> >> TAP? Then you may just adding code to call TUNSETIFF without
> IFF_VNET_HDR?
> >>
> >> Thanks
> >>
> >>
> >>> Vincenzo Maffione (1):
> >>>     net: tap: allow net frontends to un-negotiate virtio-net header
> >>>
> >>>    net/tap.c | 1 -
> >>>    1 file changed, 1 deletion(-)
> >>>
> >
>


-- 
Vincenzo

Re: [Qemu-devel] [PATCH v1 0/1] Allow TAP to unnegotiate virtio-net header

[Jason Wang]

On 2019/2/19 下午4:28, Vincenzo Maffione wrote:
> Il giorno mar 19 feb 2019 alle ore 07:48 Jason Wang <jasowang@redhat.com>
> ha scritto:
>
>> On 2019/2/18 下午7:53, Vincenzo Maffione wrote:
>>> Hi Jason,
>>>     Thanks for the quick reply.
>>>
>>> My PV device (to be open sourced soon) uses the QEMU net backend
>> interface,
>>> in a way similar to virtio-net.
>>> For example it uses qemu_set_offload(), qemu_has_vnet_hdr_len(),
>>> qemu_using_vnet_hdr(), qemu_send_packet(), etc.
>>> This means that the device itself does not know which net backend is
>> using,
>>> of course.
>>> In addition to TAP, also the netmap backend supports the virtio-net
>> header,
>>> and so the PV device will work with both.
>>> Regarding the virtio-net header, the netmap backend is more flexible
>>> because you can change the virtio-net header length as you wish (choosing
>>> between 0, 10 and 12 bytes).
>>
>> Interesting, actually, there's case that vnet header is not used even
>> for virtio-net, e.g for the case of XDP withouth csum support. If we
>> don't pass vnet header in this case, we may get even higher PPS.
>>
> I guess in this case we simply pass a zeroed header. According to my
> experience I think it's unlikely that PPS are affected by having 12
> additional bytes to copy around, as long as no processing is done on the
> header. I may be wrong of course.


The problem is device doesn't know whether it was zeroed, so it need to 
examine some fields.

Thanks


>
>>
>>> My problem cannot be solved by making sure that TAPs are opened without
>>> IFF_VNET_HDR. What I want instead
>>> is to dynamically change the length of the virtio-net header that the
>>> backend accepts, switching between 12 bytes (virtio 1.0 header) and 0 (no
>>> header). By "dynamically" I mean while the guest (and the device) is
>>> running. This is possible and safe if we get rid of that assert().
>>> The virtio-net device does not need that flexibility (once you enable the
>>> vtnet header, you never disable that again), so I guess that's why you
>> have
>>> that assert().
>>> My PV device can be reconfigured on the fly in such a way it may or not
>> may
>>> be aware of the virtio-net header, and as a consequence it may need to
>> call
>>> qemu_using_vnet_hdr() with true or false (multiple times).
>>>
>>> Does it make sense?
>>
>> Yes, it is. But I think maybe it's better to send this patch with your
>> new PV device implementation.
>>
>> Sounds good, thanks.
> Cheers,
>    Vincenzo
>
>
>> Thanks
>>
>>
>>> Cheers,
>>>     Vincenzo
>>>
>>>
>>> Il giorno lun 18 feb 2019 alle ore 05:19 Jason Wang <jasowang@redhat.com
>>>
>>> ha scritto:
>>>
>>>> On 2019/2/16 上午1:46, Vincenzo Maffione wrote:
>>>>> Hi,
>>>>>      I was doing experiments with a custom paravirtualized net device,
>>>>> and I ran into a limitation of the TAP backend. I see from the kernel
>>>>> code that it is not possible to set the TAP virtio-net header
>>>>> length to something different from 10 or 12, which means that it
>>>>> is not possible to set it to 0. That's fine.
>>>>> The QEMU implementation of the TAP backend already supports
>>>>> the case where virtio-net header is set to 10 or 12 in kernel,
>>>>> but the emulated/paravirtualized net device is not using that:
>>>>> the TAP backend will just strip/prepend a zeroed header in this case
>>>>> (e.g., see the if statement in tap_receive_iov()).
>>>>> However, the tap_using_vnet_hdr() has an assert() that prevents
>>>>> this situation to happen, and I don't understand why. Maybe it
>>>>> is a leftover? I tried to remove the assert and by doing that
>>>>> my paravirtualized device was able to stop using the virtio-net
>>>>> header.
>>>> Hi:
>>>>
>>>> If  I understand this correctly, your PV device is API compatible with
>>>> TAP? Then you may just adding code to call TUNSETIFF without
>> IFF_VNET_HDR?
>>>> Thanks
>>>>
>>>>
>>>>> Vincenzo Maffione (1):
>>>>>      net: tap: allow net frontends to un-negotiate virtio-net header
>>>>>
>>>>>     net/tap.c | 1 -
>>>>>     1 file changed, 1 deletion(-)
>>>>>
>