[libvirt] [PATCH v4 5/6] qemu: Network stats support for VF Representor

[John Ferlan]

On 06/28/2018 09:22 AM, 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 enabled on host for
> pci SR-IOV device according to switchdev model.
> 
> Signed-off-by: Jai Singh Rana <JaiSingh.Rana at cavium.com>
> ---
>  src/qemu/qemu_driver.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 4e94b4f095..167807704b 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -67,6 +67,7 @@
>  #include "virbuffer.h"
>  #include "virhostcpu.h"
>  #include "virhostmem.h"
> +#include "virnetdevhostdev.h"
>  #include "virnetdevtap.h"
>  #include "virnetdevopenvswitch.h"
>  #include "capabilities.h"
> @@ -11258,6 +11259,10 @@ 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, !swapped) < 0)
> +            goto cleanup;
Based on feedback from 4/6, this is the patch where the virDomainNetFind
can then use the True argument since the HOSTDEV part is handled in
order to do the determination for hostdev using SR-IOV w/ VF Representor
in switchdev mode.

>      } else {
>          if (virNetDevTapInterfaceStats(net->ifname, stats,
>                                         !virDomainNetTypeSharesHostView(net)) < 0)
> @@ -19935,6 +19940,7 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>  {
>      size_t i;
>      struct _virDomainInterfaceStats tmp;
> +    char *vf_ifname = NULL;
>      int ret = -1;
>  
>      if (!virDomainObjIsActive(dom))
> @@ -19947,21 +19953,41 @@ 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]);

This is where I'd point out that a fatal error from *GetVFRIfName
probably isn't a good thing to continue from. Still I see other code in
there that resets last error and continues, so perhaps *this* is where
that happens rather than in patch3... That is let/force the caller
decide what it wants to ignore rather than having the lower in the stack
method handle that logic.

> +            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, vf_ifname);
> +        else
> +            QEMU_ADD_NAME_PARAM(record, maxparams,
> +                                "net", "name", i, net->ifname);
>  
>          if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
>              if (virNetDevOpenvswitchInterfaceStats(net->ifname, &tmp) < 0) {
>                  virResetLastError();
>                  continue;
>              }
> +        } else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
> +            int rc;
> +            bool swapped = virDomainNetTypeSharesHostView(net);
> +
> +            rc = virNetdevHostdevVFRIfStats(vf_ifname, &tmp, !swapped);
> +            VIR_FREE(vf_ifname);
> +            if (rc < 0) {
> +                virResetLastError();


hmm.. well this shows that this caller doesn't care, so maybe fatal
failures aren't so bad.

Although since virNetdevHostdevVFRIfStats is just a wrapper to
virNetDevGetProcNetdevStats, then as I pointed out earlier the direct
call to virNetdevHostdevNetSysfsPath is fine.


John

> +                continue;
> +            }
>          } else {
>              if (virNetDevTapInterfaceStats(net->ifname, &tmp,
>                                             !virDomainNetTypeSharesHostView(net)) < 0) {
>