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

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

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!

> >> > +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).
> 
If so, I think I should define a function to iterate the function array
to return TRUE if any feature has been set into the array. Then, use
this function to replace above checking points.

> Jan

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