[libvirt] [PATCH v3 2/2] qemu: conf: Network stats support for VF Representors

[John Ferlan]

On 04/04/2018 12:29 PM, Jai Singh Rana wrote:
> In case of pci SR-IOV device with interface_type as 'hostdev', return
> network stats if it has a VF Representor interface on host for
> pci SR-IOV device according to switchdev model.
> ---
> v3 includes changes based on v2's[1] feedback and suggestions. Includes
> fix for hostdev to net mapping in a given domain.
> [1] https://www.redhat.com/archives/libvir-list/2018-February/msg00563.html
> 
>  docs/news.xml          |  9 +++++++++
>  src/conf/domain_conf.c | 15 +++++++++++++++
>  src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++++++----
>  3 files changed, 53 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/news.xml b/docs/news.xml
> index 87f52e83e..04c18495f 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -47,6 +47,15 @@
>            supported. In fact, kernel has been supporting this since 4.10.
>          </description>
>        </change>
> +      <change>
> +        <summary>
> +          qemu: Support interface network stats for VF Representors
> +        </summary>
> +        <description>
> +          Interface network stats are supported now for SR-IOV device(hostdev)
> +          if this interface has VF representor on host in switchdev mode.
> +        </description>
> +      </change>
>      </section>
>      <section title="Bug fixes">
>      </section>

This needs to be it's own separate and last patch.

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ef16431aa..50813701c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -56,6 +56,7 @@
>  #include "virsecret.h"
>  #include "virstring.h"
>  #include "virnetdev.h"
> +#include "virnetdevhostdev.h"
>  #include "virnetdevmacvlan.h"
>  #include "virhostdev.h"
>  #include "virmdev.h"
> @@ -28264,12 +28265,26 @@ virDomainNetFindByName(virDomainDefPtr def,
>                         const char *ifname)
>  {
>      size_t i;
> +    size_t j;
>  
>      for (i = 0; i < def->nnets; i++) {
>          if (STREQ_NULLABLE(ifname, def->nets[i]->ifname))
>              return def->nets[i];
>      }
>  
> +    /* Give a try to hostdev */
> +    for (i = 0; i < def->nhostdevs; i++) {
> +        if (virNetdevHostdevCheckVFRIfName(def->hostdevs[i], ifname)) {

You probably need to gate the above a bit more as a hostdev can be non
network... No sense in calling the very heavy above function for scsi
disks is there?

There's also interesting relationship between nnets and nhostdevs - see
virDomainNetInsert, which is something I think you need to understand
before unnecessarily walking this hostdev's list.

> +            for (j = 0; j < def->nnets; j++) {
> +                if (def->nets[j]->type != VIR_DOMAIN_NET_TYPE_HOSTDEV)
> +                    continue;

We already walked the nnets and what about virDomainNetGetActualType...

Not so sure about this. Laine perhaps understand all these relationships
a bit better than I do.

> +                if (memcmp(def->hostdevs[i], &def->nets[j]->data.hostdev,
> +                           sizeof(virDomainHostdevDef)) == 0)
> +                    return def->nets[j];
> +            }
> +        }
> +    }
> +
>      return NULL;
>  }

If it stays, I think this hunk is separable into it's own patch...

>  
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 5c31dfdd5..f2f9d290b 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -66,6 +66,7 @@
>  #include "virbuffer.h"
>  #include "virhostcpu.h"
>  #include "virhostmem.h"
> +#include "virnetdevhostdev.h"
>  #include "virnetdevtap.h"
>  #include "virnetdevopenvswitch.h"
>  #include "capabilities.h"
> @@ -11156,6 +11157,11 @@ qemuDomainInterfaceStats(virDomainPtr dom,
>      if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
>          if (virNetDevOpenvswitchInterfaceStats(net->ifname, stats) < 0)
>              goto cleanup;
> +    } else if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) {

    bool swapped = virDomainNetTypeSharesHostView(net);

> +        if (virNetdevHostdevVFRIfStats(device, stats,
> +                                       !virDomainNetTypeSharesHostView(net))

                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Prefer to see a local "swapped" boolean used/defined than a function
call within a function call.

> +                                       < 0)

That would at least not leave this naked < 0) on one line

> +            goto cleanup;
>      } else {
>          if (virNetDevTapInterfaceStats(net->ifname, stats,
>                                         !virDomainNetTypeSharesHostView(net)) < 0)
> @@ -19818,6 +19824,7 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>  {
>      size_t i;
>      struct _virDomainInterfaceStats tmp;
> +    char *vf_ifname = NULL;
>      int ret = -1;
>  
>      if (!virDomainObjIsActive(dom))
> @@ -19830,21 +19837,39 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>          virDomainNetDefPtr net = dom->def->nets[i];
>          virDomainNetType actualType;
>  
> -        if (!net->ifname)
> +        actualType = virDomainNetGetActualType(net);
> +
> +        if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
> +            vf_ifname = virNetdevHostdevGetVFRIfName(dom->def->hostdevs[i]);
> +            if (!vf_ifname)
> +                continue;
> +        }
> +        else if (!net->ifname)
>              continue;
>  
>          memset(&tmp, 0, sizeof(tmp));
>  
> -        actualType = virDomainNetGetActualType(net);
>  
> -        QEMU_ADD_NAME_PARAM(record, maxparams,
> -                            "net", "name", i, net->ifname);
> +        if (actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV)
> +            QEMU_ADD_NAME_PARAM(record, maxparams,
> +                                "net", "name", i, net->ifname);
> +        else
> +            QEMU_ADD_NAME_PARAM(record, maxparams,
> +                                "net", "name", i, vf_ifname);
>  
>          if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
>              if (virNetDevOpenvswitchInterfaceStats(net->ifname, &tmp) < 0) {
>                  virResetLastError();
>                  continue;
>              }
> +        } else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
> +            if (virNetdevHostdevVFRIfStats(vf_ifname, &tmp,
> +                                           !virDomainNetTypeSharesHostView(net)) < 0) {
> +                VIR_FREE(vf_ifname);

Same here w/r/t boolean....

> +                virResetLastError();
> +                continue;
> +            }
> +            VIR_FREE(vf_ifname);

consider:

   rc = virNetdevHostdevVFRIfStats();
   VIR_FREE(vf_ifname);
   if (rc < 0) {
      virResetLastError
      continue;
   }
>          } else {
>              if (virNetDevTapInterfaceStats(net->ifname, &tmp,
>                                             !virDomainNetTypeSharesHostView(net)) < 0) {

Then it can be used here too

John
>