1. Patchew
  2. Libvirt
  3. View series

[libvirt PATCH v2] Adds e1000e/vmxnet3 Vnet_hdr suuport

Patrick Magauran posted 1 patch 3 years, 8 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200808135913.39739-1-patmagauran.j@gmail.com
src/qemu/qemu_interface.c | 15 +++++++++++----
src/qemu/qemu_interface.h |  1 +
2 files changed, 12 insertions(+), 4 deletions(-)
[libvirt PATCH v2] Adds e1000e/vmxnet3 Vnet_hdr suuport
Posted by Patrick Magauran 3 years, 8 months ago 
Changes from Original:
Moved Comparison to qemuInterfaceIsVnetCompatModel in qemu_interface.c per the recommendation of Daniel Berrangé

-----
Libvirt bases its decision about whether to apply the vnet_hdr flag to the tap interface on whether or not the selected model is VirtIO. Originally, VirtIO was the only model to support the vnet_hdr in QEMU; however, the e1000e & vmxnet3 adapters also support it(seemingly from introduction based on commits). This passes the whole packet to the host, reducing emulation overhead and improving performance.

Signed-off-by: Patrick Magauran <patmagauran.j@gmail.com>
---
 src/qemu/qemu_interface.c | 15 +++++++++++----
 src/qemu/qemu_interface.h |  1 +
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index ffec992596..229bb299aa 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -230,6 +230,13 @@ qemuInterfaceStopDevices(virDomainDefPtr def)
     return 0;
 }
 
+bool qemuInterfaceIsVnetCompatModel(const virDomainNetDef *net)
+{
+    return (virDomainNetIsVirtioModel(net) ||
+           net->model == VIR_DOMAIN_NET_MODEL_E1000E ||
+           net->model == VIR_DOMAIN_NET_MODEL_VMXNET3);
+}
+
 
 /**
  * qemuInterfaceDirectConnect:
@@ -255,7 +262,7 @@ qemuInterfaceDirectConnect(virDomainDefPtr def,
     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
     unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;
 
-    if (virDomainNetIsVirtioModel(net))
+    if (qemuInterfaceIsVnetCompatModel(net))
         macvlan_create_flags |= VIR_NETDEV_MACVLAN_VNET_HDR;
 
     if (virNetDevMacVLanCreateWithVPortProfile(net->ifname,
@@ -417,7 +424,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
         }
     }
 
-    if (virDomainNetIsVirtioModel(net))
+    if (qemuInterfaceIsVnetCompatModel(net))
         tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
 
     if (net->managed_tap == VIR_TRISTATE_BOOL_NO) {
@@ -436,7 +443,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
             if (virNetDevMacVLanTapOpen(net->ifname, tapfd, tapfdSize) < 0)
                 goto cleanup;
             if (virNetDevMacVLanTapSetup(tapfd, tapfdSize,
-                                         virDomainNetIsVirtioModel(net)) < 0) {
+                                         qemuInterfaceIsVnetCompatModel(net)) < 0) {
                 goto cleanup;
             }
         } else {
@@ -559,7 +566,7 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
         template_ifname = true;
     }
 
-    if (virDomainNetIsVirtioModel(net))
+    if (qemuInterfaceIsVnetCompatModel(net))
         tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
 
     if (driver->privileged) {
diff --git a/src/qemu/qemu_interface.h b/src/qemu/qemu_interface.h
index 0464b903d7..9e3f61e8e0 100644
--- a/src/qemu/qemu_interface.h
+++ b/src/qemu/qemu_interface.h
@@ -58,3 +58,4 @@ int qemuInterfaceOpenVhostNet(virDomainDefPtr def,
 
 qemuSlirpPtr qemuInterfacePrepareSlirp(virQEMUDriverPtr driver,
                                        virDomainNetDefPtr net);
+bool qemuInterfaceIsVnetCompatModel(const virDomainNetDef *net);
-- 
2.26.2
Re: [libvirt PATCH v2] Adds e1000e/vmxnet3 Vnet_hdr suuport
Posted by Daniel Henrique Barboza 3 years, 7 months ago 
Be mindful of the typo in the commit title: suuport -> support

Just a nit below:

On 8/8/20 10:59 AM, Patrick Magauran wrote:
> Changes from Original:
> Moved Comparison to qemuInterfaceIsVnetCompatModel in qemu_interface.c per the recommendation of Daniel Berrangé
> 
> -----
> Libvirt bases its decision about whether to apply the vnet_hdr flag to the tap interface on whether or not the selected model is VirtIO. Originally, VirtIO was the only model to support the vnet_hdr in QEMU; however, the e1000e & vmxnet3 adapters also support it(seemingly from introduction based on commits). This passes the whole packet to the host, reducing emulation overhead and improving performance.
> 
> Signed-off-by: Patrick Magauran <patmagauran.j@gmail.com>
> ---
>   src/qemu/qemu_interface.c | 15 +++++++++++----
>   src/qemu/qemu_interface.h |  1 +
>   2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
> index ffec992596..229bb299aa 100644
> --- a/src/qemu/qemu_interface.c
> +++ b/src/qemu/qemu_interface.c
> @@ -230,6 +230,13 @@ qemuInterfaceStopDevices(virDomainDefPtr def)
>       return 0;
>   }
>   
> +bool qemuInterfaceIsVnetCompatModel(const virDomainNetDef *net)
> +{
> +    return (virDomainNetIsVirtioModel(net) ||
> +           net->model == VIR_DOMAIN_NET_MODEL_E1000E ||
> +           net->model == VIR_DOMAIN_NET_MODEL_VMXNET3);
> +}


^ indentation here is misaligned by a few spaces:

     return (virDomainNetIsVirtioModel(net) ||
             net->model == VIR_DOMAIN_NET_MODEL_E1000E ||
             net->model == VIR_DOMAIN_NET_MODEL_VMXNET3);


Rest of the code LGTM. I'm CC'ing Laine because I don't have authority to
decide if what you're doing here is enough or more changes are required,
and network is Laine's turf. As far as I understand:


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


> +
>   
>   /**
>    * qemuInterfaceDirectConnect:
> @@ -255,7 +262,7 @@ qemuInterfaceDirectConnect(virDomainDefPtr def,
>       virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>       unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;
>   
> -    if (virDomainNetIsVirtioModel(net))
> +    if (qemuInterfaceIsVnetCompatModel(net))
>           macvlan_create_flags |= VIR_NETDEV_MACVLAN_VNET_HDR;
>   
>       if (virNetDevMacVLanCreateWithVPortProfile(net->ifname,
> @@ -417,7 +424,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
>           }
>       }
>   
> -    if (virDomainNetIsVirtioModel(net))
> +    if (qemuInterfaceIsVnetCompatModel(net))
>           tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
>   
>       if (net->managed_tap == VIR_TRISTATE_BOOL_NO) {
> @@ -436,7 +443,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
>               if (virNetDevMacVLanTapOpen(net->ifname, tapfd, tapfdSize) < 0)
>                   goto cleanup;
>               if (virNetDevMacVLanTapSetup(tapfd, tapfdSize,
> -                                         virDomainNetIsVirtioModel(net)) < 0) {
> +                                         qemuInterfaceIsVnetCompatModel(net)) < 0) {
>                   goto cleanup;
>               }
>           } else {
> @@ -559,7 +566,7 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
>           template_ifname = true;
>       }
>   
> -    if (virDomainNetIsVirtioModel(net))
> +    if (qemuInterfaceIsVnetCompatModel(net))
>           tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
>   
>       if (driver->privileged) {
> diff --git a/src/qemu/qemu_interface.h b/src/qemu/qemu_interface.h
> index 0464b903d7..9e3f61e8e0 100644
> --- a/src/qemu/qemu_interface.h
> +++ b/src/qemu/qemu_interface.h
> @@ -58,3 +58,4 @@ int qemuInterfaceOpenVhostNet(virDomainDefPtr def,
>   
>   qemuSlirpPtr qemuInterfacePrepareSlirp(virQEMUDriverPtr driver,
>                                          virDomainNetDefPtr net);
> +bool qemuInterfaceIsVnetCompatModel(const virDomainNetDef *net);
>
Re: [libvirt PATCH v2] Adds e1000e/vmxnet3 Vnet_hdr suuport
Posted by Laine Stump 3 years, 7 months ago 
On 8/8/20 9:59 AM, Patrick Magauran wrote:
> Changes from Original:
> Moved Comparison to qemuInterfaceIsVnetCompatModel in qemu_interface.c per the recommendation of Daniel Berrangé
>
> -----
> Libvirt bases its decision about whether to apply the vnet_hdr flag to the tap interface on whether or not the selected model is VirtIO. Originally, VirtIO was the only model to support the vnet_hdr in QEMU; however, the e1000e & vmxnet3 adapters also support it(seemingly from introduction based on commits).


The above paragraph is a single line (probably not noticed because your 
editor "flows" it into multiple lines?). Our formatting guidelines say 
that it should be in multiple lines with the margin set at 72 (makes 
displaying "git log" in an 80 char. wide terminal window more palatable).

And keeping in line with the obsessive-compulsive urge to locate exact 
commits: The QEMU upstream commit first adding vmxnet3 support was 
786fd2b0f87, and I verified it does mention vnet_hdr. The QEMU upstream 
commit for e1000e was 6f3fbe4ed06a, and it also checks for vnet_hdr. I'm 
going to add a mention to these in the commit log, for benefit of others 
who suffer from OCD :-)


>   This passes the whole packet to the host, reducing emulation overhead and improving performance.
>
> Signed-off-by: Patrick Magauran <patmagauran.j@gmail.com>
> ---
>   src/qemu/qemu_interface.c | 15 +++++++++++----
>   src/qemu/qemu_interface.h |  1 +
>   2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
> index ffec992596..229bb299aa 100644
> --- a/src/qemu/qemu_interface.c
> +++ b/src/qemu/qemu_interface.c
> @@ -230,6 +230,13 @@ qemuInterfaceStopDevices(virDomainDefPtr def)
>       return 0;
>   }
>   
> +bool qemuInterfaceIsVnetCompatModel(const virDomainNetDef *net)
> +{
> +    return (virDomainNetIsVirtioModel(net) ||
> +           net->model == VIR_DOMAIN_NET_MODEL_E1000E ||
> +           net->model == VIR_DOMAIN_NET_MODEL_VMXNET3);
> +}
> +


Since this function is never called from anywhere else, it should just 
be made static, and not mentioned in qemu_interface.h.


(also, speaking of OCD... Newly added functions should have 2 blank 
lines before and after the function, and the return type should be on a 
separate line from the function name. Don't ask me why, I'm just the 
enforcer :-))


>   
>   /**
>    * qemuInterfaceDirectConnect:
> @@ -255,7 +262,7 @@ qemuInterfaceDirectConnect(virDomainDefPtr def,
>       virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>       unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;
>   
> -    if (virDomainNetIsVirtioModel(net))
> +    if (qemuInterfaceIsVnetCompatModel(net))
>           macvlan_create_flags |= VIR_NETDEV_MACVLAN_VNET_HDR;
>   
>       if (virNetDevMacVLanCreateWithVPortProfile(net->ifname,
> @@ -417,7 +424,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
>           }
>       }
>   
> -    if (virDomainNetIsVirtioModel(net))
> +    if (qemuInterfaceIsVnetCompatModel(net))
>           tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
>   
>       if (net->managed_tap == VIR_TRISTATE_BOOL_NO) {
> @@ -436,7 +443,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
>               if (virNetDevMacVLanTapOpen(net->ifname, tapfd, tapfdSize) < 0)
>                   goto cleanup;
>               if (virNetDevMacVLanTapSetup(tapfd, tapfdSize,
> -                                         virDomainNetIsVirtioModel(net)) < 0) {
> +                                         qemuInterfaceIsVnetCompatModel(net)) < 0) {
>                   goto cleanup;
>               }
>           } else {
> @@ -559,7 +566,7 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
>           template_ifname = true;
>       }
>   
> -    if (virDomainNetIsVirtioModel(net))
> +    if (qemuInterfaceIsVnetCompatModel(net))
>           tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
>   
>       if (driver->privileged) {
> diff --git a/src/qemu/qemu_interface.h b/src/qemu/qemu_interface.h
> index 0464b903d7..9e3f61e8e0 100644
> --- a/src/qemu/qemu_interface.h
> +++ b/src/qemu/qemu_interface.h
> @@ -58,3 +58,4 @@ int qemuInterfaceOpenVhostNet(virDomainDefPtr def,
>   
>   qemuSlirpPtr qemuInterfacePrepareSlirp(virQEMUDriverPtr driver,
>                                          virDomainNetDefPtr net);
> +bool qemuInterfaceIsVnetCompatModel(const virDomainNetDef *net);

Reviewed-by: Laine Stump <laine@redhat.com>


I made the cosmetic changes I noted above (plus those noticed by 
Daniel), and pushed the patch. Thanks for your contribution, and 
congratulations on your first libvirt patch!

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.