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 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!

> allow elimination of all of the get_old_val, compare_val, and
> fits_cos_max hooks afaict. At that point the "new" in
> set_new_val is also no longer needed to contrast with the "old"
> in the no longer existing get_old_val. Arguably insert_val may
> be even slightly more precise in describing what the function
> does.

I have modified it to 'set_val_to_array'.

> 
> The checks done first in what is currently *_set_new_val() also
> look like they could be moved into the (generic) caller(s), making

MBA value is throttling value which is different with CBM. So, the
check is different. So, we have to have 'check_val()' at least.

> clear that at least for now even cbm_len (just like suggested for
> cos_max) should be a generic rather than unionized field in

MBA does not have cbm_len.

> struct feat_node. In the worst case a new check_val() hook
> might be needed.
> 
After analyzing codes again, for 'gather_val', 'compare_val' and
'fits_cos_max', we need get all values of a feature. E.g. we need
get both DATA and CODE for CDP. But for 'psr_get_val', we only need
get one value per cbm_type. So, I think these should be different
hooks. Can I implement 'get_val' for getting one value per cbm_type,
and 'get_vals' to get all values?

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel