Re: [PATCH v5 1/1] sched/uclamp: add SCHED_FLAG_UTIL_CLAMP_RESET flag to reset uclamp

[Peter Zijlstra]

On Fri, Nov 06, 2020 at 11:36:48AM +0100, Patrick Bellasi wrote:

> > +static int uclamp_reset(enum uclamp_id clamp_id, unsigned long flags)
> > +{
> > +	/* No _UCLAMP_RESET flag set: do not reset */
> > +	if (!(flags & SCHED_FLAG_UTIL_CLAMP_RESET))
> > +		return false;
> > +
> > +	/* Only _UCLAMP_RESET flag set: reset both clamps */
> > +	if (!(flags & (SCHED_FLAG_UTIL_CLAMP_MIN | SCHED_FLAG_UTIL_CLAMP_MAX)))
> > +		return true;
> > +
> > +	/* Both _UCLAMP_RESET and _UCLAMP_MIN flags are set: reset only min */
> > +	if ((flags & SCHED_FLAG_UTIL_CLAMP_MIN) && clamp_id == UCLAMP_MIN)
> > +		return true;
> > +
> > +	/* Both _UCLAMP_RESET and _UCLAMP_MAX flags are set: reset only max */
> > +	if ((flags & SCHED_FLAG_UTIL_CLAMP_MAX) && clamp_id == UCLAMP_MAX)
> > +		return true;
> > +
> > +	return false;
> 
> I was suggesting maybe we need READ_ONCE() in the if above but did not
> addressed previous Dietmar's question [2] on why.
> 
> The function above has a correct semantics only when the ordering of the
> if statement is strictly observed.
> 
> Now, since we don't have any data dependency on the multiple if cases,
> are we sure an overzealous compiler will never come up with some
> "optimized reordering" of the checks required?
> 
> IOW, if the compiler could scramble the checks on an optimization, we
> would get a wrong semantics which is also very difficult do debug.
> Hence the idea to use READ_ONCE, to both tell the compiler to not
> even attempt reordering those checks and also to better document the
> code about the importance of the ordering on those checks.
> 
> Is now more clear? Does that makes sense?

I don't think the compiler is allowed to do as you fear. Specifically it
cannot move the first branch down because that would change the meaning
of this function and affect observable behaviour even in the traditional
single-threaded environment.