[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 13/15] tools: implement new generic get value interface and MBA get value command



On Tue, Sep 05, 2017 at 05:32:35PM +0800, Yi Sun wrote:
> This patch implements generic get value interfaces in libxc and libxl.
> It also refactors the get value flow in xl to make it be suitable for all
> allocation features. Based on that, a new MBA get value command is added in 
> xl.
> 
> Signed-off-by: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>
> Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
> v3:
>     - replace 'libxl_psr_cbm_type' to 'libxl_psr_type' in newly defined
>       interfaces.
>       (suggested by Roger Pau Monné)
> v2:
>     - change 'CAT_INFO'/'MBA_INFO' to 'CAT'/'MBA'. The related structure names
>       are changed too.
>       (suggested by Chao Peng)
> ---
>  tools/libxc/include/xenctrl.h |   7 +-
>  tools/libxc/xc_psr.c          |   9 +-
>  tools/libxl/libxl_psr.c       |  59 +++++++++-----
>  tools/xl/xl.h                 |   1 +
>  tools/xl/xl_cmdtable.c        |   5 ++
>  tools/xl/xl_psr.c             | 185 
> ++++++++++++++++++++++++++++++------------
>  6 files changed, 184 insertions(+), 82 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 63b92d2..eef06be 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2455,6 +2455,7 @@ enum xc_psr_type {
>      XC_PSR_CAT_L3_CBM_CODE = 2,
>      XC_PSR_CAT_L3_CBM_DATA = 3,
>      XC_PSR_CAT_L2_CBM      = 4,
> +    XC_PSR_MBA_THRTL       = 5,
>  };
>  typedef enum xc_psr_type xc_psr_type;
>  
> @@ -2501,9 +2502,9 @@ int xc_psr_cmt_enabled(xc_interface *xch);
>  int xc_psr_cat_set_domain_data(xc_interface *xch, uint32_t domid,
>                                 xc_psr_type type, uint32_t target,
>                                 uint64_t data);
> -int xc_psr_cat_get_domain_data(xc_interface *xch, uint32_t domid,
> -                               xc_psr_type type, uint32_t target,
> -                               uint64_t *data);
> +int xc_psr_get_domain_data(xc_interface *xch, uint32_t domid,
> +                           xc_psr_type type, uint32_t target,
> +                           uint64_t *data);
>  int xc_psr_get_hw_info(xc_interface *xch, uint32_t socket,
>                         xc_psr_feat_type type, xc_psr_hw_info *hw_info);
>  
> diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c
> index 80642a2..2f0eed9 100644
> --- a/tools/libxc/xc_psr.c
> +++ b/tools/libxc/xc_psr.c
> @@ -283,9 +283,9 @@ int xc_psr_cat_set_domain_data(xc_interface *xch, 
> uint32_t domid,
>      return do_domctl(xch, &domctl);
>  }
>  
> -int xc_psr_cat_get_domain_data(xc_interface *xch, uint32_t domid,
> -                               xc_psr_type type, uint32_t target,
> -                               uint64_t *data)
> +int xc_psr_get_domain_data(xc_interface *xch, uint32_t domid,
> +                           xc_psr_type type, uint32_t target,
> +                           uint64_t *data)
>  {
>      int rc;
>      DECLARE_DOMCTL;
> @@ -305,6 +305,9 @@ int xc_psr_cat_get_domain_data(xc_interface *xch, 
> uint32_t domid,
>      case XC_PSR_CAT_L2_CBM:
>          cmd = XEN_DOMCTL_PSR_ALLOC_GET_L2_CBM;
>          break;
> +    case XC_PSR_MBA_THRTL:
> +        cmd = XEN_DOMCTL_PSR_ALLOC_GET_MBA_THRTL;
> +        break;
>      default:
>          errno = EINVAL;
>          return -1;
> diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> index c8d2921..78d5bc5 100644
> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
> @@ -71,16 +71,30 @@ static void libxl__psr_cmt_log_err_msg(libxl__gc *gc, int 
> err)
>      LOGE(ERROR, "%s", msg);
>  }
>  
> -static void libxl__psr_cat_log_err_msg(libxl__gc *gc, int err)
> +static void libxl__psr_alloc_log_err_msg(libxl__gc *gc,
> +                                         int err,
> +                                         libxl_psr_type type)
>  {
> +    /*
> +     * Index is 'libxl_psr_type' so we set two 'CDP' to correspond to
> +     * DATA and CODE.
> +     */
> +    const char * const feat_name[6] = {

The explicit '6' is not needed.

> +        "UNKNOWN",
> +        "L3 CAT",
> +        "CDP",
> +        "CDP",
> +        "L2 CAT",
> +        "MBA",

I'm not sure whether you want to use designated initializers here, in
case someone decides to change the order of the xc_psr_type enum or
the order here. ie:

feat_name[] = {
    [XC_PSR_CAT_L3_CBM] = "L3 CAT",
    [XC_PSR_CAT_L3_CBM_CODE..XC_PSR_CAT_L3_CBM_DATA] = "CDP",
    ...
}

> +    };
>      char *msg;
>  
>      switch (err) {
>      case ENODEV:
> -        msg = "CAT is not supported in this system";
> +        msg = "is not supported in this system";
>          break;
>      case ENOENT:
> -        msg = "CAT is not enabled on the socket";
> +        msg = "is not enabled on the socket";
>          break;
>      case EOVERFLOW:
>          msg = "no free COS available";
> @@ -106,7 +120,7 @@ static void libxl__psr_cat_log_err_msg(libxl__gc *gc, int 
> err)
>          return;
>      }
>  
> -    LOGE(ERROR, "%s", msg);
> +    LOGE(ERROR, "%s: %s", feat_name[type], msg);

I don't think you should use LOGE here, but rather LOG. LOGE should be
used when errno is set, which I don't think is the case here.

>  }
>  
>  static int libxl__pick_socket_cpu(libxl__gc *gc, uint32_t socketid)
> @@ -303,10 +317,10 @@ out:
>      return rc;
>  }
>  
> -static inline xc_psr_type libxl__psr_cbm_type_to_libxc_psr_type(
> -    libxl_psr_cbm_type type)
> +static inline xc_psr_type libxl__psr_type_to_libxc_psr_type(
> +    libxl_psr_type type)
>  {
> -    BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_type));
> +    BUILD_BUG_ON(sizeof(libxl_psr_type) != sizeof(xc_psr_type));
>      return (xc_psr_type)type;
>  }
>  
> @@ -330,10 +344,10 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t 
> domid,
>          if (socketid >= nr_sockets)
>              break;
>  
> -        xc_type = libxl__psr_cbm_type_to_libxc_psr_type(type);
> +        xc_type = libxl__psr_type_to_libxc_psr_type(type);
>          if (xc_psr_cat_set_domain_data(ctx->xch, domid, xc_type,
>                                         socketid, cbm)) {
> -            libxl__psr_cat_log_err_msg(gc, errno);
> +            libxl__psr_alloc_log_err_msg(gc, errno, type);
>              rc = ERROR_FAIL;
>          }
>      }
> @@ -347,18 +361,7 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid,
>                            libxl_psr_cbm_type type, uint32_t target,
>                            uint64_t *cbm_r)
>  {
> -    GC_INIT(ctx);
> -    int rc = 0;
> -    xc_psr_type xc_type = libxl__psr_cbm_type_to_libxc_psr_type(type);
> -
> -    if (xc_psr_cat_get_domain_data(ctx->xch, domid, xc_type,
> -                                   target, cbm_r)) {
> -        libxl__psr_cat_log_err_msg(gc, errno);
> -        rc = ERROR_FAIL;
> -    }
> -
> -    GC_FREE;
> -    return rc;
> +    return libxl_psr_get_val(ctx, domid, type, target, cbm_r);
>  }

