[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 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: > >> > On 17-03-30 05:55:52, Jan Beulich wrote: > >> >> >>> On 30.03.17 at 03:37, <yi.y.sun@xxxxxxxxxxxxxxx> wrote: > >> >> > On 17-03-29 03:57:52, Jan Beulich wrote: > >> >> >> >>> On 29.03.17 at 03:36, <yi.y.sun@xxxxxxxxxxxxxxx> wrote: > >> >> >> > On 17-03-29 09:20:21, Yi Sun wrote: > >> >> >> >> On 17-03-28 06:20:48, Jan Beulich wrote: > >> >> >> >> > >>> On 28.03.17 at 13:59, <yi.y.sun@xxxxxxxxxxxxxxx> wrote: > >> >> >> >> > > I think we at least need a 'get_val()' hook. > >> >> >> >> > > >> >> >> >> > Of course. > >> >> >> >> > > >> >> >> >> > > I try to implement CAT/CDP hook. > >> >> >> >> > > Please help to check if this is what you thought. > >> >> >> >> > > >> >> >> >> > One remark below, but other than that - yes. > >> >> >> >> > > >> >> >> >> > > static void cat_get_val(const struct feat_node *feat, > >> >> >> >> > > unsigned int cos, > >> >> >> >> > > enum cbm_type type, int flag, > >> >> >> >> > > uint32_t *val) > >> >> >> >> > > { > >> >> >> >> > > *val = feat->cos_reg_val[cos]; > >> >> >> >> > > } > >> >> >> >> > > > >> >> >> >> > > static void l3_cdp_get_val(const struct feat_node *feat, > >> >> >> >> > > unsigned int > >> >> > cos, > >> >> >> >> > > enum cbm_type type, int flag, > >> >> >> >> > > uint32_t *val) > >> >> >> >> > > { > >> >> >> >> > > if ( type == PSR_CBM_TYPE_L3_DATA || flag == 0 ) > >> >> >> >> > > *val = get_cdp_data(feat, cos); > >> >> >> >> > > if ( type == PSR_CBM_TYPE_L3_CODE || flag == 1 ) > >> >> >> >> > > *val = get_cdp_code(feat, cos); > >> >> >> >> > > } > >> >> >> >> > > >> >> >> >> > Why the redundancy between type and flag? > >> >> >> >> > > >> >> >> >> For psr_get_val, upper layer input the cbm_type to get either > >> >> >> >> DATA or > > CODE > >> >> >> >> value. For other cases, we use flag as cos_num index to get > >> >> >> >> either DATA > > or > >> >> >> >> CODE. > >> >> >> >> > >> >> >> > Let me explain more to avoid confusion. For other cases, we use > >> >> >> > cos_num > > as > >> >> >> > index to get values from a feature. In these cases, we do not know > >> >> >> > the > >> >> >> > cbm_type of the feature. So, I use the cos_num as flag to make > >> >> >> > 'get_val' > >> >> >> > know which value should be returned. > >> >> >> > >> >> >> I'm pretty sure this redundancy can be avoided. > >> >> >> > >> >> > Then, I think I have to reuse the 'type'. As only CDP needs type to > >> >> > decide > >> >> > which value to be returned so far, I think I can implement codes like > > below > >> >> > to make CDP can handle all scenarios. > >> >> > > >> >> > static void l3_cdp_get_val(const struct feat_node *feat, unsigned int > >> >> > cos, > >> >> > enum cbm_type type, uint32_t *val) > >> >> > { > >> >> > if ( type == PSR_CBM_TYPE_L3_DATA || flag == 0xF000 ) > >> >> > *val = get_cdp_data(feat, cos); > >> >> > if ( type == PSR_CBM_TYPE_L3_CODE || flag == 0xF001 ) > >> >> > *val = get_cdp_code(feat, cos); > >> >> > } > >> >> > > >> >> > static bool fits_cos_max(...) > >> >> > { > >> >> > ...... > >> >> > for (i = 0; i < feat->props->cos_num; i++) > >> >> > { > >> >> > feat->props->get_val(feat, cos, i + 0xF000, &default_val); > >> >> > if ( val[i] == default_val ) > >> >> > ...... > >> >> > } > >> >> > ...... > >> >> > } > >> >> > > >> >> > Is that good for you? > >> >> > >> >> Urgh - no, not really: This is hackery. Do you have a tree > >> >> somewhere so I could look at the file after at least the CDP > >> >> patches have all been applied, to see whether I have a > >> >> better idea? Alternatively, could you attach psr.c the way > >> >> you have it right now with the entire series applied? > >> >> > >> > 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; } } /* Example for getting value by cos_num. */ static bool fits_cos_max(...) { enum psr_feat_type feat_type; ... for ( j = 0; j < feat->props->cos_num; j++ ) { unsigned int type = feat_type << 16 + j; feat->props->get_val(feat, 0, type, &default_val); ... } /* Example for getting value by cbm_type. */ int psr_get_val(enum cbm_type type...) { ... feat->props->get_val(feat, cos, type, val); ... } _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |