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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

> +     * 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.

> +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.

>  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.

> +        if ( exceeds_cos_max(val, array_len, info, old_cos) )

Also please fold the two if()-s.

> +            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).

> +    /* 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.

Jan

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