You could even move this to libxl.h as a static function IMHO.

>  
>  static xc_psr_feat_type libxl__feat_type_to_libxc_feat_type(
> @@ -462,7 +465,19 @@ int libxl_psr_get_val(libxl_ctx *ctx, uint32_t domid,
>                        libxl_psr_type type, unsigned int target,
>                        uint64_t *val)
>  {
> -    return ERROR_FAIL;
> +    GC_INIT(ctx);
> +    int rc = 0;
> +
> +    xc_psr_type xc_type = libxl__psr_type_to_libxc_psr_type(type);
> +
> +    if (xc_psr_get_domain_data(ctx->xch, domid, xc_type,
> +                               target, val)) {
> +        libxl__psr_alloc_log_err_msg(gc, errno, type);
> +        rc = ERROR_FAIL;
> +    }
> +
> +    GC_FREE;
> +    return rc;
>  }
>  
>  static int libxl__xc_hw_info_to_libxl_hw_info(
> diff --git a/tools/xl/xl.h b/tools/xl/xl.h
> index 8d7b957..3389df9 100644
> --- a/tools/xl/xl.h
> +++ b/tools/xl/xl.h
> @@ -204,6 +204,7 @@ int main_psr_cmt_detach(int argc, char **argv);
>  int main_psr_cmt_show(int argc, char **argv);
>  int main_psr_cat_cbm_set(int argc, char **argv);
>  int main_psr_cat_show(int argc, char **argv);
> +int main_psr_mba_show(int argc, char **argv);
>  #endif
>  int main_qemu_monitor_command(int argc, char **argv);
>  
> diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> index a01245d..cdc2349 100644
> --- a/tools/xl/xl_cmdtable.c
> +++ b/tools/xl/xl_cmdtable.c
> @@ -560,6 +560,11 @@ struct cmd_spec cmd_table[] = {
>        "[options] <Domain>",
>        "-l <level>        Specify the cache level to process, otherwise L3 
> cache is processed\n"
>      },
> +    { "psr-mba-show",
> +      &main_psr_mba_show, 0, 1,
> +      "Show Memory Bandwidth Allocation information",
> +      "<Domain>",
> +    },
>  #endif
>      { "usbctrl-attach",
>        &main_usbctrl_attach, 0, 1,
> diff --git a/tools/xl/xl_psr.c b/tools/xl/xl_psr.c
> index 40269b4..46b7788 100644
> --- a/tools/xl/xl_psr.c
> +++ b/tools/xl/xl_psr.c
> @@ -327,19 +327,27 @@ out:
>      return rc;
>  }
>  
> -static void psr_cat_print_one_domain_cbm_type(uint32_t domid, uint32_t 
> socketid,
> -                                              libxl_psr_cbm_type type)
> +static void psr_print_one_domain_val_type(uint32_t domid,
> +                                          libxl_psr_hw_info *info,
> +                                          libxl_psr_type type)
>  {
> -    uint64_t cbm;
> +    uint64_t val;
>  
> -    if (!libxl_psr_cat_get_cbm(ctx, domid, type, socketid, &cbm))
> -        printf("%#16"PRIx64, cbm);
> +    if (!libxl_psr_get_val(ctx, domid, type, info->id, &val))
> +    {
> +        if (type == LIBXL_PSR_CBM_TYPE_MBA_THRTL && info->u.mba.linear)
> +            printf("%16"PRIu64, val);
> +        else
> +            printf("%#16"PRIx64, val);
> +    }
>      else
>          printf("%16s", "error");
>  }
>  
> -static void psr_cat_print_one_domain_cbm(uint32_t domid, uint32_t socketid,
> -                                         bool cdp_enabled, unsigned int lvl)
> +static void psr_print_one_domain_val(uint32_t domid,
> +                                     libxl_psr_hw_info *info,
> +                                     libxl_psr_feat_type type,
> +                                     unsigned int lvl)
>  {
>      char *domain_name;
>  
> @@ -347,106 +355,154 @@ static void psr_cat_print_one_domain_cbm(uint32_t 
> domid, uint32_t socketid,
>      printf("%5d%25s", domid, domain_name);
>      free(domain_name);
>  
> -    switch (lvl) {
> -    case 3:
> -        if (!cdp_enabled) {
> -            psr_cat_print_one_domain_cbm_type(domid, socketid,
> +    switch (type) {
> +    case LIBXL_PSR_FEAT_TYPE_CAT:
> +        switch (lvl) {
> +        case 3:
> +            if (!info->u.cat.cdp_enabled) {
> +                psr_print_one_domain_val_type(domid, info,
>                                                LIBXL_PSR_CBM_TYPE_L3_CBM);
> -        } else {
> -            psr_cat_print_one_domain_cbm_type(domid, socketid,
> +            } else {
> +                psr_print_one_domain_val_type(domid, info,
>                                                
> LIBXL_PSR_CBM_TYPE_L3_CBM_CODE);
> -            psr_cat_print_one_domain_cbm_type(domid, socketid,
> +                psr_print_one_domain_val_type(domid, info,
>                                                
> LIBXL_PSR_CBM_TYPE_L3_CBM_DATA);
> -        }
> -        break;
> -    case 2:
> -        psr_cat_print_one_domain_cbm_type(domid, socketid,
> +            }
> +            break;
> +
> +        case 2:
> +            psr_print_one_domain_val_type(domid, info,
>                                            LIBXL_PSR_CBM_TYPE_L2_CBM);
> +            break;
> +
> +        default:
> +            printf("Input lvl %d is wrong!", lvl);
> +        }
>          break;
> -    default:
> -        printf("Input lvl %d is wrong!", lvl);
> +
> +    case LIBXL_PSR_FEAT_TYPE_MBA:
> +        psr_print_one_domain_val_type(domid, info,
> +                                      LIBXL_PSR_CBM_TYPE_MBA_THRTL);
>          break;
>      }
>  
>      printf("\n");
>  }
>  
> -static int psr_cat_print_domain_cbm(uint32_t domid, uint32_t socketid,
> -                                    bool cdp_enabled, unsigned int lvl)
> +static int psr_print_domain_val(uint32_t domid,
> +                                libxl_psr_hw_info *info,
> +                                libxl_psr_feat_type type,
> +                                unsigned int lvl)
>  {
>      int i, nr_domains;
>      libxl_dominfo *list;
>  
>      if (domid != INVALID_DOMID) {
> -        psr_cat_print_one_domain_cbm(domid, socketid, cdp_enabled, lvl);
> +        psr_print_one_domain_val(domid, info, type, lvl);
>          return 0;
>      }
>  
>      if (!(list = libxl_list_domain(ctx, &nr_domains))) {
> -        fprintf(stderr, "Failed to get domain list for cbm display\n");
> -        return -1;
> +        fprintf(stderr, "Failed to get domain list for value display\n");
> +        return EXIT_FAILURE;
>      }
>  
>      for (i = 0; i < nr_domains; i++)
> -        psr_cat_print_one_domain_cbm(list[i].domid, socketid, cdp_enabled, 
> lvl);
> +        psr_print_one_domain_val(list[i].domid, info, type, lvl);
>      libxl_dominfo_list_free(list, nr_domains);
>  
>      return 0;
>  }
>  
> -static int psr_cat_print_socket(uint32_t domid, libxl_psr_cat_info *info,
> -                                unsigned int lvl)
> +static int psr_print_socket(uint32_t domid,
> +                            libxl_psr_hw_info *info,
> +                            libxl_psr_feat_type type,
> +                            unsigned int lvl)
>  {
> -    int rc;
> -    uint32_t l3_cache_size;
> -
>      printf("%-16s: %u\n", "Socket ID", info->id);
>  
> -    /* So far, CMT only supports L3 cache. */
> -    if (lvl == 3) {
> -        rc = libxl_psr_cmt_get_l3_cache_size(ctx, info->id, &l3_cache_size);
> -        if (rc) {
> -            fprintf(stderr, "Failed to get l3 cache size for socket:%d\n",
> -                    info->id);
> -            return -1;
> +    switch (type) {
> +    case LIBXL_PSR_FEAT_TYPE_CAT:
> +    {
> +        int rc;
> +        uint32_t l3_cache_size;
> +
> +        /* So far, CMT only supports L3 cache. */
> +        if (lvl == 3) {

Shouldn't you print some kind of error message if lvl != 3? Or is it
expected that this function will be called with lvl != 3 and it should
be ignored?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.