Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
References: <20200618195525.7889-1-qais.yousef@arm.com> <20200618195525.7889-3-qais.yousef@arm.com> <jhjwo43cpfl.mognet@arm.com> <20200619125148.y4cq3hwllgozbutq@e107158-lin.cambridge.arm.com>
User-agent: mu4e 0.9.17; emacs 26.3
From:   Valentin Schneider <valentin.schneider@arm.com>
To:     Qais Yousef <qais.yousef@arm.com>
Cc:     Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
        Patrick Bellasi <patrick.bellasi@matbug.net>,
        Chris Redpath <chrid.redpath@arm.com>,
        Lukasz Luba <lukasz.luba@arm.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] sched/uclamp: Protect uclamp fast path code with static key
In-reply-to: <20200619125148.y4cq3hwllgozbutq@e107158-lin.cambridge.arm.com>
Date:   Fri, 19 Jun 2020 14:25:20 +0100
Message-ID: <jhjsgerchmn.mognet@arm.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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