Re: [PATCH v6 1/2] sched/uclamp: Add a new sysctl to control RT default boost value

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>,
	Doug Anderson <dianders@chromium.org>,
	Jonathan Corbet <corbet@lwn.net>,
	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>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Kees Cook <keescook@chromium.org>,
	Iurii Zaikin <yzaikin@google.com>,
	Quentin Perret <qperret@google.com>,
	Patrick Bellasi <patrick.bellasi@matbug.net>,
	Pavan Kondeti <pkondeti@codeaurora.org>,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v6 1/2] sched/uclamp: Add a new sysctl to control RT default boost value
Date: Wed, 08 Jul 2020 22:45:12 +0100	[thread overview]
Message-ID: <jhjv9ix7kbn.mognet@arm.com> (raw)
In-Reply-To: <20200708130831.4oaukv65hbano3j7@e107158-lin>

On 08/07/20 14:08, Qais Yousef wrote:
> On 07/08/20 12:05, Valentin Schneider wrote:
>> > AFAIU rcu_read_lock() is light weight. So having the protection applied is more
>> > robust against future changes.
>>
>> So I think the one thing you win by having this dance with mb's and the
>> suggested handling of the task list is that you do not need any
>> rcu_synchronize() anymore. Both approaches have merit, it's just that the
>> way I understood the suggestion to add sched_post_fork() was to simplify
>> the ordering of the update with the aforementioned scheme.
>
> The synchronize_rcu() is not for sched_post_fork(). It is to deal with the
> preemption problem.
>
>>
>> >
>> >>
>> >> sched_post_fork() being preempted out is a bit more annoying, but what
>> >> prevents us from making that bit preempt-disabled?
>> >
>> > preempt_disable() is not friendly to RT and heavy handed approach IMO.
>> >
>>
>> True, but this is both an infrequent and slow sysctl path, so I don't think
>> RT would care much.
>
> There's an easy answer for that. But first I'm not sure what problem are we
> discussing here.
>
> What is the problem with rcu? And how is preempt_disable() fixes it or improves
> on it?
>

Minimizing the races we have to think and take care of would make this
easier to review and maintain. That's what I was suggesting with getting
entirely rid of the preemption+update issue and having only the
update+forkee race to take care of, which is IMO fairly straightforward to
reason about on its own.

That said, you're driving the thing, and I'm not, so I'll trust your
judgement here.

> Thanks