Re: [PATCH RFC] schedutil: Address the r/w ordering race in kthread
On Wed, May 23, 2018 at 1:50 AM, Joel Fernandes (Google)wrote: > Currently there is a race in schedutil code for slow-switch single-CPU > systems. Fix it by enforcing ordering the write to work_in_progress to > happen before the read of next_freq. > > Kthread Sched update > > sugov_work() sugov_update_single() > > lock(); > // The CPU is free to rearrange below > // two in any order, so it may clear > // the flag first and then read next > // freq. Lets assume it does. > work_in_progress = false > >if (work_in_progress) > return; > >sg_policy->next_freq = 0; > freq = sg_policy->next_freq; >sg_policy->next_freq = > real-freq; > unlock(); > > Reported-by: Viresh Kumar > CC: Rafael J. Wysocki > CC: Peter Zijlstra > CC: Ingo Molnar > CC: Patrick Bellasi > CC: Juri Lelli > Cc: Luca Abeni > CC: Todd Kjos > CC: clau...@evidence.eu.com > CC: kernel-t...@android.com > CC: linux...@vger.kernel.org > Signed-off-by: Joel Fernandes (Google) > --- > I split this into separate patch, because this race can also happen in > mainline. > > kernel/sched/cpufreq_schedutil.c | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/kernel/sched/cpufreq_schedutil.c > b/kernel/sched/cpufreq_schedutil.c > index 5c482ec38610..ce7749da7a44 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -401,6 +401,13 @@ static void sugov_work(struct kthread_work *work) > */ > raw_spin_lock_irqsave(_policy->update_lock, flags); > freq = sg_policy->next_freq; > + > + /* > +* sugov_update_single can access work_in_progress without > update_lock, > +* make sure next_freq is read before work_in_progress is set. > +*/ > + smp_mb(); > + This requires a corresponding barrier somewhere else. > sg_policy->work_in_progress = false; > raw_spin_unlock_irqrestore(_policy->update_lock, flags); > > -- Also, as I said I actually would prefer to use the spinlock in the one-CPU case when the kthread is used. I'll have a patch for that shortly.
Re: [PATCH RFC] schedutil: Address the r/w ordering race in kthread
On Wed, May 23, 2018 at 1:50 AM, Joel Fernandes (Google) wrote: > Currently there is a race in schedutil code for slow-switch single-CPU > systems. Fix it by enforcing ordering the write to work_in_progress to > happen before the read of next_freq. > > Kthread Sched update > > sugov_work() sugov_update_single() > > lock(); > // The CPU is free to rearrange below > // two in any order, so it may clear > // the flag first and then read next > // freq. Lets assume it does. > work_in_progress = false > >if (work_in_progress) > return; > >sg_policy->next_freq = 0; > freq = sg_policy->next_freq; >sg_policy->next_freq = > real-freq; > unlock(); > > Reported-by: Viresh Kumar > CC: Rafael J. Wysocki > CC: Peter Zijlstra > CC: Ingo Molnar > CC: Patrick Bellasi > CC: Juri Lelli > Cc: Luca Abeni > CC: Todd Kjos > CC: clau...@evidence.eu.com > CC: kernel-t...@android.com > CC: linux...@vger.kernel.org > Signed-off-by: Joel Fernandes (Google) > --- > I split this into separate patch, because this race can also happen in > mainline. > > kernel/sched/cpufreq_schedutil.c | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/kernel/sched/cpufreq_schedutil.c > b/kernel/sched/cpufreq_schedutil.c > index 5c482ec38610..ce7749da7a44 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -401,6 +401,13 @@ static void sugov_work(struct kthread_work *work) > */ > raw_spin_lock_irqsave(_policy->update_lock, flags); > freq = sg_policy->next_freq; > + > + /* > +* sugov_update_single can access work_in_progress without > update_lock, > +* make sure next_freq is read before work_in_progress is set. > +*/ > + smp_mb(); > + This requires a corresponding barrier somewhere else. > sg_policy->work_in_progress = false; > raw_spin_unlock_irqrestore(_policy->update_lock, flags); > > -- Also, as I said I actually would prefer to use the spinlock in the one-CPU case when the kthread is used. I'll have a patch for that shortly.
Re: [PATCH RFC] schedutil: Address the r/w ordering race in kthread
Hi Joel, On 22/05/18 16:50, Joel Fernandes (Google) wrote: > Currently there is a race in schedutil code for slow-switch single-CPU > systems. Fix it by enforcing ordering the write to work_in_progress to > happen before the read of next_freq. > > Kthread Sched update > > sugov_work()sugov_update_single() > > lock(); > // The CPU is free to rearrange below > // two in any order, so it may clear > // the flag first and then read next > // freq. Lets assume it does. > work_in_progress = false > >if (work_in_progress) > return; > >sg_policy->next_freq = 0; > freq = sg_policy->next_freq; >sg_policy->next_freq = > real-freq; > unlock(); > > Reported-by: Viresh Kumar> CC: Rafael J. Wysocki > CC: Peter Zijlstra > CC: Ingo Molnar > CC: Patrick Bellasi > CC: Juri Lelli > Cc: Luca Abeni > CC: Todd Kjos > CC: clau...@evidence.eu.com > CC: kernel-t...@android.com > CC: linux...@vger.kernel.org > Signed-off-by: Joel Fernandes (Google) > --- > I split this into separate patch, because this race can also happen in > mainline. > > kernel/sched/cpufreq_schedutil.c | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/kernel/sched/cpufreq_schedutil.c > b/kernel/sched/cpufreq_schedutil.c > index 5c482ec38610..ce7749da7a44 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -401,6 +401,13 @@ static void sugov_work(struct kthread_work *work) >*/ > raw_spin_lock_irqsave(_policy->update_lock, flags); > freq = sg_policy->next_freq; > + > + /* > + * sugov_update_single can access work_in_progress without update_lock, > + * make sure next_freq is read before work_in_progress is set. s/set/reset/ > + */ > + smp_mb(); > + Also, doesn't this need a corresponding barrier (I guess in sugov_should_update_freq)? That being a wmb and this a rmb? Best, - Juri
Re: [PATCH RFC] schedutil: Address the r/w ordering race in kthread
Hi Joel, On 22/05/18 16:50, Joel Fernandes (Google) wrote: > Currently there is a race in schedutil code for slow-switch single-CPU > systems. Fix it by enforcing ordering the write to work_in_progress to > happen before the read of next_freq. > > Kthread Sched update > > sugov_work()sugov_update_single() > > lock(); > // The CPU is free to rearrange below > // two in any order, so it may clear > // the flag first and then read next > // freq. Lets assume it does. > work_in_progress = false > >if (work_in_progress) > return; > >sg_policy->next_freq = 0; > freq = sg_policy->next_freq; >sg_policy->next_freq = > real-freq; > unlock(); > > Reported-by: Viresh Kumar > CC: Rafael J. Wysocki > CC: Peter Zijlstra > CC: Ingo Molnar > CC: Patrick Bellasi > CC: Juri Lelli > Cc: Luca Abeni > CC: Todd Kjos > CC: clau...@evidence.eu.com > CC: kernel-t...@android.com > CC: linux...@vger.kernel.org > Signed-off-by: Joel Fernandes (Google) > --- > I split this into separate patch, because this race can also happen in > mainline. > > kernel/sched/cpufreq_schedutil.c | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/kernel/sched/cpufreq_schedutil.c > b/kernel/sched/cpufreq_schedutil.c > index 5c482ec38610..ce7749da7a44 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -401,6 +401,13 @@ static void sugov_work(struct kthread_work *work) >*/ > raw_spin_lock_irqsave(_policy->update_lock, flags); > freq = sg_policy->next_freq; > + > + /* > + * sugov_update_single can access work_in_progress without update_lock, > + * make sure next_freq is read before work_in_progress is set. s/set/reset/ > + */ > + smp_mb(); > + Also, doesn't this need a corresponding barrier (I guess in sugov_should_update_freq)? That being a wmb and this a rmb? Best, - Juri
Re: [PATCH RFC] schedutil: Address the r/w ordering race in kthread
On Tue, May 22, 2018 at 04:50:28PM -0700, Joel Fernandes (Google) wrote: > Currently there is a race in schedutil code for slow-switch single-CPU > systems. Fix it by enforcing ordering the write to work_in_progress to > happen before the read of next_freq. Aargh, s/before/after/. Commit log has above issue but code is Ok. Should I resend this patch or are there any additional comments? thanks! - Joel [..]
Re: [PATCH RFC] schedutil: Address the r/w ordering race in kthread
On Tue, May 22, 2018 at 04:50:28PM -0700, Joel Fernandes (Google) wrote: > Currently there is a race in schedutil code for slow-switch single-CPU > systems. Fix it by enforcing ordering the write to work_in_progress to > happen before the read of next_freq. Aargh, s/before/after/. Commit log has above issue but code is Ok. Should I resend this patch or are there any additional comments? thanks! - Joel [..]
[PATCH RFC] schedutil: Address the r/w ordering race in kthread
Currently there is a race in schedutil code for slow-switch single-CPU systems. Fix it by enforcing ordering the write to work_in_progress to happen before the read of next_freq. Kthread Sched update sugov_work() sugov_update_single() lock(); // The CPU is free to rearrange below // two in any order, so it may clear // the flag first and then read next // freq. Lets assume it does. work_in_progress = false if (work_in_progress) return; sg_policy->next_freq = 0; freq = sg_policy->next_freq; sg_policy->next_freq = real-freq; unlock(); Reported-by: Viresh KumarCC: Rafael J. Wysocki CC: Peter Zijlstra CC: Ingo Molnar CC: Patrick Bellasi CC: Juri Lelli Cc: Luca Abeni CC: Todd Kjos CC: clau...@evidence.eu.com CC: kernel-t...@android.com CC: linux...@vger.kernel.org Signed-off-by: Joel Fernandes (Google) --- I split this into separate patch, because this race can also happen in mainline. kernel/sched/cpufreq_schedutil.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c index 5c482ec38610..ce7749da7a44 100644 --- a/kernel/sched/cpufreq_schedutil.c +++ b/kernel/sched/cpufreq_schedutil.c @@ -401,6 +401,13 @@ static void sugov_work(struct kthread_work *work) */ raw_spin_lock_irqsave(_policy->update_lock, flags); freq = sg_policy->next_freq; + + /* +* sugov_update_single can access work_in_progress without update_lock, +* make sure next_freq is read before work_in_progress is set. +*/ + smp_mb(); + sg_policy->work_in_progress = false; raw_spin_unlock_irqrestore(_policy->update_lock, flags); -- 2.17.0.441.gb46fe60e1d-goog
[PATCH RFC] schedutil: Address the r/w ordering race in kthread
Currently there is a race in schedutil code for slow-switch single-CPU systems. Fix it by enforcing ordering the write to work_in_progress to happen before the read of next_freq. Kthread Sched update sugov_work() sugov_update_single() lock(); // The CPU is free to rearrange below // two in any order, so it may clear // the flag first and then read next // freq. Lets assume it does. work_in_progress = false if (work_in_progress) return; sg_policy->next_freq = 0; freq = sg_policy->next_freq; sg_policy->next_freq = real-freq; unlock(); Reported-by: Viresh Kumar CC: Rafael J. Wysocki CC: Peter Zijlstra CC: Ingo Molnar CC: Patrick Bellasi CC: Juri Lelli Cc: Luca Abeni CC: Todd Kjos CC: clau...@evidence.eu.com CC: kernel-t...@android.com CC: linux...@vger.kernel.org Signed-off-by: Joel Fernandes (Google) --- I split this into separate patch, because this race can also happen in mainline. kernel/sched/cpufreq_schedutil.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c index 5c482ec38610..ce7749da7a44 100644 --- a/kernel/sched/cpufreq_schedutil.c +++ b/kernel/sched/cpufreq_schedutil.c @@ -401,6 +401,13 @@ static void sugov_work(struct kthread_work *work) */ raw_spin_lock_irqsave(_policy->update_lock, flags); freq = sg_policy->next_freq; + + /* +* sugov_update_single can access work_in_progress without update_lock, +* make sure next_freq is read before work_in_progress is set. +*/ + smp_mb(); + sg_policy->work_in_progress = false; raw_spin_unlock_irqrestore(_policy->update_lock, flags); -- 2.17.0.441.gb46fe60e1d-goog