qemu: Set noqueue qdisc for TAP devices

[PATCH 0/2] qemu: Set noqueue qdisc for TAP devices

Posted by Michal Privoznik 5 years, 4 months ago

See 2/2 for detailed explanation.
Long story short - we can squeeze more bandwidth from TAP devices we
create for domains.

Michal Prívozník (2):
  virnetdev: Introduce virNetDevSetRootQDisc()
  qemu: Set noqueue qdisc for TAP devices

 src/libvirt_private.syms |  1 +
 src/qemu/qemu_command.c  |  2 ++
 src/qemu/qemu_domain.c   | 36 +++++++++++++++++++++++++++++++
 src/qemu/qemu_domain.h   |  4 ++++
 src/qemu/qemu_hotplug.c  |  2 ++
 src/util/virnetdev.c     | 46 ++++++++++++++++++++++++++++++++++++++++
 src/util/virnetdev.h     |  3 +++
 7 files changed, 94 insertions(+)

-- 
2.26.2

Re: [PATCH 0/2] qemu: Set noqueue qdisc for TAP devices

Posted by Daniel Henrique Barboza 5 years, 3 months ago

On 10/8/20 3:10 PM, Michal Privoznik wrote:
> See 2/2 for detailed explanation.
> Long story short - we can squeeze more bandwidth from TAP devices we
> create for domains.
> 
> Michal Prívozník (2):
>    virnetdev: Introduce virNetDevSetRootQDisc()
>    qemu: Set noqueue qdisc for TAP devices

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Have you tried it out with older kernels? Reading the bug I understood
that 4.2 and older (2015 kernels) does not have qdisc support and that you
can, for example, test for NETIF_F_LLTX of the ETHTOOL_GFEATURES ioctl to
verify it.

I consider this to be a 'nice to have' that can be added in a follow up
patch though, if applicable. By the way, do we have any documentation about
"the latest Libvirt release will not care about N+ years old kernel/QEMU"?

Thanks,

DHB

> 
>   src/libvirt_private.syms |  1 +
>   src/qemu/qemu_command.c  |  2 ++
>   src/qemu/qemu_domain.c   | 36 +++++++++++++++++++++++++++++++
>   src/qemu/qemu_domain.h   |  4 ++++
>   src/qemu/qemu_hotplug.c  |  2 ++
>   src/util/virnetdev.c     | 46 ++++++++++++++++++++++++++++++++++++++++
>   src/util/virnetdev.h     |  3 +++
>   7 files changed, 94 insertions(+)
>

Re: [PATCH 0/2] qemu: Set noqueue qdisc for TAP devices

Posted by Michal Privoznik 5 years, 3 months ago

On 10/13/20 3:09 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 10/8/20 3:10 PM, Michal Privoznik wrote:
>> See 2/2 for detailed explanation.
>> Long story short - we can squeeze more bandwidth from TAP devices we
>> create for domains.
>>
>> Michal Prívozník (2):
>>    virnetdev: Introduce virNetDevSetRootQDisc()
>>    qemu: Set noqueue qdisc for TAP devices
> 
> 
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 

Thanks. Just before I merged these I realized that it will be a good 
idea to mock virNetDevSetRootQDisc() so that we don't run tc from the 
test suite. But the diff is really trivial:

diff --git i/src/util/virnetdev.h w/src/util/virnetdev.h
index 82943b8e08..dfef49938f 100644
--- i/src/util/virnetdev.h
+++ w/src/util/virnetdev.h
@@ -313,6 +313,7 @@ int virNetDevRunEthernetScript(const char *ifname, 
const char *script)
      G_GNUC_NO_INLINE;

  int virNetDevSetRootQDisc(const char *ifname,
-                          const char *qdisc);
+                          const char *qdisc)
+    G_GNUC_NO_INLINE;

  G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetDevRxFilter, virNetDevRxFilterFree);
diff --git i/tests/qemuxml2argvmock.c w/tests/qemuxml2argvmock.c
index 9bf4357b66..b9322f4f2a 100644
--- i/tests/qemuxml2argvmock.c
+++ w/tests/qemuxml2argvmock.c
@@ -286,3 +286,11 @@ qemuBuildTPMOpenBackendFDs(const char *tpmdev 
G_GNUC_UNUSED,
      *cancelfd = 1731;
      return 0;
  }
