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

Re: [Xen-devel] [PATCH 4/5] tools: add tools support for Intel CDP



On Wed, Sep 02, 2015 at 04:28:01PM +0800, He Chen wrote:
> This is the xc/xl changes to support Intel Code/Data Prioritization.
> Two new xl commands are introduced to enable/disable CDP dynamically,
> and CAT xl commands to set/get CBMs are extended to support CDP.
> 
> Signed-off-by: He Chen <he.chen@xxxxxxxxxxxxxxx>
> ---
>  tools/libxc/include/xenctrl.h | 10 ++++--
>  tools/libxc/xc_psr.c          | 42 +++++++++++++++++++++-
>  tools/libxl/libxl.h           | 12 +++++++
>  tools/libxl/libxl_psr.c       | 64 +++++++++++++++++++++++++++++++++-
>  tools/libxl/libxl_types.idl   |  3 ++
>  tools/libxl/xl.h              |  4 +++
>  tools/libxl/xl_cmdimpl.c      | 81 
> +++++++++++++++++++++++++++++++++++++------
>  tools/libxl/xl_cmdtable.c     | 15 ++++++++
>  8 files changed, 217 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index de3c0ad..665e6bd 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2798,7 +2798,10 @@ enum xc_psr_cmt_type {
>  typedef enum xc_psr_cmt_type xc_psr_cmt_type;
>  
>  enum xc_psr_cat_type {
> -    XC_PSR_CAT_L3_CBM = 1,
> +    XC_PSR_CAT_L3_CBM  = 1,
> +    XC_PSR_CAT_L3_CODE = 2,
> +    XC_PSR_CAT_L3_DATA = 3,
> +

Stray blank line.

>  };
>  typedef enum xc_psr_cat_type xc_psr_cat_type;
>  
> @@ -2824,7 +2827,10 @@ int xc_psr_cat_get_domain_data(xc_interface *xch, 
> uint32_t domid,
>                                 xc_psr_cat_type type, uint32_t target,
>                                 uint64_t *data);
>  int xc_psr_cat_get_l3_info(xc_interface *xch, uint32_t socket,
> -                           uint32_t *cos_max, uint32_t *cbm_len);
> +                           uint32_t *cos_max, uint32_t *cbm_len,
> +                           uint8_t *cdp_enabled);

Why does it need to be uint8_t? The name suggests bool should be used.

