Re: V4 [PATCH] Set tunable value as well as min/max values

[Siddhesh Poyarekar <siddhesh@sourceware.org>]

On 29/09/20 18:00, H.J. Lu wrote:
> Here is the updated patch with TUNABLE_SET_BOUNDS_IF_VALID.
> 
> OK for master?

I don't think TUNABLE_SET_BOUNDS_IF_VALID is doing what it intends to do.

> +#define TUNABLE_SET_BOUNDS_IF_VALID(__cur, __maxp, __minp, __type)	      \

minp and maxp are switched around.

> +({									      \
> +  __type min = __minp ? *((__type *) __minp) : (__cur)->type.min;	      \
> +  __type max = __maxp ? *((__type *) __maxp) : (__cur)->type.max;	      \
> +  if (__minp)								      \
> +    {									      \
> +      if (__maxp)							      \
> +	{								      \
> +	  if (max > min)						      \
> +	    {								      \
> +	      (__cur)->type.min = min;					      \
> +	      (__cur)->type.max = max;					      \
> +	    }								      \

When both minp and maxp are specified, it checks if they're sane with
respect to each other but it should also check whether the range they
describe is a *subset* of (__cur)->type.min and (__cur)->type.max.

> +	}								      \
> +      else if (max > min)						      \
> +	(__cur)->type.min = min;					      \

You also need to make sure that (__cur)->type.min < min to ensure a more
restrictive range.

> +    }									      \
> +  else if (max > min)							      \
> +    (__cur)->type.max = min;						      \

I did not understand this one.

Basically, this is what it should look like:

    if (__minp != NULL
        && *__minp <= *__maxp
        && *__minp >= (__cur)->type.min
        && *__minp <= (__cur)->type.max)
      (__cur)->type.min = *__minp;

    if (__maxp != NULL
        && *__minp <= *_maxp
        && *__maxp >= (__cur)->type.min
        && *__maxp <= (__cur)->type.max)
      (__cur)->type.max = *maxp;

You could collapse some of these conditions if we assume that maxp and
minp are always either NULL or not NULL together.

Siddhesh