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

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

On 17-04-05 02:20:53, Jan Beulich wrote:
> >>> On 05.04.17 at 05:12, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > On 17-04-03 09:50:34, Jan Beulich wrote:
> >> >>> On 01.04.17 at 15:53, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> >> > +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.
> 
> Well, in the interest of not needlessly delaying forward progress
> I'm fine with you keeping this patch for now. Should the series
> miss 4.9, though, I'd prefer if you eliminated it.
> 
As other patches are still not reviewed yet, I am afraid the 4.9 will be missed.
Then, I will consider to eliminate this patch 3.

> >> > +/*
> >> > + * 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'.
> 
> This is contradictory: The two fields aren't in struct feat_node in
> the current version of the patch, so do you mean "keep in struct
> feat_props" or "move back to struct feat_node"?
> 
"move back to struct 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!
> 
> As said above, I didn't get to look at the other patches yet. It's
> up to you whether to resubmit with the adjustments done here
> (and carried through the rest of the series), or whether you wait.
> As it's Wednesday already, don't have too much hope for the
> series to make 4.9 - I'm sorry for this, but I also can't do much
> about it.
> 
Per current status, I will wait for all your review comments. Thanks!

BRs,
Sun Yi

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