[v9,12/25] x86: refactor psr: L3 CAT: set value: implement cos id picking flow.

Commit Message

Yi Sun March 16, 2017, 11:08 a.m. UTC

Continue from previous patch:
'x86: refactor psr: L3 CAT: set value: implement cos finding flow.'

If fail to find a COS ID, we need pick a new COS ID for domain. Only COS ID
that ref[COS_ID] is 1 or 0 can be picked to input a new set feature values.

Signed-off-by: Yi Sun <[e-mail]>
---
v9:
    - modify return value of 'pick_avail_cos' to make it more accurate.
    - rename 'l3_cat_fits_cos_max' to 'cat_fits_cos_max' to cover L3/L2 CAT
      features.
      (suggested by Roger Pau)
    - replace feature list handling to feature array handling.
      (suggested by Roger Pau)
    - fix comment.
      (suggested by Wei Liu)
    - directly use 'cos_reg_val[0]' as default value.
      (suggested by Jan Beulich)
    - replace 'get_cos_num' to 'feat->cos_num'.
      (suggested by Jan Beulich)
    - modify patch title to indicate 'L3 CAT'.
      (suggested by Jan Beulich)
    - changes about 'uint64_t' to 'uint32_t'.
      (suggested by Jan Beulich)
v5:
    - modify commit message to provide exact patch name to continue from.
      (suggested by Jan Beulich)
    - change 'exceeds_cos_max' to 'fits_cos_max' to be accurate.
      (suggested by Jan Beulich)
    - modify comments according to changes of codes.
      (suggested by Jan Beulich)
    - modify return value of callback functions because we do not need them
      to return number of entries the feature uses. In caller, we call
      'get_cos_num' to get the number of entries the feature uses.
      (suggested by Jan Beulich)
    - move type check out from callback functions to caller.
      (suggested by Jan Beulich)
    - modify variables names to make them better, e.g. 'feat_tmp' to 'feat'.
      (suggested by Jan Beulich)
    - modify code format.
      (suggested by Jan Beulich)
v4:
    - create this patch to make codes easier to understand.
      (suggested by Jan Beulich)
---
 xen/arch/x86/psr.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 98 insertions(+), 1 deletion(-)

Comments

Jan Beulich March 27, 2017, 10:37 a.m. UTC | #1

>>> On 16.03.17 at 12:08, <[e-mail]> wrote:
> +static bool cat_fits_cos_max(const uint32_t val[],
> +                             const struct feat_node *feat,
> +                             unsigned int cos)
> +{
> +    if ( cos > feat->info.cat_info.cos_max &&
> +         val[0] != feat->cos_reg_val[0] )
> +            /*
> +             * Exceed cos_max and value to set is not default,
> +             * return error.
> +             */
> +            return false;
> +
> +    return true;
> +}

Same here - with cos_max moved out, the hook would seem to
become unnecessary.

>  static int pick_avail_cos(const struct psr_socket_info *info,
>                            const uint32_t val[], uint32_t array_len,
>                            unsigned int old_cos,
>                            enum psr_feat_type feat_type)
>  {
> +    unsigned int cos;
> +    unsigned int cos_max = 0;
> +    const struct feat_node *feat;
> +    const unsigned int *ref = info->cos_ref;
> +
>      ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock)));
> -    return -ENOENT;
> +
> +    /* cos_max is the one of the feature which is being set. */
> +    feat = info->features[feat_type];
> +    if ( !feat )
> +        return -ENOENT;
> +
> +    cos_max = feat->ops.get_cos_max(feat);
> +    if ( !cos_max )
> +        return -ENOENT;
> +
> +    /*
> +     * If old cos is referred only by the domain, then use it. And, we cannot

"the domain" here is lacking context - there's no domain involved
in the function - "... the domain currently using it, then re-use it"?

> +     * use id 0 because it stores the default values.
> +     */
> +    if ( old_cos && ref[old_cos] == 1 &&
> +         fits_cos_max(val, array_len, info, old_cos) )
> +            return old_cos;
> +
> +    /* Find an unused one other than cos0. */
> +    for ( cos = 1; cos <= cos_max; cos++ )
> +    {
> +        /*
> +         * ref is 0 means this COS is not used by other domain and
> +         * can be used for current setting.
> +         */
> +        if ( !ref[cos] )
> +        {
> +            if ( !fits_cos_max(val, array_len, info, cos) )
> +                return -EOVERFLOW;

Perhaps better use "break;" here.

> +            return cos;
> +        }
> +    }
> +
> +    return -EOVERFLOW;
>  }

