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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.02.17 at 09:49, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> @@ -397,6 +408,25 @@ static int l3_cat_compare_val(const uint64_t val[],
>      return 0;
>  }
>  
> +static bool l3_cat_fits_cos_max(const uint64_t val[],
> +                                const struct feat_node *feat,
> +                                unsigned int cos)
> +{
> +    uint64_t l3_def_cbm;
> +
> +    l3_def_cbm = (1ull << feat->info.l3_cat_info.cbm_len) - 1;

Seeing this pattern recur I wonder whether this also wouldn't be
something that could be stored once in a generic manner, avoiding
the need for this per-feature callback (cos_max should be common
to all features too - not the values, but the abstract notion - so
perhaps get_cos_max() isn't needed as a callback either).

>  static int pick_avail_cos(const struct psr_socket_info *info,
>                            const uint64_t *val, uint32_t array_len,
>                            unsigned int old_cos,
>                            enum psr_feat_type feat_type)
>  {
> +    unsigned int cos;
> +    unsigned int cos_max = 0;
> +    const struct feat_node *feat;
> +    const unsigned int *ref = info->cos_ref;
> +
> +    /*
> +     * cos_max is the one of the feature which is being set.
> +     */

This is a single line comment.

> +    list_for_each_entry(feat, &info->feat_list, list)
> +    {
> +        if ( feat->feature != feat_type )
> +            continue;
> +
> +        cos_max = feat->ops.get_cos_max(feat);
> +        if ( cos_max > 0 )
> +            break;
> +    }

I think I've seen this a number of times by now - please make this a
helper function, or store the result (which isn't going to change
afaict).

Other than these comments given to earlier patches apply here too,
as I think is obvious.

Jan


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