[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v9 12/25] x86: refactor psr: L3 CAT: set value: implement cos id picking flow.
On 17-03-31 08:35:14, Jan Beulich wrote: > >>> On 31.03.17 at 15:22, <yi.y.sun@xxxxxxxxxxxxxxx> wrote: > > On 17-03-31 06:51:07, Jan Beulich wrote: > >> >>> On 31.03.17 at 14:40, <yi.y.sun@xxxxxxxxxxxxxxx> wrote: > >> > On 17-03-31 04:19:49, Jan Beulich wrote: > >> >> >>> On 31.03.17 at 11:12, <yi.y.sun@xxxxxxxxxxxxxxx> wrote: > >> >> > On 17-03-31 02:47:25, Jan Beulich wrote: > >> >> >> >>> On 30.03.17 at 14:10, <yi.y.sun@xxxxxxxxxxxxxxx> wrote: > >> >> >> > I think you can check v9 codes here: > >> >> >> > https://github.com/yisun-git/xen/tree/l2_cat_v9 > >> >> >> > >> >> >> Looking at this made me notice that cat_get_old_val() passes a > >> >> >> bogus literal 0 to cat_get_val(), which needs taking care of too. > >> >> >> One option I can see is for each feature to make available an > >> >> >> array of type enum cbm_type, with cos_num elements. The order > >> >> >> would match that of the order of values in their arrays. This will > >> >> > > >> >> > Sorry, not very clear your meaning. How to do that? Could you please > >> >> > provide pieces of codes? Thanks! > >> >> > >> >> I'm sorry, but I'm afraid I don't see how I would reasonably supply > >> >> code here without taking over your series altogether (which I don't > >> >> intend to do). What is unclear with, at the example of CDP, you > >> >> needing to add an array at initialization time, slot 0 of which holds > >> >> PSR_CBM_TYPE_L3_DATA and slot 1 PSR_CBM_TYPE_L3_CODE (or > >> >> the other way around). Granted I was wrong with the type of the > >> >> array (as the above aren't enum psr_feat_type enumerators, but > >> >> enum cbm_type ones), but I think the basic idea should have been > >> >> clear anyway: You need to provide a way for generic code to pass > >> >> suitable type information into ->get_val(). > >> >> > >> > May I change the 'get_val()' parameter 'enum cbm_type' to a generic type > >> > 'unsigned int' to make it be a flexible type, and then combine feature > >> > type with cos_num together as a flag to indicate which feature it is, > >> > which value to get and distinguish it with cbm_type? For example: > >> > > >> > #define CDP_GATHER_BOTH_DATA ( PSR_SOCKET_L3_CDP << 16 ) > >> > #define CDP_GATHER_BOTH_CODE ( PSR_SOCKET_L3_CDP << 16 + 1 ) > >> > static void l3_cdp_get_val(const struct feat_node *feat, unsigned int > >> > cos, > >> > unsigned int type, uint32_t *val) > >> > { > >> > switch ( type ) > >> > { > >> > case PSR_CBM_TYPE_L3_DATA: > >> > case CDP_GATHER_BOTH_DATA: > >> > *val = get_cdp_data(feat, cos); > >> > break; > >> > case PSR_CBM_TYPE_L3_CODE: > >> > case CDP_GATHER_BOTH_CODE: > >> > *val = get_cdp_code(feat, cos); > >> > break; > >> > } > >> > } > >> > >> The two case labels are still indicative of unnecessary redundancy > >> (and, even right now only highly theoretical, risk of collisions). What's > >> wrong with the model I've proposed? > >> > > Oh, sorry. I did not understand your proposal just now so I provided another > > solution. > > > > After reading your suggestion again, I think your meaing is below in codes: > > > > struct feat_props { > > ... > > unsigned int cos_num; > > enum cbm_type cos_to_type[2]; > > ... > > } > > > > static void cat_init_feature(...) > > { > > ... > > case PSR_SOCKET_L3_CDP: > > feat->props->cos_to_type[0] = PSR_CBM_TYPE_L3_DATA; > > feat->props->cos_to_type[1] = PSR_CBM_TYPE_L3_CODE; > > ... > > } > > > > Then, in functions to iterate 'cos_num', we can input 'cos_to_type[i]'. > > Yes (albeit I'd call the array field just "type"). > Got it. This should be the last issue not locked down. I will send out v10 in soon. Thank you very much for the review! 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 |