Jan

Yi Sun March 28, 2017, 4:58 a.m. UTC | #2

On 17-03-27 04:37:37, Jan Beulich wrote:
> >>> On 16.03.17 at 12:08, <[e-mail]> wrote:
> > +static bool cat_fits_cos_max(const uint32_t val[],
> > +                             const struct feat_node *feat,
> > +                             unsigned int cos)
> > +{
> > +    if ( cos > feat->info.cat_info.cos_max &&
> > +         val[0] != feat->cos_reg_val[0] )
> > +            /*
> > +             * Exceed cos_max and value to set is not default,
> > +             * return error.
> > +             */
> > +            return false;
> > +
> > +    return true;
> > +}
> 
> Same here - with cos_max moved out, the hook would seem to
> become unnecessary.
> 
As explanation in previous patch, CDP has different behavior.
static bool l3_cdp_fits_cos_max(...)
{
    if ( cos > feat->info.cat_info.cos_max &&
         (val[0] != get_cdp_data(feat, 0) || val[1] != get_cdp_code(feat, 0)) )
            /*
             * Exceed cos_max and value to set is not default,
             * return error.
             */
            return false;

    return true;

}

> >  static int pick_avail_cos(const struct psr_socket_info *info,
> >                            const uint32_t val[], uint32_t array_len,
> >                            unsigned int old_cos,
> >                            enum psr_feat_type feat_type)
> >  {
> > +    unsigned int cos;
> > +    unsigned int cos_max = 0;
> > +    const struct feat_node *feat;
> > +    const unsigned int *ref = info->cos_ref;
> > +
> >      ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock)));
> > -    return -ENOENT;
> > +
> > +    /* cos_max is the one of the feature which is being set. */
> > +    feat = info->features[feat_type];
> > +    if ( !feat )
> > +        return -ENOENT;
> > +
> > +    cos_max = feat->ops.get_cos_max(feat);
> > +    if ( !cos_max )
> > +        return -ENOENT;
> > +
> > +    /*
> > +     * If old cos is referred only by the domain, then use it. And, we cannot
> 
> "the domain" here is lacking context - there's no domain involved

How about "the domain input through 'psr_set_val'"?

> in the function - "... the domain currently using it, then re-use it"?
> 
Ok, 're-use it' is better.

> > +     * use id 0 because it stores the default values.
> > +     */
> > +    if ( old_cos && ref[old_cos] == 1 &&
> > +         fits_cos_max(val, array_len, info, old_cos) )
> > +            return old_cos;
> > +
> > +    /* Find an unused one other than cos0. */
> > +    for ( cos = 1; cos <= cos_max; cos++ )
> > +    {
> > +        /*
> > +         * ref is 0 means this COS is not used by other domain and
> > +         * can be used for current setting.
> > +         */
> > +        if ( !ref[cos] )
> > +        {
> > +            if ( !fits_cos_max(val, array_len, info, cos) )
> > +                return -EOVERFLOW;
> 
> Perhaps better use "break;" here.
> 
Oh yes, thanks!

> > +            return cos;
> > +        }
> > +    }
> > +
> > +    return -EOVERFLOW;
> >  }
> 
> Jan

Jan Beulich March 28, 2017, 8:45 a.m. UTC | #3

