Re: [Xen-devel] [PATCH v9 12/25] x86: refactor psr: L3 CAT: set value: implement cos id picking flow.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.03.17 at 06:58, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-03-27 04:37:37, Jan Beulich wrote:
>> >>> On 16.03.17 at 12:08, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > +static bool cat_fits_cos_max(const uint32_t val[],
>> > +                             const struct feat_node *feat,
>> > +                             unsigned int cos)
>> > +{
>> > +    if ( cos > feat->info.cat_info.cos_max &&
>> > +         val[0] != feat->cos_reg_val[0] )
>> > +            /*
>> > +             * Exceed cos_max and value to set is not default,
>> > +             * return error.
>> > +             */
>> > +            return false;
>> > +
>> > +    return true;
>> > +}
>> 
>> Same here - with cos_max moved out, the hook would seem to
>> become unnecessary.
>> 
> As explanation in previous patch, CDP has different behavior.
> static bool l3_cdp_fits_cos_max(...)
> {
>     if ( cos > feat->info.cat_info.cos_max &&
>          (val[0] != get_cdp_data(feat, 0) || val[1] != get_cdp_code(feat, 0)) 
> )
>             /*
>              * Exceed cos_max and value to set is not default,
>              * return error.
>              */
>             return false;
> 
>     return true;
> 
> }

As said in reply, by making get_val() flexible enough you should
be able to avoid this. The caller knows how many values to
compare.

>> >  static int pick_avail_cos(const struct psr_socket_info *info,
>> >                            const uint32_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;
>> > +
>> >      ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock)));
>> > -    return -ENOENT;
>> > +
>> > +    /* cos_max is the one of the feature which is being set. */
>> > +    feat = info->features[feat_type];
>> > +    if ( !feat )
>> > +        return -ENOENT;
>> > +
>> > +    cos_max = feat->ops.get_cos_max(feat);
>> > +    if ( !cos_max )
>> > +        return -ENOENT;
>> > +
>> > +    /*
>> > +     * If old cos is referred only by the domain, then use it. And, we 
>> > cannot
>> 
>> "the domain" here is lacking context - there's no domain involved
> 
> How about "the domain input through 'psr_set_val'"?

If you assume this going to remain a helper function for just this
one purpose, then I could live with that. Note however that if
ever a 2nd caller would appear, such a comment likely would
become stale. Therefore it is generally better to write comments
based on what the specific function does or assumes, without
regard to its caller(s) assumptions/restrictions.

Jan


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