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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

>> > +struct psr_socket_info {
>> > +    /*
>> > +     * It maps to values defined in 'enum psr_feat_type' below. Value in 
>> > 'enum
>> > +     * psr_feat_type' means the bit position.
>> > +     * bit 0:   L3 CAT
>> > +     * bit 1:   L3 CDP
>> > +     * bit 2:   L2 CAT
>> > +     */
>> > +    unsigned int feat_mask;
>> 
>> Comment or not I don't understand what use this mask is, and
>> this is again something which I'm pretty sure I've mentioned in
>> v8 review, when the switch to ...
>> 
>> > +    struct feat_node *features[PSR_SOCKET_MAX_FEAT];
>> > +    unsigned int cos_ref[MAX_COS_REG_CNT];
>> 
>> ... this array was suggested by Roger. The pointers in the
>> array being non-NULL can - afaict - easily fulfill the role of
>> the mask bits, so the latter are redundant.
>> 
> I thought 'feat_mask' can facilitate things handling. If we check feature 
> array
> to know if any feature has been initialized, we have to iterate the array. 
> But
> it is simple to check 'feat_mask'.
> 1. In 'psr_cpu_init', we can use it to check if initialization on the socket
>    has been done.
>     if ( info->feat_mask )
>         goto assoc_init;
> 2. Same purpose in 'psr_assoc_init'.
>     if ( info->feat_mask )
>         psra->cos_mask = ((1ull << get_count_order(cos_max)) - 1) <<
>                          PSR_ASSOC_REG_SHIFT;
> 3. Same purpose in 'get_socket_info'.
>     if ( !socket_info[socket].feat_mask )
>  
> What is your advice? Thanks!

My advice is: Avoid redundant data as much as possible. Any such
instance poses the risk of the two pieces of information going out
of sync. (That isn't to say that there aren't cases where redundancy
is almost unavoidable, e.g. in order to not overly complicate code,
but that's pretty clearly not the case here).

Jan

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