>>> On 28.03.17 at 06:58, <[e-mail]> wrote:
> On 17-03-27 04:37:37, Jan Beulich wrote:
>> >>> On 16.03.17 at 12:08, <[e-mail]> wrote:
>> > +static bool cat_fits_cos_max(const uint32_t val[],
>> > +                             const struct feat_node *feat,
>> > +                             unsigned int cos)
>> > +{
>> > +    if ( cos > feat->info.cat_info.cos_max &&
>> > +         val[0] != feat->cos_reg_val[0] )
>> > +            /*
>> > +             * Exceed cos_max and value to set is not default,
>> > +             * return error.
>> > +             */
>> > +            return false;
>> > +
>> > +    return true;
>> > +}
>> 
>> Same here - with cos_max moved out, the hook would seem to
>> become unnecessary.
>> 
> As explanation in previous patch, CDP has different behavior.
> static bool l3_cdp_fits_cos_max(...)
> {
>     if ( cos > feat->info.cat_info.cos_max &&
>          (val[0] != get_cdp_data(feat, 0) || val[1] != get_cdp_code(feat, 0)) )
>             /*
>              * Exceed cos_max and value to set is not default,
>              * return error.
>              */
>             return false;
> 
>     return true;
> 
> }

As said in reply, by making get_val() flexible enough you should
be able to avoid this. The caller knows how many values to
compare.

>> >  static int pick_avail_cos(const struct psr_socket_info *info,
>> >                            const uint32_t val[], uint32_t array_len,
>> >                            unsigned int old_cos,
>> >                            enum psr_feat_type feat_type)
>> >  {
>> > +    unsigned int cos;
>> > +    unsigned int cos_max = 0;
>> > +    const struct feat_node *feat;
>> > +    const unsigned int *ref = info->cos_ref;
>> > +
>> >      ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock)));
>> > -    return -ENOENT;
>> > +
>> > +    /* cos_max is the one of the feature which is being set. */
>> > +    feat = info->features[feat_type];
>> > +    if ( !feat )
>> > +        return -ENOENT;
>> > +
>> > +    cos_max = feat->ops.get_cos_max(feat);
>> > +    if ( !cos_max )
>> > +        return -ENOENT;
>> > +
>> > +    /*
>> > +     * If old cos is referred only by the domain, then use it. And, we cannot
>> 
>> "the domain" here is lacking context - there's no domain involved
> 
> How about "the domain input through 'psr_set_val'"?

If you assume this going to remain a helper function for just this
one purpose, then I could live with that. Note however that if
ever a 2nd caller would appear, such a comment likely would
become stale. Therefore it is generally better to write comments
based on what the specific function does or assumes, without
regard to its caller(s) assumptions/restrictions.

Jan

Yi Sun March 28, 2017, 10:31 a.m. UTC | #4

On 17-03-28 02:45:13, Jan Beulich wrote:
> >>> On 28.03.17 at 06:58, <[e-mail]> wrote:
> > On 17-03-27 04:37:37, Jan Beulich wrote:
> >> >>> On 16.03.17 at 12:08, <[e-mail]> wrote:
> >> > +static bool cat_fits_cos_max(const uint32_t val[],
> >> > +                             const struct feat_node *feat,
> >> > +                             unsigned int cos)
> >> > +{
> >> > +    if ( cos > feat->info.cat_info.cos_max &&
> >> > +         val[0] != feat->cos_reg_val[0] )
> >> > +            /*
> >> > +             * Exceed cos_max and value to set is not default,
> >> > +             * return error.
> >> > +             */
> >> > +            return false;
> >> > +
> >> > +    return true;
> >> > +}
> >> 
> >> Same here - with cos_max moved out, the hook would seem to
> >> become unnecessary.
> >> 
> > As explanation in previous patch, CDP has different behavior.
> > static bool l3_cdp_fits_cos_max(...)
> > {
> >     if ( cos > feat->info.cat_info.cos_max &&
> >          (val[0] != get_cdp_data(feat, 0) || val[1] != get_cdp_code(feat, 0)) )
> >             /*
> >              * Exceed cos_max and value to set is not default,
> >              * return error.
> >              */
> >             return false;
> > 
> >     return true;
> > 
> > }
> 
> As said in reply, by making get_val() flexible enough you should
> be able to avoid this.

