Re: [Xen-devel] [PATCH v9 10/25] x86: refactor psr: L3 CAT: set value: assemble features value array.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.03.17 at 05:12, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-03-27 04:17:28, Jan Beulich wrote:
>> >>> On 16.03.17 at 12:08, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > --- a/xen/arch/x86/psr.c
>> > +++ b/xen/arch/x86/psr.c
>> > @@ -101,6 +101,28 @@ struct feat_node {
>> >          /* get_val is used to get feature COS register value. */
>> >          void (*get_val)(const struct feat_node *feat, unsigned int cos,
>> >                          enum cbm_type type, uint32_t *val);
>> > +
>> > +        /*
>> > +         * get_old_val and set_new_val are a pair of functions called in 
>> > order.
>> > +         * The caller will traverse all features in the array and call
>> > +         * 'get_old_val' to get old_cos register value of all supported
>> > +         * features. Then, call 'set_new_val' to set the new value for the
>> > +         * designated feature.
>> > +         *
>> > +         * All the values are set into value array according to the 
>> > traversal
>> > +         * order, meaning the same order of feature array members.
>> > +         *
>> > +         * The return value meaning of set_new_val:
>> > +         * 0 - success.
>> > +         * negative - error.
>> > +         */
>> > +        void (*get_old_val)(uint32_t val[],
>> > +                            const struct feat_node *feat,
>> > +                            unsigned int old_cos);
>> > +        int (*set_new_val)(uint32_t val[],
>> > +                           const struct feat_node *feat,
>> > +                           enum cbm_type type,
>> > +                           uint32_t new_val);
>> 
>> Along the lines of an earlier comment - are "old" and "new" really
>> meaningful here?
>> 
> Maybe 'old' is not accurate. How about 'current'? In fact, we use this
> function to get domain's current CBM value. Furthermore, this is to 
> distinguish
> 'get_val' which is declared above.

I'm fine with "current", but the name collision - would "current" be
omitted still bothers me. The fact that cat_get_old_val() calls
cat_get_val(), however, strongly suggests that the hook here is
redundant anyway. Even in the CDP case I think you can get
away without it, but if this turns out really impossible or clumsy,
then the hook could be introduced there (with a better name)
and be an optional one (with the caller using ->get_val() if the
one here is NULL).

> I think 'new' is meaningful to express we are setting the newly input value.

Well, this is the meaning to its caller. The function itself doesn't
care whether the value is a new one, or just some other value
coming from an unnamed source.

>> > @@ -212,6 +234,29 @@ static enum psr_feat_type 
>> > psr_cbm_type_to_feat_type(enum cbm_type type)
>> >  }
>> >  
>> >  /* CAT common functions implementation. */
>> > +static bool psr_check_cbm(unsigned int cbm_len, uint32_t cbm)
>> > +{
>> > +    unsigned int first_bit, zero_bit;
>> > +
>> > +    /* Set bits should only in the range of [0, cbm_len]. */
>> > +    if ( cbm & (~0ul << cbm_len) )
>> 
>> Same question as elsewhere about the use of the ul suffix here:
>> Can cbm_len really be any value in [0,32]? If not, I don't see
>> why the calculation needs to be done as unsigned long. Otoh ...
>> 
> cbm_len is got as below:
> #define CAT_CBM_LEN_MASK 0x1f
> cat.cbm_len = (regs.a & CAT_CBM_LEN_MASK) + 1;
> 
> So its max value is 32.

And its min one is 1. I.e. no need for an unsigned long calculation.

>> > +        return false;
>> > +
>> > +    /* At least one bit need to be set. */
>> > +    if ( cbm == 0 )
>> > +        return false;
>> > +
>> > +    first_bit = find_first_bit((uint64_t *)&cbm, cbm_len);
>> > +    zero_bit = find_next_zero_bit((uint64_t *)&cbm, cbm_len, first_bit);
>> 
>> ... these bogus casts suggest that the function would best have
>> an "unsigned long" parameter.
>> 
> I would like to modify 'cbm' type to 'uint64_t'. Use a local variable in 
> caller
> to do the type conversion. 

Why uint64_t? As said elsewhere, please stay away from fixed width
types if you don't really need them.

>> > @@ -589,7 +672,34 @@ static int gather_val_array(uint32_t val[],
>> >                              const struct psr_socket_info *info,
>> >                              unsigned int old_cos)
>> >  {
>> > -    return -EINVAL;
>> > +    const struct feat_node *feat;
>> > +    unsigned int i;
>> > +
>> > +    if ( !val )
>> > +        return -EINVAL;
>> > +
>> > +    /* Get all features current values according to old_cos. */
>> > +    for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ )
>> > +    {
>> > +        if ( !info->features[i] )
>> > +            continue;
>> > +
>> > +        feat = info->features[i];
>> > +
>> > +        if ( old_cos > feat->ops.get_cos_max(feat) )
>> > +            old_cos = 0;
>> > +
>> > +        /* value getting order is same as feature array */
>> > +        feat->ops.get_old_val(val, feat, old_cos);
>> > +
>> > +        array_len -= feat->cos_num;
>> 
>> So this I should really have asked about on a much earlier patch,
>> but I've recognize the oddity only now: Why is cos_num
>> per-feature-node instead of per-feature? This should really be a
>> field in struct feat_ops (albeit the name "ops" then will be slightly
>> misleading, but I think that's tolerable if you can't think of a better
>> name).
>> 
> Ok, I got your meaning. How about 'feat_props'? No matter operations or
> variables are all properties of the feature.

"props" sounds fine to me.

>> > +        if ( array_len < 0 )
>> > +            return -ENOSPC;
>> 
>> This check needs doing earlier - you need to make sure array_len
>> >= ops.cos_num prior to calling ops.get_old_val(). (Doing the
>> check after the subtraction even causes wrapping issues, which
>> are even more visible in similar code further down.)
>> 
> Thanks for the suggestion! Will move 'array_len >= cos_num' check prior to
> call 'get_old_val'.

And don't forget to do this everywhere in the series - there were
quite a few similar constructs in later patches, iirc.

Jan

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