[libvirt] [PATCH 4/9] util: Add MBA capability information query to resctrl

[John Ferlan]

On 07/18/2018 03:57 AM, bing.niu at intel.com wrote:
> From: Bing Niu <bing.niu at intel.com>
> 
> Introducing virResctrlInfoMemBW for the information memory bandwidth
> allocation information. Those information is used for memory
> bandwidth allocating.

Last sentence is duplicitous.

> 
> Signed-off-by: Bing Niu <bing.niu at intel.com>
> ---
>  src/util/virresctrl.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 104 insertions(+)
> 
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index 1515de2..06e2702 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -80,6 +80,9 @@ typedef virResctrlInfoPerType *virResctrlInfoPerTypePtr;
>  typedef struct _virResctrlInfoPerLevel virResctrlInfoPerLevel;
>  typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr;
>  
> +typedef struct _virResctrlInfoMemBW virResctrlInfoMemBW;
> +typedef virResctrlInfoMemBW *virResctrlInfoMemBWPtr;
> +
>  typedef struct _virResctrlAllocPerType virResctrlAllocPerType;
>  typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr;
>  
> @@ -116,11 +119,31 @@ struct _virResctrlInfoPerLevel {
>      virResctrlInfoPerTypePtr *types;
>  };
>  
> +/* Information about memory bandwidth allocation */
> +struct _virResctrlInfoMemBW {
> +    /* minimum memory bandwidth allowed */
> +    unsigned int min_bandwidth;
> +    /* bandwidth granularity */
> +    unsigned int bandwidth_granularity;
> +    /* Maximum number of simultaneous allocations */
> +    unsigned int max_allocation;
> +    /* level number of last level cache */
> +    unsigned int last_level_cache;
> +    /* max id of last level cache, this is used to track
> +     * how many last level cache available in host system,
> +     * the number of memory bandwidth allocation controller
> +     * is identical with last level cache.
> +     */

The closing */ can be on the previous line.

> +    unsigned int max_id;
> +};
> +
>  struct _virResctrlInfo {
>      virObject parent;
>  
>      virResctrlInfoPerLevelPtr *levels;
>      size_t nlevels;
> +
> +    virResctrlInfoMemBWPtr membw_info;
>  };
>  
>  
> @@ -146,6 +169,7 @@ virResctrlInfoDispose(void *obj)
>          VIR_FREE(level);
>      }
>  
> +    VIR_FREE(resctrl->membw_info);
>      VIR_FREE(resctrl->levels);
>  }
>  
> @@ -442,6 +466,65 @@ virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, DIR *dirp)
>  
>  
>  static int
> +virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl)
> +{
> +    int ret = -1;
> +    int rv = -1;
> +    virResctrlInfoMemBWPtr i_membw = NULL;
> +
> +    /* query memory bandwidth allocation info */
> +    if (VIR_ALLOC(i_membw) < 0)
> +        goto cleanup;
> +    rv = virFileReadValueUint(&i_membw->bandwidth_granularity,
> +                              SYSFS_RESCTRL_PATH "/info/MB/bandwidth_gran");
> +    if (rv == -2) {
> +        /* The file doesn't exist, so it's unusable for us,
> +         *  properly mba unsupported */

s/ properly/probably/

??  Is that what you meant?

s/mba/memory bandwidth/

right?

> +        VIR_INFO("The path '" SYSFS_RESCTRL_PATH "/info/MB/bandwidth_gran'"
> +                 "does not exist");
> +        ret = 0;
> +        goto cleanup;
> +    } else if (rv < 0) {
> +        /* Other failures are fatal, so just quit */
> +        goto cleanup;
> +    }
> +
> +    rv = virFileReadValueUint(&i_membw->min_bandwidth,
> +                              SYSFS_RESCTRL_PATH "/info/MB/min_bandwidth");
> +    if (rv == -2) {
> +        /* If the previous file exists, so should this one.  Hence -2 is

s/  Hence/ Hence/

It's a raging debate at times on this list whether to go with 1 space or
2. I think 1 space usually wins, but I use 2 many times as well. I'll
adjust this to 1 space though ;-)

> +         * fatal in this case (errors out in next condition) - the
> +         * kernel interface might've changed too much or something else is
> +         * wrong. */

"kernel" can fit on previous line meaning "wrong." can fit on previous
line.  Also, no need for blank line between here and virReportError

> +
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Cannot get min bandwidth from resctrl memory info"));
> +    }
> +    if (rv < 0)
> +        goto cleanup;
> +
> +    rv = virFileReadValueUint(&i_membw->max_allocation,
> +                              SYSFS_RESCTRL_PATH "/info/MB/num_closids");
> +    if (rv == -2) {
> +        /* If the previous file exists, so should this one.  Hence -2 is
> +         * fatal in this case as well (errors out in next condition) - the
> +         * kernel interface might've changed too much or something else is
> +         * wrong. */

I'd just note /* Similar reasoning as min_bandwidth above */

> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Cannot get max allocation from resctrl memory info"));
> +    }
> +    if (rv < 0)
> +        goto cleanup;
> +
> +    VIR_STEAL_PTR(resctrl->membw_info, i_membw);
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(i_membw);
> +    return ret;
> +}
> +
> +
> +static int
>  virResctrlGetInfo(virResctrlInfoPtr resctrl)
>  {
>      DIR *dirp = NULL;
> @@ -451,6 +534,10 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
>      if (ret <= 0)
>          goto cleanup;
>  
> +    ret = virResctrlGetMemoryBandwidthInfo(resctrl);
> +    if (ret < 0)
> +        goto cleanup;
> +
>      ret = virResctrlGetCacheInfo(resctrl, dirp);
>      if (ret < 0)
>          goto cleanup;
> @@ -492,6 +579,9 @@ virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl)
>      if (!resctrl)
>          return true;
>  
> +    if (resctrl->membw_info)
> +        return false;
> +
>      for (i = 0; i < resctrl->nlevels; i++) {
>          virResctrlInfoPerLevelPtr i_level = resctrl->levels[i];
>  
> @@ -517,12 +607,26 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl,
>  {
>      virResctrlInfoPerLevelPtr i_level = NULL;
>      virResctrlInfoPerTypePtr i_type = NULL;
> +    virResctrlInfoMemBWPtr membw_info = NULL;

This can be local to the if...

>      size_t i = 0;
>      int ret = -1;
>  
>      if (virResctrlInfoIsEmpty(resctrl))
>          return 0;
>  
> +    /* Let's take the opportunity to update the number of last level
> +     * cache. This number of memory bandwidth controller is same with
> +     * last level cache */
> +    if (resctrl->membw_info) {

           virResctrlInfoMemBWPtr membw_info = resctrl->membw_info;

> +        membw_info = resctrl->membw_info;
> +        if (level > membw_info->last_level_cache) {
> +            membw_info->last_level_cache = level;
> +            membw_info->max_id = 0;
> +        } else if (membw_info->last_level_cache == level) {
> +            membw_info->max_id++;
> +        }
> +    }
> +

This last hunk should be it's own patch. I can split it out for you.
The rest of the patch introduces the concept, does the query, and saves
the data in the "right place".

This hunk says, hey we have some membw_info data that can change our
perception of reality, so we need to adjust. Although nothing yet has
set last_level_cache or max_id...  I'll assume that's comeing.

I'll also make membw_info "local" to the if {}.

The only hmm... I have is this last hunk, I've already forgotten what we
discussed the previous series. But my hmm is related to why is it here
and what impact can it (eventually) have if the values are changed in
this method while perhaps also being changed in a different method.  I'm
sure I'll learn more of that as I move forward.

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

BTW: I'm stopping here for the evening, I'll work through the rest
hopefully tomorrow depending on interruptions.

>      if (level >= resctrl->nlevels)
>          return 0;
>  
>