Sorry, I am confused here. 'fits_cos_max' is called during set value process.
Why "making get_val() flexible enough" can avoid this?

> The caller knows how many values to compare.
> 
My purpose to implement such hook is to avoid caller knows feature details.
Then, we can create a general flow to cover all features. So, I do not
understand your mearing here. Sorry.

> >> >  static int pick_avail_cos(const struct psr_socket_info *info,
> >> >                            const uint32_t val[], uint32_t array_len,
> >> >                            unsigned int old_cos,
> >> >                            enum psr_feat_type feat_type)
> >> >  {
> >> > +    unsigned int cos;
> >> > +    unsigned int cos_max = 0;
> >> > +    const struct feat_node *feat;
> >> > +    const unsigned int *ref = info->cos_ref;
> >> > +
> >> >      ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock)));
> >> > -    return -ENOENT;
> >> > +
> >> > +    /* cos_max is the one of the feature which is being set. */
> >> > +    feat = info->features[feat_type];
> >> > +    if ( !feat )
> >> > +        return -ENOENT;
> >> > +
> >> > +    cos_max = feat->ops.get_cos_max(feat);
> >> > +    if ( !cos_max )
> >> > +        return -ENOENT;
> >> > +
> >> > +    /*
> >> > +     * If old cos is referred only by the domain, then use it. And, we cannot
> >> 
> >> "the domain" here is lacking context - there's no domain involved
> > 
> > How about "the domain input through 'psr_set_val'"?
> 
> If you assume this going to remain a helper function for just this
> one purpose, then I could live with that. Note however that if
> ever a 2nd caller would appear, such a comment likely would
> become stale. Therefore it is generally better to write comments
> based on what the specific function does or assumes, without
> regard to its caller(s) assumptions/restrictions.
> 
Ok, I should explain this in caller and I think there already are
comments to explain this. So, I think I may remove this comment here.

> Jan

Jan Beulich March 28, 2017, 10:40 a.m. UTC | #5

>>> On 28.03.17 at 12:31, <[e-mail]> wrote:
> On 17-03-28 02:45:13, Jan Beulich wrote:
>> >>> On 28.03.17 at 06:58, <[e-mail]> wrote:
>> > On 17-03-27 04:37:37, Jan Beulich wrote:
>> >> >>> On 16.03.17 at 12:08, <[e-mail]> wrote:
>> >> > +static bool cat_fits_cos_max(const uint32_t val[],
>> >> > +                             const struct feat_node *feat,
>> >> > +                             unsigned int cos)
>> >> > +{
>> >> > +    if ( cos > feat->info.cat_info.cos_max &&
>> >> > +         val[0] != feat->cos_reg_val[0] )
>> >> > +            /*
>> >> > +             * Exceed cos_max and value to set is not default,
>> >> > +             * return error.
>> >> > +             */
>> >> > +            return false;
>> >> > +
>> >> > +    return true;
>> >> > +}
>> >> 
>> >> Same here - with cos_max moved out, the hook would seem to
>> >> become unnecessary.
>> >> 
>> > As explanation in previous patch, CDP has different behavior.
>> > static bool l3_cdp_fits_cos_max(...)
>> > {
>> >     if ( cos > feat->info.cat_info.cos_max &&
>> >          (val[0] != get_cdp_data(feat, 0) || val[1] != get_cdp_code(feat, 0)) )
>> >             /*
>> >              * Exceed cos_max and value to set is not default,
>> >              * return error.
>> >              */
>> >             return false;
>> > 
>> >     return true;
>> > 
>> > }
>> 
>> As said in reply, by making get_val() flexible enough you should
>> be able to avoid this.
> 
> Sorry, I am confused here. 'fits_cos_max' is called during set value process.
> Why "making get_val() flexible enough" can avoid this?

