Re: [Xen-devel] [PATCH v8 20/24] x86: L2 CAT: implement set value flow.

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

On 17-02-28 15:25:39, Roger Pau Monn� wrote:
> On Wed, Feb 15, 2017 at 04:49:35PM +0800, Yi Sun wrote:
> > +static int l2_cat_compare_val(const uint64_t val[],
> > +                              const struct feat_node *feat,
> > +                              unsigned int cos, bool *found)
> > +{
> > +    uint64_t l2_def_cbm;
> > +
> > +    l2_def_cbm = (1ull << feat->info.l2_cat_info.cbm_len) - 1;
> > +
> > +    if ( cos > feat->info.l2_cat_info.cos_max )
> > +    {
> > +        if ( val[0] != l2_def_cbm )
> > +        {
> > +            *found = false;
> > +            return -ENOENT;
> > +        }
> > +        *found = true;
> > +    }
> > +    else
> > +        *found = (val[0] == feat->cos_reg_val[cos]);
> > +
> > +    return 0;
> 
> The logic of this function is kind of weird IMHO, you seem to be able to 
> return
> an error, and also a parameter that indicates success ("found"). Can't this be
> simplified to simply return an error code, and the found parameter removed?
> 
> Roger.

As I explained in previous patch, the value must be default value if the COS ID
exceeds the max ID. If not, we have to return error to exit the whole flow. That
is the reason we return '-ENOENT'.

In find_cos(), compare_val() is called to check if there is already a COS ID
that all features values are same as input. All features in list should be
checked. The 'found' is used record the final result, if all features values are
same as input value array. You can see, after traversal of feature list, we
return the cos if found is true.

Of course, I can change the function to remove 'found' from the parameter.
But is it so necessary? Thanks!

static int find_cos(const uint64_t *val, uint32_t array_len,
...
{
...
    for ( cos = 0; cos <= cos_max; cos++ )
    {
        if ( cos && !ref[cos] )
            continue;

        /* Not found, need find again from beginning. */
        val_tmp = val;
        list_for_each_entry(feat, &info->feat_list, list)
        {
            /*
             * Compare value according to feature list order.
             * We must follow this order because value array is assembled
             * as this order in get_old_set_new().
             */
            ret = feat->ops.compare_val(val_tmp, feat, cos, &found);
            if ( ret < 0 )
                return ret;

            /* If fail to match, go to next cos to compare. */
            if ( !found )
                break;

            val_tmp += feat->ops.get_cos_num(feat);
            if ( val_tmp - val > array_len )
                return -EINVAL;
        }

        /* For this COS ID all entries in the values array did match. Use it. */
        if ( found )
            return cos;
    }
...
}

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