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: <20200624172605.26715-1-qais.yousef@arm.com> <20200624172605.26715-3-qais.yousef@arm.com> <jhj5zbgroct.mognet@arm.com> <20200625110006.q3iepcrh2uh4oizv@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 <chris.redpath@arm.com>,
        Lukasz Luba <lukasz.luba@arm.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/2] sched/uclamp: Protect uclamp fast path code with static key
Message-ID: <jhjpn9ngzlx.mognet@arm.com>
In-reply-to: <20200625110006.q3iepcrh2uh4oizv@e107158-lin.cambridge.arm.com>
Date:   Thu, 25 Jun 2020 12:26:04 +0100
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 25/06/20 12:00, Qais Yousef wrote:
> Hi Valentin
>
> On 06/25/20 01:16, Valentin Schneider wrote:
>> In schedutil_cpu_util(), when uclamp isn't compiled it, we have an explicit
>> 'goto max'. When uclamp *is* compiled in, that's taken care of by the
>> "natural" RT uclamp aggregation... Which doesn't happen until we flip the
>> static key.
>>
>> It's yucky, but if you declare the key in the internal sched header, you
>> could reuse it in the existing 'goto max' (or sysctl value, when we make
>> that tweakable) path.
>
> Not sure if this is the best way forward. I need to think about it.
> While I am not keen on enabling in kernel users of util clamp, but there was
> already an attempt to do so. This approach will not allow us to implement
> something in the future for that. Which maybe is what we want..
>

Just to be clear, I'm not suggesting to add any in-kernel toggling of
uclamp outside of the scheduler: by keeping that to the internal sched
header & schedutil, we're keeping it contained to internal scheduler land.

Since a diff is worth a thousand words, here's what I was thinking of (not
even compiled):
---
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 7fbaee24c824..68731c3316ef 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -210,7 +210,7 @@ unsigned long schedutil_cpu_util(int cpu, unsigned long util_cfs,
        unsigned long dl_util, util, irq;
        struct rq *rq = cpu_rq(cpu);

-	if (!IS_BUILTIN(CONFIG_UCLAMP_TASK) &&
+	if ((!IS_BUILTIN(CONFIG_UCLAMP_TASK) || !static_branch_likely(&sched_uclamp_used)) &&
            type == FREQUENCY_UTIL && rt_rq_is_runnable(&rq->rt)) {
                return max;
        }
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 1d4e94c1e5fe..3fd5c792f247 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1638,6 +1638,7 @@ static const_debug __maybe_unused unsigned int sysctl_sched_features =

 extern struct static_key_false sched_numa_balancing;
 extern struct static_key_false sched_schedstats;
+extern struct static_key_false sched_uclamp_used;

 static inline u64 global_rt_period(void)
 {
---

>> > + * As soon as userspace modifies any of the uclamp knobs, the static key is
>> > + * enabled, since we have an actual users that make use of uclamp
>> > + * functionality.
>> > + *
>> > + * The knobs that would enable this static key are:
>> > + *
>> > + *   * A task modifying its uclamp value with sched_setattr().
>>
>> That one makes it not just userspace, right? While the sched_setattr()
>> stuff is expected to be unexported, it isn't ATM and we may expect some
>> modules to ask for a uclamp API eventually.
>
> This has already come up with another thread [1]. I think in-kernel users
> shouldn't go through this path. I will propose something to give stronger
> guarantees in this regard.
>

True; and they'll have to go through another path anyway once the unexport
thing happens.

>> > -	if (update_root_tg)
>> > +	if (update_root_tg) {
>> >            uclamp_update_root_tg();
>> > +		static_branch_enable(&sched_uclamp_used);
>>
>> I don't think it matters ATM, but shouldn't we flip that *before* updating
>> the TG's to avoid any future surprises?
>
> What sort of surprises are you thinking of?
>

Say if we end up adding static key checks in some other uclamp functions
(which are called in the TG update) and don't change this here, someone
will have to scratch their heads to figure out the key enablement needs to
be moved one line higher. It's harmless future-proofing, I think.

> Thanks