Re: [Xen-devel] [PATCH v4 11/24] x86: refactor psr: set value: implement cos id allocation flow.

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 17-01-10 08:08:19, Jan Beulich wrote:
> >>> On 14.12.16 at 05:07, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -169,6 +169,23 @@ struct feat_ops {
> >       */
> >      int (*compare_val)(const uint64_t val[], const struct feat_node *feat,
> >                          unsigned int cos, bool *found);
> > +    /*
> > +     * exceeds_cos_max is used to check if the input cos id exceeds the
> > +     * feature's cos_max and if the input value is not the default one.
> > +     * Even if the associated cos exceeds the cos_max, HW can work with 
> > default
> > +     * value. That is the reason we need check if input value is default 
> > one.
> > +     * If both criteria are fulfilled, that means the input exceeds the
> > +     * range.
> 
> Isn't this last sentence the wrong way round?
> 
Sorry.

> > +     * The return value of the function means the number of the value array
> > +     * entries to skip or error.
> > +     * 1 - one entry in value array.
> > +     * 2 - two entries in value array, e.g. CDP uses two entries.
> > +     * 0 - error, exceed cos_max and the input value is not default.
> > +     */
> > +    unsigned int (*exceeds_cos_max)(const uint64_t val[],
> > +                                    const struct feat_node *feat,
> > +                                    unsigned int cos);
> 
> IIRC I did recommend "exceeds" in the name during earlier review,
> but also iirc the semantics of the call were different back then.
> Please try to name functions such that they describe themselves
> in at least a minimalistic ways. My main issue with this naming is
> that the function returning non-zero (i.e. true in C meaning within
> conditionals) means "no" here rather than "yes". fits_cos_max()
> would therefore be a possibly better fit.
> 
Thanks for the suggestion!

> > +static bool exceeds_cos_max(const uint64_t *val,
> > +                            uint32_t array_len,
> > +                            const struct psr_socket_info *info,
> > +                            unsigned int cos)
> > +{
> > +    unsigned int ret;
> > +    const uint64_t *val_tmp = val;
> > +    const struct feat_node *feat_tmp;
> > +
> > +    list_for_each_entry(feat_tmp, &info->feat_list, list)
> > +    {
> > +        ret = feat_tmp->ops.exceeds_cos_max(val_tmp, feat_tmp, cos);
> > +        if ( !ret )
> > +            return false;
> > +
> > +        val_tmp += ret;
> > +        if ( val_tmp - val > array_len )
> > +            return false;
> > +    }
> > +
> > +    return true;
> > +}
> 
> Similarly here - name and return value don't fit together; I think
> you want to invert the return values or (along the lines above)
> rename the function.
> 
Will rename the callback function to make it accurate. Thanks!

> >  static int alloc_new_cos(const struct psr_socket_info *info,
> >                           const uint64_t *val, uint32_t array_len,
> >                           unsigned int old_cos,
> >                           enum cbm_type type)
> >  {
> > -    return 0;
> > +    unsigned int cos;
> > +    unsigned int cos_max = 0;
> > +    const struct feat_node *feat_tmp;
> > +    const unsigned int *ref = info->cos_ref;
> > +
> > +    /*
> > +     * cos_max is the one of the feature which is being set.
> > +     */
> > +    list_for_each_entry(feat_tmp, &info->feat_list, list)
> > +    {
> > +        cos_max = feat_tmp->ops.get_cos_max_from_type(feat_tmp, type);
> > +        if ( cos_max > 0 )
> > +            break;
> > +    }
> > +
> > +    if ( !cos_max )
> > +        return -ENOENT;
> > +
> > +    /*
> > +     * If old cos is referred only by the domain, then use it. And, we 
> > cannot
> > +     * use id 0 because it stores the default values.
> > +     */
> > +    if ( ref[old_cos] == 1 && old_cos )
> 
> Please swap both sides of && - cheaper checks should come first if
> possible.
> 
Sure, thanks!

> > +        if ( exceeds_cos_max(val, array_len, info, old_cos) )
> 
> Also please fold the two if()-s.
> 
Ok, thanks!

> > +            return old_cos;
> 
> And then, as indicated before, this part is not really an allocation,
> but a re-use, so would likely better move into the caller (or the
> function's name should be adjusted).
> 
Prefer to change function name to 'pick_avail_cos'.

> > +    /* Find an unused one other than cos0. */
> > +    for ( cos = 1; cos <= cos_max; cos++ )
> > +        /*
> > +         * ref is 0 means this COS is not used by other domain and
> > +         * can be used for current setting.
> > +         */
> > +        if ( !ref[cos] )
> > +        {
> > +            if ( !exceeds_cos_max(val, array_len, info, cos) )
> > +                return -ENOENT;
> > +
> > +            return cos;
> > +        }
> 
> While a comment is just white space, this looks suspicious without
> another pair of braces around the for() body.
> 
Sure.

> Jan

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