Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Qais Yousef <firstname.lastname@example.org>
To: Peter Zijlstra <email@example.com>
Cc: Ingo Molnar <firstname.lastname@example.org>,
Doug Anderson <email@example.com>,
Jonathan Corbet <firstname.lastname@example.org>,
Juri Lelli <email@example.com>,
Vincent Guittot <firstname.lastname@example.org>,
Dietmar Eggemann <email@example.com>,
Steven Rostedt <firstname.lastname@example.org>,
Ben Segall <email@example.com>, Mel Gorman <firstname.lastname@example.org>,
Luis Chamberlain <email@example.com>,
Kees Cook <firstname.lastname@example.org>,
Iurii Zaikin <email@example.com>,
Quentin Perret <firstname.lastname@example.org>,
Valentin Schneider <email@example.com>,
Patrick Bellasi <firstname.lastname@example.org>,
Pavan Kondeti <email@example.com>,
Subject: Re: [PATCH v6 1/2] sched/uclamp: Add a new sysctl to control RT default boost value
Date: Mon, 13 Jul 2020 15:27:55 +0100 [thread overview]
Message-ID: <firstname.lastname@example.org> (raw)
On 07/13/20 15:35, Peter Zijlstra wrote:
> On Mon, Jul 13, 2020 at 01:12:46PM +0100, Qais Yousef wrote:
> > On 07/13/20 13:21, Peter Zijlstra wrote:
> > > It's monday, and I cannot get my brain working.. I cannot decipher the
> > > comments you have with the smp_[rw]mb(), what actual ordering do they
> > > enforce?
> > It was a bit of a paranoia to ensure that readers on other cpus see the new
> > value after this point.
> IIUC that's not something any barrier can provide.
> Barriers can only order between (at least) two memory operations:
> X = 1; y = Y;
> smp_wmb(); smp_rmb();
> Y = 1; x = X;
> guarantees that if y == 1, then x must also be 1. Because the left hand
> side orders the store of Y after the store of X, while the right hand
> side order the load of X after the load of Y. Therefore, if the first
> load observes the last store, the second load must observe the first
> Without a second variable, barriers can't guarantee _anything_. Which is
> why any barrier comment should refer to at least two variables.
Hmmm okay. I thought this will order the write with the read. In my head if,
for example, the write was stuck in the write buffer of CPU0, then a read on
CPU1 would wait for this to be committed before carrying on and issue a read.
So I was reading this as don't issue new reads before current writes are done.
I need to go back and read memory-barriers.rst. It's been 10 years since I last
> > > Also, your synchronize_rcu() relies on write_lock() beeing
> > > non-preemptible, which isn't true on PREEMPT_RT.
> > >
> > > The below seems simpler...
> > Hmm maybe I am missing something obvious, but beside the race with fork; I was
> > worried about another race and that's what the synchronize_rcu() is trying to
> > handle.
> > It's the classic preemption in the middle of RMW operation race.
> > copy_process() sysctl_uclamp
> > sched_post_fork()
> > __uclamp_sync_rt()
> > // read sysctl
> > // PREEMPT
> > for_each_process_thread()
> > // RESUME
> > // write syctl to p
> > 2. sysctl_uclamp happens *during* sched_post_fork()
> > There's the risk of the classic preemption in the middle of RMW where another
> > CPU could have changed the shared variable after the current CPU has already
> > read it, but before writing it back.
> Aah.. I see.
> > I protect this with rcu_read_lock() which as far as I know synchronize_rcu()
> > will ensure if we do the update during this section; we'll wait for it to
> > finish. New forkees entering the rcu_read_lock() section will be okay because
> > they should see the new value.
> > spinlocks() and mutexes seemed inferior to this approach.
> Well, didn't we just write in another patch that p->uclamp_* was
> protected by both rq->lock and p->pi_lock?
__setscheduler_uclamp() path is holding these locks, not sure by design or it
just happened this path holds the lock. I can't see the lock in the
uclamp_fork() path. But it's hard sometimes to unfold the layers of callers,
especially not all call sites are annotated for which lock is assumed to be
Is it safe to hold the locks in uclamp_fork() while the task is still being
created? My new code doesn't hold it of course.
We can enforce this rule if you like. Though rcu critical section seems lighter
weight to me.
If all of this does indeed start looking messy we can put the update in
a delayed worker and schedule that instead of doing synchronous setup.
Or go back to task_woken_rt() with a cached per-rq variable of
sysctl_util_min_rt that is more likely to be cache hot compared to the global
next prev parent reply other threads:[~2020-07-13 14:28 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-06 14:28 [PATCH v6 0/2] sched/uclamp: new sysctl for default RT " Qais Yousef
2020-07-06 14:28 ` [PATCH v6 1/2] sched/uclamp: Add a new sysctl to control RT default " Qais Yousef
2020-07-06 15:49 ` Valentin Schneider
2020-07-07 9:34 ` Qais Yousef
2020-07-07 11:30 ` Valentin Schneider
2020-07-07 12:36 ` Qais Yousef
2020-07-08 11:05 ` Valentin Schneider
2020-07-08 13:08 ` Qais Yousef
2020-07-08 21:45 ` Valentin Schneider
2020-07-07 11:39 ` Valentin Schneider
2020-07-07 12:58 ` Qais Yousef
2020-07-13 11:21 ` Peter Zijlstra
2020-07-13 11:36 ` peterz
2020-07-13 12:12 ` Qais Yousef
2020-07-13 13:35 ` Peter Zijlstra
2020-07-13 14:27 ` Qais Yousef [this message]
2020-07-13 16:54 ` Peter Zijlstra
2020-07-13 18:09 ` Qais Yousef
2020-07-06 14:28 ` [PATCH v6 2/2] Documentation/sysctl: Document uclamp sysctl knobs Qais Yousef
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--subject='Re: [PATCH v6 1/2] sched/uclamp: Add a new sysctl to control RT default boost value' \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).