Because that would then replace the get_cdp_data() and
get_cdp_code() calls above.

Jan

Yi Sun March 28, 2017, 11:59 a.m. UTC | #6

On 17-03-28 04:40:41, Jan Beulich wrote:
> >>> On 28.03.17 at 12:31, <[e-mail]> wrote:
> > On 17-03-28 02:45:13, Jan Beulich wrote:
> >> >>> On 28.03.17 at 06:58, <[e-mail]> wrote:
> >> > On 17-03-27 04:37:37, Jan Beulich wrote:
> >> >> >>> On 16.03.17 at 12:08, <[e-mail]> wrote:
> >> >> > +static bool cat_fits_cos_max(const uint32_t val[],
> >> >> > +                             const struct feat_node *feat,
> >> >> > +                             unsigned int cos)
> >> >> > +{
> >> >> > +    if ( cos > feat->info.cat_info.cos_max &&
> >> >> > +         val[0] != feat->cos_reg_val[0] )
> >> >> > +            /*
> >> >> > +             * Exceed cos_max and value to set is not default,
> >> >> > +             * return error.
> >> >> > +             */
> >> >> > +            return false;
> >> >> > +
> >> >> > +    return true;
> >> >> > +}
> >> >> 
> >> >> Same here - with cos_max moved out, the hook would seem to
> >> >> become unnecessary.
> >> >> 
> >> > As explanation in previous patch, CDP has different behavior.
> >> > static bool l3_cdp_fits_cos_max(...)
> >> > {
> >> >     if ( cos > feat->info.cat_info.cos_max &&
> >> >          (val[0] != get_cdp_data(feat, 0) || val[1] != get_cdp_code(feat, 0)) )
> >> >             /*
> >> >              * Exceed cos_max and value to set is not default,
> >> >              * return error.
> >> >              */
> >> >             return false;
> >> > 
> >> >     return true;
> >> > 
> >> > }
> >> 
> >> As said in reply, by making get_val() flexible enough you should
> >> be able to avoid this.
> > 
> > Sorry, I am confused here. 'fits_cos_max' is called during set value process.
> > Why "making get_val() flexible enough" can avoid this?
> 
> Because that would then replace the get_cdp_data() and
> get_cdp_code() calls above.
> 
I think we at least need a 'get_val()' hook. I try to implement CAT/CDP hook.
Please help to check if this is what you thought.

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);
}

static bool fits_cos_max(const uint32_t val[],
                         uint32_t array_len,
                         const struct psr_socket_info *info,
                         unsigned int cos)
{
    unsigned int i, j;
    const struct feat_node *feat;
    uint32_t default_val;
	
    for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ )
    {
        if ( !info->features[i] )
            continue;

        feat = info->features[i];

        if ( array_len < feat->cos_num )
            return false;
	
        /* Move the fits_cos_max() hook content into caller here. */
        if ( cos > feat->props->cos_max )
        {
            for ( j = 0; j < feat->cos_num; j++ )
            {
                /* cos_reg_val[0] always stores the default value so set cos to 0. */
                feat->props->get_val(feat, 0, 0, j, &default_val);
                if ( val[j] != default_val )
                    return false;
            }
        }

        val += feat->cos_num;
    }

    return true;
}

Jan Beulich March 28, 2017, 12:20 p.m. UTC | #7

>>> On 28.03.17 at 13:59, <[e-mail]> 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?

Jan

Yi Sun March 29, 2017, 1:20 a.m. UTC | #8

On 17-03-28 06:20:48, Jan Beulich wrote:
> >>> On 28.03.17 at 13:59, <[e-mail]> 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.

> Jan

Yi Sun March 29, 2017, 1:36 a.m. UTC | #9

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, <[e-mail]> 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.

