[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 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. > --- > 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. > +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. 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. > +/* > + * 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? 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). Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |