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

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

On 17-04-03 09:50:34, Jan Beulich wrote:
> >>> On 01.04.17 at 15:53, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> 
> I was about to ack this, but there are a few more things which bother
> me.
> 
Do you mean ack to this patch 3 or whole patch set? Thanks!

> > ---
> > v10:
> >     - remove initialization for 'PSR_SOCKET_L3_CAT'.
> >       (suggested by Jan Beulich)
> >     - rename 'feat_ops' to 'feat_props'.
> >       (suggested by Jan Beulich)
> >     - move 'cbm_len' to 'feat_props' because it is feature independent so 
> > far.
> >       (suggested by Jan Beulich)
> >     - move 'cos_max' to 'feat_props' because it is feature independent.
> >       (suggested by Jan Beulich)
> >     - move 'cos_num' to 'feat_props' because it is feature independent.
> >       (suggested by Jan Beulich)
> >     - remove union 'info' and struct 'psr_cat_hw_info'.
> >     - remove 'get_cos_max' from 'feat_props'.
> >       (suggested by Jan Beulich)
> >     - remove 'feat_mask' from 'psr_socket_info' because we can use 
> > 'features[]'
> >       to check if any feature is initialized.
> >       (suggested by Jan Beulich)
> >     - move 'ref_lock' above 'cos_ref'.
> >       (suggested by Jan Beulich)
> 
> The movement done is fine for the moment, but it would have been
> even better if you had moved it ahead of the other array too.
> 
Got it.

> > +enum psr_feat_type {
> > +    PSR_SOCKET_L3_CAT,
> > +    PSR_SOCKET_L3_CDP,
> > +    PSR_SOCKET_L2_CAT,
> > +    PSR_SOCKET_MAX_FEAT,
> > +};
> 
> It's not really logical to have the first three here - they should have
> been added by their respective patches. Which gets me back to
> the question of the usefulness of a patch like this one: Without the
> following patches, everything being added here is unused. Iirc the
> original version of this patch was quite a bit larger, better justifying
> to break out all of this. The size it has shrunk to by now is a pretty
> good indication that it should have been folded altogether.
> 
Ok, I will add item one by one in related feature's patch. But can I keep
this patch 3? This patch outlines the infrastructure and I demonstrated how
I analyze the data structures in commit message. If I split these data
structures into pieces and implement them into different patches, I am
afraid to lose the completeness.

> Also I think we've had the discussion about the difference between
> "max" and "num" already: The former normally indicates an inclusive
> upper bound, while the latter would usually be an exclusive one.
> Clearly the above naming doesn't match up with this.
> 
Ok, may change it to 'PSR_SOCKET_FEAT_NUM'.

> > +/*
> > + * This structure represents one feature.
> > + * feat_props  - Feature properties, including operation callback functions
> > +                 and feature common values.
> > + * 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).
> > + */
> > +struct feat_node {
> > +    /*
> > +     * This structure defines feature operation callback functions. Every
> > +     * feature enabled MUST implement such callback functions and register
> > +     * them to props.
> > +     *
> > +     * Feature specific behaviors will be encapsulated into these callback
> > +     * functions. Then, the main flows will not be changed when introducing
> > +     * a new feature.
> > +     *
> > +     * Feature independent HW info and common values are also defined in 
> > it.
> > +     */
> > +    const struct feat_props {
> > +        /*
> > +         * cos_num, cos_max and cbm_len are common values for all features
> > +         * so far.
> > +         * cos_num - COS registers number that feature uses for one COS ID.
> > +         *           It is defined in SDM.
> > +         * cos_max - The max COS registers number got through CPUID.
> > +         * cbm_len - The length of CBM got through CPUID.
> > +         */
> > +        unsigned int cos_num;
> > +        unsigned int cos_max;
> > +        unsigned int cbm_len;
> > +    } *props;
> 
> Let's think the data arrangement changes done so far a little further:
> Why do we need this pointer per-node (i.e. once per socket)? Now
> that we have a well established order of features used to index
> struct psr_socket_info's features[], why can't the same indexing be
> used to obtain the props pointer from a static (single instance) array
> of props pointers?
> 
Hmm, yes, we can declare a static standalone array of props pointers for all
features and all sockets. It does not belong to 'feat_node' or 'socket_info'.

> Otoh I'm not sure you really meant to move all three data members
> into there, if you still have reason to believe that different sockets
> may have different values for cos_max and/or cbm_len. I.e. these
> might better be members of struct feat_node (just not placed in a
> union, as you had them in v9).
> 
We still believe there may be chances that different sockets may have different
configurations. So, I would prefer to keep cos_max/cbm_len in 'feat_node'.

Because this Friday is the code freeze date, can I quickly make the changes
according to above comments and submit a new version if you do not have
further oppinion? Thanks!

BRs,
Sun Yi

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