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

2015-09-09 Thread Jan Beulich 
>>> On 09.09.15 at 10:10,  wrote:
> On Wed, Sep 09, 2015 at 03:32:11PM +0800, Chao Peng wrote:
>> On Wed, Sep 09, 2015 at 01:16:47PM +0800, He Chen wrote:
>> > @@ -304,14 +305,41 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t 
>> > domid,
>> >  goto out;
>> >  }
>> >  
>> > +rc = libxl_psr_cat_get_l3_info(ctx, , _sockets);
>> > +if (rc) {
>> > +LOGE(ERROR, "Failed to get cat info");
>> > +goto out;
>> > +}
>> > +
>> > +if (!info->cdp_enabled &&
>> > +   (type == LIBXL_PSR_CBM_TYPE_L3_CODE ||
>> > +type == LIBXL_PSR_CBM_TYPE_L3_DATA))
>> > +{
>> > +LOGE(ERROR, "Unable to set Code/Data CBM with CDP disabled");
>> > +rc = EINVAL;
>> > +free(info);
>> > +goto out;
>> > +}
>> > +
>> >  libxl_for_each_set_bit(socketid, *target_map) {
>> >  if (socketid >= nr_sockets)
>> >  break;
>> > -if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, 
>> > cbm)) {
>> > +if (info->cdp_enabled && type == LIBXL_PSR_CBM_TYPE_L3_CBM)
>> > +{
>> > +if(xc_psr_cat_set_domain_data(ctx->xch, domid,
>> > +   LIBXL_PSR_CBM_TYPE_L3_CODE, socketid, cbm) ||
>> > +   xc_psr_cat_set_domain_data(ctx->xch, domid,
>> > +   LIBXL_PSR_CBM_TYPE_L3_DATA, socketid, cbm))
>> > +{
>> > +libxl__psr_cat_log_err_msg(gc, errno);
>> > +rc = ERROR_FAIL;
>> > +}
>> 
>> Will you merge the two if's?
> 
> Surely.

Surely not you mean, or else how will this ...

>> > +} else if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, 
>> > socketid, cbm)) {

... work?

Looking at this I'm surprised though that consumers is required to do
two calls (and know whether CDP is enabled) when they want to set
things up without caring for the code/data distinction. I think this
should be taken care of in the hypervisor.

Jan


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

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

2015-09-09 Thread Chao Peng 
On Wed, Sep 09, 2015 at 02:37:36AM -0600, Jan Beulich wrote:
> >>> On 09.09.15 at 10:10,  wrote:
> > On Wed, Sep 09, 2015 at 03:32:11PM +0800, Chao Peng wrote:
> >> On Wed, Sep 09, 2015 at 01:16:47PM +0800, He Chen wrote:
> >> > @@ -304,14 +305,41 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t 
> >> > domid,
> >> >  goto out;
> >> >  }
> >> >  
> >> > +rc = libxl_psr_cat_get_l3_info(ctx, , _sockets);
> >> > +if (rc) {
> >> > +LOGE(ERROR, "Failed to get cat info");
> >> > +goto out;
> >> > +}
> >> > +
> >> > +if (!info->cdp_enabled &&
> >> > +   (type == LIBXL_PSR_CBM_TYPE_L3_CODE ||
> >> > +type == LIBXL_PSR_CBM_TYPE_L3_DATA))
> >> > +{
> >> > +LOGE(ERROR, "Unable to set Code/Data CBM with CDP 
> >> > disabled");
> >> > +rc = EINVAL;
> >> > +free(info);
> >> > +goto out;
> >> > +}
> >> > +
> >> >  libxl_for_each_set_bit(socketid, *target_map) {
> >> >  if (socketid >= nr_sockets)
> >> >  break;
> >> > -if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, 
> >> > cbm)) {
> >> > +if (info->cdp_enabled && type == LIBXL_PSR_CBM_TYPE_L3_CBM)
> >> > +{
> >> > +if(xc_psr_cat_set_domain_data(ctx->xch, domid,
> >> > +   LIBXL_PSR_CBM_TYPE_L3_CODE, socketid, cbm) ||
> >> > +   xc_psr_cat_set_domain_data(ctx->xch, domid,
> >> > +   LIBXL_PSR_CBM_TYPE_L3_DATA, socketid, cbm))
> >> > +{
> >> > +libxl__psr_cat_log_err_msg(gc, errno);
> >> > +rc = ERROR_FAIL;
> >> > +}
> >> 
> >> Will you merge the two if's?
> > 
> > Surely.
> 
> Surely not you mean, or else how will this ...
> 
> >> > +} else if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, 
> >> > socketid, cbm)) {
> 
> ... work?

Indeed. Then just ignore it.

> 
> Looking at this I'm surprised though that consumers is required to do
> two calls (and know whether CDP is enabled) when they want to set
> things up without caring for the code/data distinction. I think this
> should be taken care of in the hypervisor.

Agreed, hypervisor should take care of it for this case.

Chao


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

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

2015-09-09 Thread Chao Peng 
On Wed, Sep 09, 2015 at 01:16:47PM +0800, He Chen wrote:
> This is the xl/xc changes to support Intel Code/Data Prioritization.
> CAT xl commands to set/get CBMs are extended to support CDP.
> 
> Signed-off-by: He Chen 
> ---
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 5f9047c..e4eb4df 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -796,6 +796,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
> libxl_mac *src);
>   * If this is defined, the Cache Allocation Technology feature is supported.
>   */
>  #define LIBXL_HAVE_PSR_CAT 1
> +
> +/*
> + * LIBXL_HAVE_PSR_CDP
> + *
> + * If this is defined, the Code/Data Prioritization feature is supported.
> + */
> +#define LIBXL_HAVE_PSR_CDP 1
>  #endif
>  
>  /*
> @@ -1729,6 +1736,9 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, 
> libxl_psr_cat_info **info,
>  void libxl_psr_cat_info_list_free(libxl_psr_cat_info *list, int nr);
>  #endif
>  
> +#ifdef LIBXL_HAVE_PSR_CDP
> +#endif
> +

Why this? There are several of them in this patch.

>  /* misc */
>  
>  /* Each of these sets or clears the flag according to whether the
> diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> index 3378239..26939a5 100644
> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
> @@ -297,6 +297,7 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
>  GC_INIT(ctx);
>  int rc;
>  int socketid, nr_sockets;
> +libxl_psr_cat_info *info;
>  
>  rc = libxl__count_physical_sockets(gc, _sockets);
>  if (rc) {
> @@ -304,14 +305,41 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t 
> domid,
>  goto out;
>  }
>  
> +rc = libxl_psr_cat_get_l3_info(ctx, , _sockets);
> +if (rc) {
> +LOGE(ERROR, "Failed to get cat info");
> +goto out;
> +}
> +
> +if (!info->cdp_enabled &&
> +   (type == LIBXL_PSR_CBM_TYPE_L3_CODE ||
> +type == LIBXL_PSR_CBM_TYPE_L3_DATA))
> +{
> +LOGE(ERROR, "Unable to set Code/Data CBM with CDP disabled");
> +rc = EINVAL;
> +free(info);
> +goto out;
> +}
> +
>  libxl_for_each_set_bit(socketid, *target_map) {
>  if (socketid >= nr_sockets)
>  break;
> -if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, 
> cbm)) {
> +if (info->cdp_enabled && type == LIBXL_PSR_CBM_TYPE_L3_CBM)
> +{
> +if(xc_psr_cat_set_domain_data(ctx->xch, domid,
> +   LIBXL_PSR_CBM_TYPE_L3_CODE, socketid, cbm) ||
> +   xc_psr_cat_set_domain_data(ctx->xch, domid,
> +   LIBXL_PSR_CBM_TYPE_L3_DATA, socketid, cbm))
> +{
> +libxl__psr_cat_log_err_msg(gc, errno);
> +rc = ERROR_FAIL;
> +}

Will you merge the two if's?

Chao
> +} else if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, 
> socketid, cbm)) {
>  libxl__psr_cat_log_err_msg(gc, errno);
>  rc = ERROR_FAIL;
>  }
>  }
> +free(info);
>  
>  out:
>  GC_FREE;

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

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

2015-09-09 Thread He Chen 
On Wed, Sep 09, 2015 at 03:32:11PM +0800, Chao Peng wrote:
> On Wed, Sep 09, 2015 at 01:16:47PM +0800, He Chen wrote:
> > This is the xl/xc changes to support Intel Code/Data Prioritization.
> > CAT xl commands to set/get CBMs are extended to support CDP.
> > 
> > Signed-off-by: He Chen 
> > ---
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 5f9047c..e4eb4df 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -796,6 +796,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
> > libxl_mac *src);
> >   * If this is defined, the Cache Allocation Technology feature is 
> > supported.
> >   */
> >  #define LIBXL_HAVE_PSR_CAT 1
> > +
> > +/*
> > + * LIBXL_HAVE_PSR_CDP
> > + *
> > + * If this is defined, the Code/Data Prioritization feature is supported.
> > + */
> > +#define LIBXL_HAVE_PSR_CDP 1
> >  #endif
> >  
> >  /*
> > @@ -1729,6 +1736,9 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, 
> > libxl_psr_cat_info **info,
> >  void libxl_psr_cat_info_list_free(libxl_psr_cat_info *list, int nr);
> >  #endif
> >  
> > +#ifdef LIBXL_HAVE_PSR_CDP
> > +#endif
> > +
> 
> Why this? There are several of them in this patch.
> 

My carelessness. I would remove the redundant code lines.

> >  /* misc */
> >  
> >  /* Each of these sets or clears the flag according to whether the
> > diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> > index 3378239..26939a5 100644
> > --- a/tools/libxl/libxl_psr.c
> > +++ b/tools/libxl/libxl_psr.c
> > @@ -297,6 +297,7 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t 
> > domid,
> >  GC_INIT(ctx);
> >  int rc;
> >  int socketid, nr_sockets;
> > +libxl_psr_cat_info *info;
> >  
> >  rc = libxl__count_physical_sockets(gc, _sockets);
> >  if (rc) {
> > @@ -304,14 +305,41 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t 
> > domid,
> >  goto out;
> >  }
> >  
> > +rc = libxl_psr_cat_get_l3_info(ctx, , _sockets);
> > +if (rc) {
> > +LOGE(ERROR, "Failed to get cat info");
> > +goto out;
> > +}
> > +
> > +if (!info->cdp_enabled &&
> > +   (type == LIBXL_PSR_CBM_TYPE_L3_CODE ||
> > +type == LIBXL_PSR_CBM_TYPE_L3_DATA))
> > +{
> > +LOGE(ERROR, "Unable to set Code/Data CBM with CDP disabled");
> > +rc = EINVAL;
> > +free(info);
> > +goto out;
> > +}
> > +
> >  libxl_for_each_set_bit(socketid, *target_map) {
> >  if (socketid >= nr_sockets)
> >  break;
> > -if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, 
> > cbm)) {
> > +if (info->cdp_enabled && type == LIBXL_PSR_CBM_TYPE_L3_CBM)
> > +{
> > +if(xc_psr_cat_set_domain_data(ctx->xch, domid,
> > +   LIBXL_PSR_CBM_TYPE_L3_CODE, socketid, cbm) ||
> > +   xc_psr_cat_set_domain_data(ctx->xch, domid,
> > +   LIBXL_PSR_CBM_TYPE_L3_DATA, socketid, cbm))
> > +{
> > +libxl__psr_cat_log_err_msg(gc, errno);
> > +rc = ERROR_FAIL;
> > +}
> 
> Will you merge the two if's?
> 
> Chao

Surely.

> > +} else if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, 
> > socketid, cbm)) {
> >  libxl__psr_cat_log_err_msg(gc, errno);
> >  rc = ERROR_FAIL;
> >  }
> >  }
> > +free(info);
> >  
> >  out:
> >  GC_FREE;
> 
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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