Re: [PATCH v8 07/24] x86: refactor psr: implement get value flow.

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-03-08 08:35:53, Jan Beulich wrote:
> >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote:
> > @@ -260,9 +263,22 @@ static bool l3_cat_get_feat_info(const struct feat_node *feat,
> >      return true;
> >  }
> >  
> > +static bool l3_cat_get_val(const struct feat_node *feat, unsigned int cos,
> > +                           enum cbm_type type, uint64_t *val)
> > +{
> > +    if ( cos > feat->info.l3_cat_info.cos_max )
> > +        /* Use default value. */
> > +        cos = 0;
> > +
> > +    *val = feat->cos_reg_val[cos];
> > +
> > +    return true;
> 
> This one never failing I wonder whether the same will apply to the
> later ones. If so, there's little point in returning a boolean here, but
> instead you could return the value instead of using indirection.
> 
I have modified this function to 'void' in next version.

> >  static void __init parse_psr_bool(char *s, char *value, char *feature,
> > @@ -482,12 +498,14 @@ static struct psr_socket_info *get_socket_info(unsigned int socket)
> >      return socket_info + socket;
> >  }
> >  
> > -int psr_get_info(unsigned int socket, enum cbm_type type,
> > -                 uint32_t data[], unsigned int array_len)
> > +static int psr_get(unsigned int socket, enum cbm_type type,
> 
> The immediately preceding patch introduced thus function, and
> now you're changing its name. Please give it the intended final
> name right away.
> 
The name is not changed. You can see below lines. Here is just to implement
a helper function which is called by 'psr_get_info'.

> > +                   uint32_t data[], unsigned int array_len,
> > +                   struct domain *d, uint64_t *val)
> 
> const struct domain *, but I'm not even sure that's an appropriate
> parameter here:
> 
> > @@ -498,6 +516,15 @@ int psr_get_info(unsigned int socket, enum cbm_type type,
> >          if ( feat->feature != feat_type )
> >              continue;
> >  
> > +        if ( d )
> > +        {
> > +            cos = d->arch.psr_cos_ids[socket];
> 
> You could equally well pass a more constrained pointer, like
> psr_cos_ids[] on its own. But of course much depends on whether
> you'll need d for other things in this function in later patches.
> 
Ok, thanks! Will consider the parameter.

> > +            if ( feat->ops.get_val(feat, cos, type, val) )
> > +                return 0;
> > +            else
> > +                break;
> > +        }
> > +
> >          if ( feat->ops.get_feat_info(feat, data, array_len) )
> >              return 0;
> >          else
> 
> Looking at the context here - is it really a good idea to overload the
> function in this way, rather than creating a second one? Your
> only complicating the live of the callers, as can be seen e.g. ...
> 
These codes were separated into two functions before, 'psr_get_info' and
'psr_get_val'. But there are some common codes. So, Konrad suggested me
to create a helper function to reduce redundant codes.

> > @@ -507,10 +534,16 @@ int psr_get_info(unsigned int socket, enum cbm_type type,
> >      return -ENOENT;
> >  }
> >  
> > -int psr_get_l3_cbm(struct domain *d, unsigned int socket,
> > -                   uint64_t *cbm, enum cbm_type type)
> > +int psr_get_info(unsigned int socket, enum cbm_type type,
> > +                 uint32_t data[], unsigned int array_len)
> > +{
> > +    return psr_get(socket, type, data, array_len, NULL, NULL);
> 
> ... here and ...
> 
> > +}
> > +
> > +int psr_get_val(struct domain *d, unsigned int socket,
> > +                uint64_t *val, enum cbm_type type)
> >  {
> > -    return 0;
> > +    return psr_get(socket, type, NULL, 0, d, val);
> >  }
> 
> ... here (it is a bad sign that both pass NULL on either side).
> 
Yes, these things look not good. But to keep a common helper I have to pass
all necessary parameters into it. What is your suggestion?

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

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