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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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:
>> 
>> 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!

Well, this one patch only - I didn't even get to look at the others yet.

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

>> > +/*
>> > + * 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"?

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

Jan

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