> > Jan
> 
> _______________________________________________
> Xen-devel mailing list
> [e-mail]
> https://lists.xen.org/xen-devel

Jan Beulich March 29, 2017, 9:57 a.m. UTC | #10

>>> On 29.03.17 at 03:36, <[e-mail]> 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, <[e-mail]> 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.

Jan

Yi Sun March 30, 2017, 1:37 a.m. UTC | #11

On 17-03-29 03:57:52, Jan Beulich wrote:
> >>> On 29.03.17 at 03:36, <[e-mail]> 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, <[e-mail]> 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?

Yi Sun March 30, 2017, 1:39 a.m. UTC | #12

On 17-03-30 09:37:33, Yi Sun wrote:
> On 17-03-29 03:57:52, Jan Beulich wrote:
> > >>> On 29.03.17 at 03:36, <[e-mail]> 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, <[e-mail]> 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?

Sorry, a mistake, forgot to change 'flag' to 'type' in 'l3_cdp_get_val'.

Jan Beulich March 30, 2017, 11:55 a.m. UTC | #13

>>> On 30.03.17 at 03:37, <[e-mail]> wrote:
> On 17-03-29 03:57:52, Jan Beulich wrote:
>> >>> On 29.03.17 at 03:36, <[e-mail]> 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, <[e-mail]> 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?

Jan

Yi Sun March 30, 2017, 12:10 p.m. UTC | #14

On 17-03-30 05:55:52, Jan Beulich wrote:
> >>> On 30.03.17 at 03:37, <[e-mail]> wrote:
> > On 17-03-29 03:57:52, Jan Beulich wrote:
> >> >>> On 29.03.17 at 03:36, <[e-mail]> 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, <[e-mail]> 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

Thanks,
Sun Yi

Jan Beulich March 31, 2017, 8:47 a.m. UTC | #15

>>> On 30.03.17 at 14:10, <[e-mail]> wrote:
> On 17-03-30 05:55:52, Jan Beulich wrote:
>> >>> On 30.03.17 at 03:37, <[e-mail]> wrote:
>> > On 17-03-29 03:57:52, Jan Beulich wrote:
>> >> >>> On 29.03.17 at 03:36, <[e-mail]> 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, <[e-mail]> 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
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.

The checks done first in what is currently *_set_new_val() also
look like they could be moved into the (generic) caller(s), making
clear that at least for now even cbm_len (just like suggested for
cos_max) should be a generic rather than unionized field in
struct feat_node. In the worst case a new check_val() hook
might be needed.

Jan

Yi Sun March 31, 2017, 9:12 a.m. UTC | #16

On 17-03-31 02:47:25, Jan Beulich wrote:
> >>> On 30.03.17 at 14:10, <[e-mail]> wrote:
> > On 17-03-30 05:55:52, Jan Beulich wrote:
> >> >>> On 30.03.17 at 03:37, <[e-mail]> wrote:
> >> > On 17-03-29 03:57:52, Jan Beulich wrote:
> >> >> >>> On 29.03.17 at 03:36, <[e-mail]> 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, <[e-mail]> 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?

Yi Sun March 31, 2017, 9:18 a.m. UTC | #17

On 17-03-31 17:12:36, Yi Sun wrote:
> On 17-03-31 02:47:25, Jan Beulich wrote:
> > >>> On 30.03.17 at 14:10, <[e-mail]> wrote:
> > > On 17-03-30 05:55:52, Jan Beulich wrote:
> > >> >>> On 30.03.17 at 03:37, <[e-mail]> wrote:
> > >> > On 17-03-29 03:57:52, Jan Beulich wrote:
> > >> >> >>> On 29.03.17 at 03:36, <[e-mail]> 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, <[e-mail]> 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?

Of course, for cos_num=1 features, 'get_vals' is NULL and 'get_val'
will be used.

Jan Beulich March 31, 2017, 10:19 a.m. UTC | #18

>>> On 31.03.17 at 11:12, <[e-mail]> wrote:
> On 17-03-31 02:47:25, Jan Beulich wrote:
>> >>> On 30.03.17 at 14:10, <[e-mail]> wrote:
>> > On 17-03-30 05:55:52, Jan Beulich wrote:
>> >> >>> On 30.03.17 at 03:37, <[e-mail]> wrote:
>> >> > On 17-03-29 03:57:52, Jan Beulich wrote:
>> >> >> >>> On 29.03.17 at 03:36, <[e-mail]> 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, <[e-mail]> 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

Yi Sun March 31, 2017, 12:40 p.m. UTC | #19

On 17-03-31 04:19:49, Jan Beulich wrote:
> >>> On 31.03.17 at 11:12, <[e-mail]> wrote:
> > On 17-03-31 02:47:25, Jan Beulich wrote:
> >> >>> On 30.03.17 at 14:10, <[e-mail]> wrote:
> >> > On 17-03-30 05:55:52, Jan Beulich wrote:
> >> >> >>> On 30.03.17 at 03:37, <[e-mail]> wrote:
> >> >> > On 17-03-29 03:57:52, Jan Beulich wrote:
> >> >> >> >>> On 29.03.17 at 03:36, <[e-mail]> 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, <[e-mail]> 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);
...
}

Jan Beulich March 31, 2017, 12:51 p.m. UTC | #20

>>> On 31.03.17 at 14:40, <[e-mail]> wrote:
> On 17-03-31 04:19:49, Jan Beulich wrote:
>> >>> On 31.03.17 at 11:12, <[e-mail]> wrote:
>> > On 17-03-31 02:47:25, Jan Beulich wrote:
>> >> >>> On 30.03.17 at 14:10, <[e-mail]> 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?

Jan

Yi Sun March 31, 2017, 1:22 p.m. UTC | #21

On 17-03-31 06:51:07, Jan Beulich wrote:
> >>> On 31.03.17 at 14:40, <[e-mail]> wrote:
> > On 17-03-31 04:19:49, Jan Beulich wrote:
> >> >>> On 31.03.17 at 11:12, <[e-mail]> wrote:
> >> > On 17-03-31 02:47:25, Jan Beulich wrote:
> >> >> >>> On 30.03.17 at 14:10, <[e-mail]> 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]'. Right?

Jan Beulich March 31, 2017, 2:35 p.m. UTC | #22

>>> On 31.03.17 at 15:22, <[e-mail]> wrote:
> On 17-03-31 06:51:07, Jan Beulich wrote:
>> >>> On 31.03.17 at 14:40, <[e-mail]> wrote:
>> > On 17-03-31 04:19:49, Jan Beulich wrote:
>> >> >>> On 31.03.17 at 11:12, <[e-mail]> wrote:
>> >> > On 17-03-31 02:47:25, Jan Beulich wrote:
>> >> >> >>> On 30.03.17 at 14:10, <[e-mail]> 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").

Jan

Yi Sun March 31, 2017, 2:46 p.m. UTC | #23

On 17-03-31 08:35:14, Jan Beulich wrote:
> >>> On 31.03.17 at 15:22, <[e-mail]> wrote:
> > On 17-03-31 06:51:07, Jan Beulich wrote:
> >> >>> On 31.03.17 at 14:40, <[e-mail]> wrote:
> >> > On 17-03-31 04:19:49, Jan Beulich wrote:
> >> >> >>> On 31.03.17 at 11:12, <[e-mail]> wrote:
> >> >> > On 17-03-31 02:47:25, Jan Beulich wrote:
> >> >> >> >>> On 30.03.17 at 14:10, <[e-mail]> 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

Patch

diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index f2c2614..d4db407 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -136,6 +136,19 @@  struct feat_node {
          */
         int (*compare_val)(const uint32_t val[], const struct feat_node *feat,
                            unsigned int cos);
+
+        /*
+         * fits_cos_max is used to check if the input cos id exceeds the
+         * feature's cos_max and if the input value is not the default one.
+         * Even if the associated cos exceeds the cos_max, HW can work with
+         * default value. That is the reason we need check if input value is
+         * default one. If both criteria are fulfilled, that means the input
+         * exceeds the range. If not, that means the input fits the
+         * requirements.
+         */
+        bool (*fits_cos_max)(const uint32_t val[],
+                             const struct feat_node *feat,
+                             unsigned int cos);
     } ops;
 
     /* Encapsulate feature specific HW info here. */
@@ -392,6 +405,21 @@  static int cat_compare_val(const uint32_t val[],
     return 0;
 }
 
+static bool cat_fits_cos_max(const uint32_t val[],
+                             const struct feat_node *feat,
+                             unsigned int cos)
+{
+    if ( cos > feat->info.cat_info.cos_max &&
+         val[0] != feat->cos_reg_val[0] )
+            /*
+             * Exceed cos_max and value to set is not default,
+             * return error.
+             */
+            return false;
+
+    return true;
+}
+
 /* L3 CAT ops */
 static const struct feat_ops l3_cat_ops = {
     .get_cos_max = cat_get_cos_max,
@@ -400,6 +428,7 @@  static const struct feat_ops l3_cat_ops = {
     .get_old_val = cat_get_old_val,
     .set_new_val = cat_set_new_val,
     .compare_val = cat_compare_val,
+    .fits_cos_max = cat_fits_cos_max,
 };
 
 static void __init parse_psr_bool(char *s, char *value, char *feature,
@@ -851,13 +880,81 @@  static int find_cos(const uint32_t val[], uint32_t array_len,
     return -ENOENT;
 }
 
+static bool fits_cos_max(const uint32_t val[],
+                         uint32_t array_len,
+                         const struct psr_socket_info *info,
+                         unsigned int cos)
+{
+    unsigned int ret, i;
+    const struct feat_node *feat;
+
+    for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ )
+    {
+        if ( !info->features[i] )
+            continue;
+
+        feat = info->features[i];
+
+        ret = feat->ops.fits_cos_max(val, feat, cos);
+        if ( !ret )
+            return false;
+
+        array_len -= feat->cos_num;
+        if ( array_len < 0 )
+            return false;
+
+        val += feat->cos_num;
+    }
+
+    return true;
+}
+
 static int pick_avail_cos(const struct psr_socket_info *info,
                           const uint32_t val[], uint32_t array_len,
                           unsigned int old_cos,
                           enum psr_feat_type feat_type)
 {
+    unsigned int cos;
+    unsigned int cos_max = 0;
+    const struct feat_node *feat;
+    const unsigned int *ref = info->cos_ref;
+
     ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock)));
-    return -ENOENT;
+
+    /* cos_max is the one of the feature which is being set. */
+    feat = info->features[feat_type];
+    if ( !feat )
+        return -ENOENT;
+
+    cos_max = feat->ops.get_cos_max(feat);
+    if ( !cos_max )
+        return -ENOENT;
+
+    /*
+     * If old cos is referred only by the domain, then use it. And, we cannot
+     * use id 0 because it stores the default values.
+     */
+    if ( old_cos && ref[old_cos] == 1 &&
+         fits_cos_max(val, array_len, info, old_cos) )
+            return old_cos;
+
+    /* Find an unused one other than cos0. */
+    for ( cos = 1; cos <= cos_max; cos++ )
+    {
+        /*
+         * ref is 0 means this COS is not used by other domain and
+         * can be used for current setting.
+         */
+        if ( !ref[cos] )
+        {
+            if ( !fits_cos_max(val, array_len, info, cos) )
+                return -EOVERFLOW;
+
+            return cos;
+        }
+    }
+
+    return -EOVERFLOW;
 }
 
 static int write_psr_msr(unsigned int socket, unsigned int cos,