Re: [Xen-devel] [PATCH v9 03/25] x86: refactor psr: implement main data structures.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.03.17 at 09:12, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-03-27 00:20:58, Jan Beulich wrote:
>> >>> On 27.03.17 at 04:38, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > On 17-03-24 10:19:30, Jan Beulich wrote:
>> >> >>> On 16.03.17 at 12:07, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> >> > +struct psr_cat_hw_info {
>> >> > +    unsigned int cbm_len;
>> >> > +    unsigned int cos_max;
>> >> 
>> >> So you have this field, and ...
>> >> 
>> >> > +};
>> >> > +
>> >> > +/*
>> >> > + * This structure represents one feature.
>> >> > + * feat_ops    - Feature operation callback functions.
>> >> > + * info        - Feature HW info.
>> >> > + * cos_reg_val - Array to store the values of COS registers. One entry 
>> >> > stores
>> >> > + *               the value of one COS register.
>> >> > + *               For L3 CAT and L2 CAT, one entry corresponds to one 
>> >> > COS_ID.
>> >> > + *               For CDP, two entries correspond to one COS_ID. E.g.
>> >> > + *               COS_ID=0 corresponds to cos_reg_val[0] (Data) and
>> >> > + *               cos_reg_val[1] (Code).
>> >> > + * cos_num     - COS registers number that feature uses in one time 
>> >> > access.
>> >> > + */
>> >> > +struct feat_node {
>> >> > +    /*
>> >> > +     * This structure defines feature operation callback functions. 
>> >> > Every feature
>> >> > +     * enabled MUST implement such callback functions and register 
>> >> > them to ops.
>> >> > +     *
>> >> > +     * Feature specific behaviors will be encapsulated into these 
>> >> > callback
>> >> > +     * functions. Then, the main flows will not be changed when 
>> >> > introducing a new
>> >> > +     * feature.
>> >> > +     */
>> >> > +    struct feat_ops {
>> >> > +        /* get_cos_max is used to get feature's cos_max. */
>> >> > +        unsigned int (*get_cos_max)(const struct feat_node *feat);
>> >> 
>> >> ... you have this op, suggesting that you expect all features
>> >> to have a cos_max. Why don't you then store the value in a
>> >> field which is not per-feature, just like ...
>> >> 
>> >> > +    } ops;
>> >> > +
>> >> > +    /* Encapsulate feature specific HW info here. */
>> >> > +    union {
>> >> > +        struct psr_cat_hw_info cat_info;
>> >> > +    } info;
>> >> > +
>> >> > +    uint32_t cos_reg_val[MAX_COS_REG_CNT];
>> >> > +    unsigned int cos_num;
>> >> 
>> >> ... this. I'm pretty sure that during v8 review I did say that
>> >> this approach should be extended to all pieces of information
>> >> where it can be applied.
>> >> 
>> > I thought this when implementing v9. As cos_max is part of feature HW 
>> > info, I
>> > thought it would be better to keep it in hw_info structure. Different 
>> > features
>> > may have different hw_info, so the callback function is needed to get 
>> > cos_max.
>> > Of course, we can keep a copy in feat_node but it is redundant. How do you
>> > think?
>> 
>> I don't follow - as long as you have a universal get_cos_max()
>> accesses, and as long as what that function returns depends
>> only on invariable things like CPUID output, I don't see why
>> this needs to be a function instead of a data field. If some
>> (perhaps future, theoretical) feature didn't want/need a
>> get_cos_max() function, the presence of that hook would
>> become questionable, yet it could surely become an optional
>> hook. However, the hook being optional could as well be
>> represented by the data field getting assigned a value of 0.
>> 
>> Bottom line: Data which can be calculated at initialization
>> time should be stored in a date object, rather than re-
>> calculating it over and over.
>> 
> The purpose to use the function is just not to define a redundant member
> in 'struct feat_node'.
> 
> The cos_max is got in cat_init_feature in patch 5 and kept in the feature's
> hw_info. The 'get_cos_max' only returns DIFFERENT features' cos_max without
> recalculation. E.g:
> 
> CAT/CDP:
> static unsigned int cat_get_cos_max(const struct feat_node *feat)
> {
>     return feat->info.cat_info.cos_max;
> }
> 
> MBA:
> static unsigned int mba_get_cos_max(const struct feat_node *feat)
> {
>     return feat->info.mba_info.cos_max;
> }
> 
> But I think it is ok to add a new member in 'struct feat_node' to keep
> cos_max for the feature.
> 
> What do you prefer? Thanks!

Sigh. If you see the above two functions, and if you expect future
new features to have similar functions, then why in the world would
you want to make the field feature specific? If every feature is
expected to have some form of maximum COS, then this is a
property applicable to all features and hence should be a field in
the common part of the structure.

Jan

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