+
+
+int
+virNetDevSetRootQDisc(const char *ifname G_GNUC_UNUSED,
+                      const char *qdisc G_GNUC_UNUSED)
+{
+    return 0;
+}


Hopefully, you are okay if I squash this to 2/2.

> 
> Have you tried it out with older kernels? Reading the bug I understood
> that 4.2 and older (2015 kernels) does not have qdisc support and that you
> can, for example, test for NETIF_F_LLTX of the ETHTOOL_GFEATURES ioctl to
> verify it.
> 

I didn't, but I had this on mind when writing patches and I made the 
whole thing best effort. Note that qemuDomainInterfaceSetDefaultQDisc() 
retval is not checked for => if setting noqueue fails (for whatever 
reason) then worst case scenario an error is printed into logs.

> I consider this to be a 'nice to have' that can be added in a follow up
> patch though, if applicable. By the way, do we have any documentation about
> "the latest Libvirt release will not care about N+ years old kernel/QEMU"?

I'm not sure what you mean. This perhaps?

https://libvirt.org/platforms.html#linux-freebsd-and-macos

Michal

Re: [PATCH 0/2] qemu: Set noqueue qdisc for TAP devices

Posted by Daniel Henrique Barboza 5 years, 3 months ago


On 10/13/20 11:06 AM, Michal Privoznik wrote:
> On 10/13/20 3:09 PM, Daniel Henrique Barboza wrote:
>>
>>
>> On 10/8/20 3:10 PM, Michal Privoznik wrote:
>>> See 2/2 for detailed explanation.
>>> Long story short - we can squeeze more bandwidth from TAP devices we
>>> create for domains.
>>>
>>> Michal Prívozník (2):
>>>    virnetdev: Introduce virNetDevSetRootQDisc()
>>>    qemu: Set noqueue qdisc for TAP devices
>>
>>
>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>
> 
> Thanks. Just before I merged these I realized that it will be a good idea to mock virNetDevSetRootQDisc() so that we don't run tc from the test suite. But the diff is really trivial:
> 
> diff --git i/src/util/virnetdev.h w/src/util/virnetdev.h
> index 82943b8e08..dfef49938f 100644
> --- i/src/util/virnetdev.h
> +++ w/src/util/virnetdev.h
> @@ -313,6 +313,7 @@ int virNetDevRunEthernetScript(const char *ifname, const char *script)
>       G_GNUC_NO_INLINE;
> 
>   int virNetDevSetRootQDisc(const char *ifname,
> -                          const char *qdisc);
> +                          const char *qdisc)
> +    G_GNUC_NO_INLINE;
> 
>   G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetDevRxFilter, virNetDevRxFilterFree);
> diff --git i/tests/qemuxml2argvmock.c w/tests/qemuxml2argvmock.c
> index 9bf4357b66..b9322f4f2a 100644
> --- i/tests/qemuxml2argvmock.c
> +++ w/tests/qemuxml2argvmock.c
> @@ -286,3 +286,11 @@ qemuBuildTPMOpenBackendFDs(const char *tpmdev G_GNUC_UNUSED,
>       *cancelfd = 1731;
>       return 0;
>   }
> +
> +
> +int
> +virNetDevSetRootQDisc(const char *ifname G_GNUC_UNUSED,
> +                      const char *qdisc G_GNUC_UNUSED)
> +{
> +    return 0;
> +}
> 
> 
> Hopefully, you are okay if I squash this to 2/2.


Yep, go ahead.


> 
>>
>> Have you tried it out with older kernels? Reading the bug I understood
>> that 4.2 and older (2015 kernels) does not have qdisc support and that you
>> can, for example, test for NETIF_F_LLTX of the ETHTOOL_GFEATURES ioctl to
>> verify it.
>>
> 
> I didn't, but I had this on mind when writing patches and I made the whole thing best effort. Note that qemuDomainInterfaceSetDefaultQDisc() retval is not checked for => if setting noqueue fails (for whatever reason) then worst case scenario an error is printed into logs.
> 
>> I consider this to be a 'nice to have' that can be added in a follow up
>> patch though, if applicable. By the way, do we have any documentation about
>> "the latest Libvirt release will not care about N+ years old kernel/QEMU"?
> 
> I'm not sure what you mean. This perhaps?
> 
> https://libvirt.org/platforms.html#linux-freebsd-and-macos


Thanks, this is what I was looking for.



DHB


> 
> Michal
>