Re: [Xen-devel] [PATCH v9 12/25] x86: refactor psr: L3 CAT: set value: implement cos id picking flow.

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

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