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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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().

>> 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'.

I think we had that name already before, and I still consider it
unsuitable. If insert_val() doesn't suit you, use the slower to
type insert_val_into_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.

At the point you introduce MBA support, which isn't in this series.

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

As per above, it would need abstracting out _then_, not _now_.

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

Why? Generic code knows how many times to call ->get_val(), by
way of knowing cos_num.

Jan

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