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

[Yi Sun]

On 17-09-20 09:57:59, Roger Pau Monn� wrote:
> On Wed, Sep 20, 2017 at 02:46:24PM +0800, Yi Sun wrote:
> > On 17-09-19 12:02:08, Roger Pau Monn� wrote:
> > > On Tue, Sep 05, 2017 at 05:32:35PM +0800, Yi Sun wrote:
> > > > +};
> > > >  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.
> > > 
> > But two places to call libxl__psr_cat_log_err_msg all set errno in 'xc_'
> > functions.
> 
> But you already translate the error into a custom message ('msg' in the
> above context), and the error variable in this case is 'err' not
> 'errno', so LOGE is not appropriate here IMHO.
> 
Ok.

[...]

> > > > 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
> > > > -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, 
> > > > _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?
> > > 
> > We only get cache size for level 3 cache. So, if input is lvl=2, we print
> > nothing.
> 
> My question would be, is it expected that this function is called with
> lvl == 2 as part of the normal operation with valid input values?
> 
Yes, lvl == 2 is a normal operation.

> Roger.

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

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

[Roger Pau Monné]

On Wed, Sep 20, 2017 at 02:46:24PM +0800, Yi Sun wrote:
> On 17-09-19 12:02:08, Roger Pau Monn� wrote:
> > On Tue, Sep 05, 2017 at 05:32:35PM +0800, Yi Sun wrote:
> > > +};
> > >  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.
> > 
> But two places to call libxl__psr_cat_log_err_msg all set errno in 'xc_'
> functions.

But you already translate the error into a custom message ('msg' in the
above context), and the error variable in this case is 'err' not
'errno', so LOGE is not appropriate here IMHO.

> > > @@ -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.
> > 
> Yes. But I prefer to keep it here with other interfaces together. Is that
> acceptable to you?

OK, as you wish.

> > > 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
> > > -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, 
> > > _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?
> > 
> We only get cache size for level 3 cache. So, if input is lvl=2, we print
> nothing.

My question would be, is it expected that this function is called with
lvl == 2 as part of the normal operation with valid input values?

Roger.

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

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

[Yi Sun]

On 17-09-19 12:02:08, Roger Pau Monn� wrote:
> On Tue, Sep 05, 2017 at 05:32:35PM +0800, Yi Sun wrote:
> > 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",
> ...
> }
> 
Got it. Thanks! One correction, the index is 'libxl_psr_type'.

> > +};
> >  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.
> 
But two places to call libxl__psr_cat_log_err_msg all set errno in 'xc_'
functions.

> > @@ -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.
> 
Yes. But I prefer to keep it here with other interfaces together. Is that
acceptable to you?

> > 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
> > -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, 
> > _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?
> 
We only get cache size for level 3 cache. So, if input is lvl=2, we print
nothing.

> Thanks, Roger.

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

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

[Roger Pau Monné]

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 
> Acked-by: Wei Liu 
> ---
> 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, );
>  }
>  
> -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",