[libvirt] [PATCHv5 10/19] util: Introduce default monitor

[John Ferlan]

On 10/9/18 6:30 AM, Wang Huaqiang wrote:
> In resctrl file system, more than one monitoring groups
> could be created within one allocation group, along with
> the creation of allocation group, a monitoring group is
> created at the same, which monitors the resource
> utilization information of whole allocation group.
> 
> This patch is introducing the concept of default monitor,
> which represents the particular monitoring group that
> created along with the creation of allocation group.
> 
> Default monitor shares the common 'vcpu' list with the
> allocation.
> 
> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virresctrl.c    | 23 +++++++++++++++++++++++
>  src/util/virresctrl.h    |  2 ++
>  3 files changed, 26 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index c93d19f..4b22ed4 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2689,6 +2689,7 @@ virResctrlMonitorGetID;
>  virResctrlMonitorNew;
>  virResctrlMonitorRemove;
>  virResctrlMonitorSetCacheLevel;
> +virResctrlMonitorSetDefault;
>  virResctrlMonitorSetID;
>  
>  
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index fca1f6f..41e8d48 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -340,6 +340,13 @@ struct _virResctrlAlloc {
>   * bandwidth technology (MBM), as well as the CAT and MBA, are all orthogonal
>   * features. The monitor will be created under the scope of default allocation
>   * if no CAT or MBA supported in the system.
> + *
> + * In resctrl file sytem, more than one monitoring groups could be created'

In the resctrl file system, more than one monitoring group could...

> + * within one allocation group, along with the creation of allocation group,

s/group, along/group. Along/

> + * a monitoring group is created at the same, which monitors the resource
> + * utilization information of whole allocation group.

Reword - it's a bit redundant

> + * A virResctrlMonitor with @default_monitor marked as 'true' is representing
> + * the monitoring group created along with the creation of allocation group.

Well I'm a bit lost, but let's see what happens. I'm not sure what
you're trying to delineate here. There is the creation of an
resctrl->alloc when a <monitor> is found by no <cachetune> is found.
Thus, we create an empty <cachetune> (one with no <cache> elements). To
me that's a default environment.

I assume a similar paradigm could exist if there was or wasn't a
<memorytune> element...

>   */
>  struct _virResctrlMonitor {
>      virObject parent;
> @@ -355,6 +362,8 @@ struct _virResctrlMonitor {
>      /* libvirt-generated path in /sys/fs/resctrl for this particular
>       * monitor */
>      char *path;
> +    /* Boolean flag for default monitor */
> +    bool default_monitor;
>      /* The cache 'level', special for cache monitor */
>      unsigned int cache_level;
>  };
> @@ -2499,6 +2508,13 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
>          return -1;
>      }
>  
> +    if (monitor->default_monitor) {
> +        if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0)

See check below ... at this point monitor->alloc could be NULL and won't
make this STRDUP very happy.

> +            return -1;
> +
> +        return 0;
> +    }
> +
>      if (monitor->alloc)
>          alloc_path = monitor->alloc->path;
>      else
> @@ -2739,3 +2755,10 @@ virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor,
>      return virResctrlMonitorGetStatistic(monitor, "llc_occupancy",
>                                           nbank, bankids, bankcaches);
>  }
> +
> +
> +void
> +virResctrlMonitorSetDefault(virResctrlMonitorPtr monitor)
> +{
> +    monitor->default_monitor = true;
> +}


I really don't see what the value of this is.  Looking later on it seems
there's some sort of check that the vcpus desired for the monitor match
that of some cachetune entry and you then set default.

To me that could happen multiple times, e.g.:

   <cachetune vcpus='0-1' ... />
   <cachetune vcpus='2-3' ... />

and

    <monitor vcpus='0-1' .../>
    <monitor vcpus='2-3' .../>

so, then it would seem there would be two defaults.

Is all this being done to save a few steps in
virResctrlMonitorDeterminePath? If so, then I see no value. It only adds
confusion.

John

> diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
> index 6137fee..371df8a 100644
> --- a/src/util/virresctrl.h
> +++ b/src/util/virresctrl.h
> @@ -228,4 +228,6 @@ virResctrlMonitorGetCacheOccupancy(virResctrlMonitorPtr monitor,
>                                     size_t *nbank,
>                                     unsigned int **bankids,
>                                     unsigned int **bankcaches);
> +void
> +virResctrlMonitorSetDefault(virResctrlMonitorPtr monitor);
>  #endif /*  __VIR_RESCTRL_H__ */
>