> +int xc_psr_cat_enable_cdp(xc_interface *xch);
> +int xc_psr_cat_disable_cdp(xc_interface *xch);
>  #endif
>  
>  #endif /* XENCTRL_H */
> diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c
> index d8b3a51..d4ff6f6 100644
> --- a/tools/libxc/xc_psr.c
> +++ b/tools/libxc/xc_psr.c
> @@ -260,6 +260,12 @@ int xc_psr_cat_set_domain_data(xc_interface *xch, 
> uint32_t domid,
>      case XC_PSR_CAT_L3_CBM:
>          cmd = XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM;
>          break;
> +    case XC_PSR_CAT_L3_CODE:
> +        cmd = XEN_DOMCTL_PSR_CAT_OP_SET_L3_CODE;
> +        break;
> +    case XC_PSR_CAT_L3_DATA:
> +        cmd = XEN_DOMCTL_PSR_CAT_OP_SET_L3_DATA;
> +        break;
>      default:
>          errno = EINVAL;
>          return -1;
> @@ -287,6 +293,12 @@ int xc_psr_cat_get_domain_data(xc_interface *xch, 
> uint32_t domid,
>      case XC_PSR_CAT_L3_CBM:
>          cmd = XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM;
>          break;
> +    case XC_PSR_CAT_L3_CODE:
> +        cmd = XEN_DOMCTL_PSR_CAT_OP_GET_L3_CODE;
> +        break;
> +    case XC_PSR_CAT_L3_DATA:
> +        cmd = XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA;
> +        break;
>      default:
>          errno = EINVAL;
>          return -1;
> @@ -306,7 +318,8 @@ int xc_psr_cat_get_domain_data(xc_interface *xch, 
> uint32_t domid,
>  }
>  
[...]
> @@ -304,6 +305,22 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
>          goto out;
>      }
>  
> +    rc = libxl_psr_cat_get_l3_info(ctx, &info, &nr_sockets);
> +    if (rc) {
> +        LOGE(ERROR, "Failed to get cat info");
> +        goto out;
> +    }
> +
> +    if (!info->cdp_enabled) {
> +        if (type == LIBXL_PSR_CBM_TYPE_L3_CODE ||
> +            type == LIBXL_PSR_CBM_TYPE_L3_DATA)

You can merge this if statement to parent.

       if (!info->cdp_enabled &&
          (type == _CODE || type == _DATA))

> +        {
> +            LOGE(ERROR, "Unable to set Code/Data CBM with CDP disabled");
> +            rc = EINVAL;
> +            goto out;
> +        }
> +    }
> +
>      libxl_for_each_set_bit(socketid, *target_map) {
>          if (socketid >= nr_sockets)
>              break;

Need to free memory info points to at the end.

> @@ -352,7 +369,7 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, 
> libxl_psr_cat_info **info,
>  
>      for (i = 0; i < nr_sockets; i++) {
>          if (xc_psr_cat_get_l3_info(ctx->xch, i, &ptr[i].cos_max,
> -                                                &ptr[i].cbm_len)) {
> +                                   &ptr[i].cbm_len, &ptr[i].cdp_enabled)) {
>              libxl__psr_cat_log_err_msg(gc, errno);
>              rc = ERROR_FAIL;
>              free(ptr);
> @@ -376,6 +393,51 @@ void libxl_psr_cat_info_list_free(libxl_psr_cat_info 
> *list, int nr)
>      free(list);
>  }
>  
[...]
> diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
> index 13bccba..e8bb774 100644
> --- a/tools/libxl/xl.h
> +++ b/tools/libxl/xl.h
> @@ -121,6 +121,10 @@ 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);
>  #endif
> +#ifdef LIBXL_HAVE_PSR_CDP
> +int main_psr_cat_cdp_enable(int argc, char **argv);
> +int main_psr_cat_cdp_disable(int argc, char **argv);
> +#endif
>  
>  void help(const char *command);
>  
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index ebbb9a5..825d707 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -8366,6 +8366,15 @@ int main_psr_cmt_show(int argc, char **argv)
>  #endif
>  
>  #ifdef LIBXL_HAVE_PSR_CAT
> +static void psr_cat_print_cdp_status(uint8_t status)
> +{
> +    if (status == 0)
> +        printf("%-16s: Disabled\n", "CDP Status");
> +    else
> +        printf("%-16s: Enabled\n", "CDP Status");
> +}
> +
> +

Extraneous blank line.

In fact I don't think this a function is necessary. There is nothing
wrong with embedding a if statement in psr_cat_hwinfo.

>  static int psr_cat_hwinfo(void)
>  {
>      int rc;
> @@ -8390,6 +8399,7 @@ static int psr_cat_hwinfo(void)
>          }
>          printf("%-16s: %u\n", "Socket ID", socketid);
>          printf("%-16s: %uKB\n", "L3 Cache", l3_cache_size);
> +        psr_cat_print_cdp_status(info->cdp_enabled);
>          printf("%-16s: %u\n", "Maximum COS", info->cos_max);
>          printf("%-16s: %u\n", "CBM length", info->cbm_len);
>          printf("%-16s: %#llx\n", "Default CBM",
> @@ -8401,7 +8411,8 @@ out:
>      return rc;
>  }
>  
[...]
> @@ -8585,6 +8620,32 @@ int main_psr_hwinfo(int argc, char **argv)
>      return ret;
>  }
>  
> +#ifdef LIBXL_HAVE_PSR_CDP
> +int main_psr_cat_cdp_enable(int argc, char **argv)
> +{
> +    int ret;
> +
> +    ret = libxl_psr_cat_enable_cdp(ctx);
> +    if (ret)
> +        return ret;

Please log the error / failure.

> +    printf("CDP is enabled\n");
> +
> +    return 0;
> +}
> +
> +int main_psr_cat_cdp_disable(int argc, char **argv)
> +{
> +    int ret;
> +
> +    ret = libxl_psr_cat_disable_cdp(ctx);
> +    if (ret)
> +        return ret;

Ditto.

Wei.

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


 


Rackspace

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