Re: [PATCH 2/2] sched/uclamp: Protect uclamp fast path code with static key

[Qais Yousef]

On 06/19/20 14:25, Valentin Schneider wrote:
> 
> On 19/06/20 13:51, Qais Yousef wrote:
> > On 06/19/20 11:36, Valentin Schneider wrote:
> >>
> >> On 18/06/20 20:55, Qais Yousef wrote:
> >> > There is a report that when uclamp is enabled, a netperf UDP test
> >> > regresses compared to a kernel compiled without uclamp.
> >> >
> >> > https://lore.kernel.org/lkml/20200529100806.GA3070@xxxxxxx/
> >> >
> >>
> >> ISTR the perennial form for those is: https://lkml.kernel.org/r/<message-id>
> >
> > The link is correct permalinnk from lore and contains the message-id as Peter
> > likes and he has accepted this form before.
> >
> 
> I think the objections I remember were on using lkml.org rather than
> lkml.kernel.org. Sorry!
> 
> > If you look closely you'll see that what you suggest is just moving 'lkml' to
> > replace lore in the dns name and put an /r/. I don't see a need to enforce one
> > form over the other as the one I used is much easier to get.
> >
> 
> My assumption would be that while lore may fade (it hasn't been there for
> that long, who knows what will come next), lkml.kernel.org ought to be
> perennial. Keyword here being "assumption".
> 
> > If Peter really insists I'll be happy to change.
> >
> > [...]
> >
> >> > +	 * This could happen if sched_uclamp_unused was disabled while the
> >> > +	 * current task was running, hence we could end up with unbalanced call
> >> > +	 * to uclamp_rq_dec_id().
> >> > +	 */
> >> > +	if (unlikely(!bucket->tasks))
> >> > +		return;
> >>
> >> I'm slightly worried about silent returns for cases like these, can we try
> >> to cook something up to preserve the previous SCHED_WARN_ON()? Say,
> >> something like the horrendous below - alternatively might be feasible with
> >> with some clever p->on_rq flag.
> >
> > I am really against extra churn and debug code to detect an impossible case
> > that is not really tricky for reviewers to discern. Outside of enqueue/dequeue
> > path, it's only used in update_uclamp_active(). It is quite easy to see that
> > it's impossible, except for the legit case now when we have a static key
> > changing when a task is running.
> >
> 
> Providing it isn't too much of a head scratcher (and admittedly what I am
> suggesting is borderline here), I believe it is worthwhile to add debug
> helps in what is assumed to be impossible cases - even more so in this case
> seeing as it had been deemed worth to check previously. We've been proved
> wrong on the "impossible" nature of some things before.
> 
> We have a few of those checks strewn over the scheduler code, so it's not
> like we would be starting a new trend.

I am sorry I am still not bought in. I think the parts you're talking about are
in the lockless part of the scheduler which are really hard to debug as several
cpus could be traversing these data from different code paths. But here this is
just extra churn.

If an imbalance has happend this means either:

	1. enqueue/dequeue_task() is imablanced itself
	2. uclamp_update_active() calls dec without inc.

If 1 happened we have more reasons to be worried about. For 2 the function
takes task_rq_lock() and does dec/inc in obvious way.

So I don't see any reason to add new info in task_struct and sprinkle #ifdefs
to protect against something that I can't see we can't reason correctly about
now.

We don't use pr_debug() in scheduler (I guess no computer would have booted
with that on), otherwise that would have been a good candidate for one, yes.
But we can't do that.

Thanks

--
Qais Yousef