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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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, 
> > > &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?
> > 
> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel