[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v10 03/25] x86: refactor psr: implement main data structures.
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |