Messages in this thread | | | From | Valentin Schneider <> | Subject | Re: [PATCH 2/2] sched/uclamp: Protect uclamp fast path code with static key | Date | Fri, 19 Jun 2020 14:25:20 +0100 |
| |
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@suse.de/ >> > >> >> 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 strongly against extra debug code just to be safe. It ends up with > confusion down the line and extra complexity, and since this is the hot path > maybe potential extra variables to mess with cache behaviors. >
Hence why I'd put this under CONFIG_SCHED_DEBUG.
> Thanks
| |