Messages in this thread | | | Date | Tue, 30 Jun 2020 21:06:43 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH v6 2/2] sched/uclamp: Protect uclamp fast path code with static key |
| |
On Tue, Jun 30, 2020 at 06:55:02PM +0100, Qais Yousef wrote: > On 06/30/20 19:07, Peter Zijlstra wrote:
> > There's a fun race described in 9107c89e269d ("perf: Fix race between > > event install and jump_labels"), are we sure this isn't also susceptible > > to something similar? > > > > I suspect not, but I just wanted to make sure. > > IIUC, the worry is that not all CPUs might have observed the change in the > static key state; hence could not be running the patched > enqueue/dequeue_task(), so we could end up with some CPUs accounting for > uclamp in the enqueue/dequeue path but not others? > > I was hoping this synchronization is guaranteed by the static_branch_*() call.
It is, that isn't quite the the problem. Looking at it more I think commit 1dbb6704de91 ("jump_label: Fix concurrent static_key_enable/disable()") fixed some of it.
From what I can remember there were two parts to this problem, one being fixed by the above commit, the other being that if we enable while a task is running we miss the switch-in event (exactly how in this patch we miss the enqueue).
Due to the missing switch-in, the state is 'weird' and the subsequent IPI to install a remote event didn't quite work.
So I put that sync_sched() call in to guarantee all CPUs have done a schedule() cycle after having the key switched. This makes sure that every running task has seen the switch-in and thus the state is as expected.
But like I said, I think we're good, that one extra branch deals with the half-state.
| |