Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1559421 > unrolled thread

[PATCH] fix race caused by hyperthreads when online an offline cpu

Back to article view | Back to linux.kernel

Contents

  [PATCH] fix race caused by hyperthreads when online an offline cpu Zhou Chengming <zhouchengming1@huawei.com> - 2017-01-16 04:40 +0100
    Re: [PATCH] fix race caused by hyperthreads when online an offline  cpu Thomas Gleixner <tglx@linutronix.de> - 2017-01-16 10:10 +0100
      Re: [PATCH] fix race caused by hyperthreads when online an offline  cpu zhouchengming <zhouchengming1@huawei.com> - 2017-01-16 11:00 +0100
        Re: [PATCH] fix race caused by hyperthreads when online an offline cpu Stephane Eranian <eranian@google.com> - 2017-01-16 19:40 +0100
          Re: [PATCH] fix race caused by hyperthreads when online an offline  cpu Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 11:50 +0100
    [tip:perf/urgent] perf/x86/intel: Handle exclusive threadid  correctly on CPU hotplug tip-bot for Zhou Chengming <tipbot@zytor.com> - 2017-01-17 11:20 +0100

#1559421 — [PATCH] fix race caused by hyperthreads when online an offline cpu

After online an offline cpu, cpu_hw_events.excl_thread_id will always be
set to 1 in intel_pmu_cpu_starting() even when its sibling's excl_thread_id
is also 1. Then the two siblings will use the same state in their shared
cpu_hw_events.excl_cntrs, it will cause race problem.

The race senario is like this:

Two cpu (7 and 19) are siblings, excl_thread_id of 7 and 19 are 0 and 1.
After offline and online cpu 7, intel_pmu_cpu_starting() will set excl_thread_id
of cpu 7 to 1. Then both cpu 7 and 19 will use the same state in their
shared cpu_hw_events.excl_cntrs.

cpu7					cpu19
---					---
intel_start_scheduling()
 set state->sched_started = true
					intel_put_excl_constraints() {
					 if (!state->sched_started)
					  spin_lock	// not executed
intel_stop_scheduling()
 set state->sched_started = false
					if (!state->sched_started)
					 spin_unlock	// excuted

Signed-off-by: NuoHan Qiao <qiaonuohan@huawei.com>
Signed-off-by: Zhou Chengming <zhouchengming1@huawei.com>
---
 arch/x86/events/intel/core.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index a74a2db..593d8c9 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3164,13 +3164,16 @@ static void intel_pmu_cpu_starting(int cpu)
 
 	if (x86_pmu.flags & PMU_FL_EXCL_CNTRS) {
 		for_each_cpu(i, topology_sibling_cpumask(cpu)) {
+			struct cpu_hw_events *sibling;
 			struct intel_excl_cntrs *c;
 
-			c = per_cpu(cpu_hw_events, i).excl_cntrs;
+			sibling = &per_cpu(cpu_hw_events, i);
+			c = sibling->excl_cntrs;
 			if (c && c->core_id == core_id) {
 				cpuc->kfree_on_online[1] = cpuc->excl_cntrs;
 				cpuc->excl_cntrs = c;
-				cpuc->excl_thread_id = 1;
+				if (!sibling->excl_thread_id)
+					cpuc->excl_thread_id = 1;
 				break;
 			}
 		}
-- 
1.7.7

[toc] | [next] | [standalone]

#1559529 — Re: [PATCH] fix race caused by hyperthreads when online an offline cpu

On Mon, 16 Jan 2017, Zhou Chengming wrote:

Can you please stop sending the same patch over and over every other day?

Granted, things get forgotten, but sending a polite reminder after a week
is definitely enough.

Maintainers are not machines responding within a split second on every mail
they get. And that patch is not so substantial that it justifies that kind
of spam.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1559569 — Re: [PATCH] fix race caused by hyperthreads when online an offline cpu

On 2017/1/16 17:05, Thomas Gleixner wrote:
> On Mon, 16 Jan 2017, Zhou Chengming wrote:
>
> Can you please stop sending the same patch over and over every other day?
>
> Granted, things get forgotten, but sending a polite reminder after a week
> is definitely enough.
>
> Maintainers are not machines responding within a split second on every mail
> they get. And that patch is not so substantial that it justifies that kind
> of spam.
>

Very sorry for the noise. We are just not sure this is the right fix because it's
hard to reproduce.

Thanks.

> Thanks,
>
> 	tglx
>
>
>
> .
>

[toc] | [prev] | [next] | [standalone]

#1559969

On Mon, Jan 16, 2017 at 1:53 AM, zhouchengming
<zhouchengming1@huawei.com> wrote:
> On 2017/1/16 17:05, Thomas Gleixner wrote:
>>
>> On Mon, 16 Jan 2017, Zhou Chengming wrote:
>>
>> Can you please stop sending the same patch over and over every other day?
>>
>> Granted, things get forgotten, but sending a polite reminder after a week
>> is definitely enough.
>>
>> Maintainers are not machines responding within a split second on every
>> mail
>> they get. And that patch is not so substantial that it justifies that kind
>> of spam.
>>
>
> Very sorry for the noise. We are just not sure this is the right fix because
> it's
> hard to reproduce.
>
I believe this is the right fixed. I tried it and instrumented the
code to verify thread_id
assignment. The problem is easy to reproduce.

$ echo 0 >/sys/devices/system/cpu/cpu2/online
$ echo 1 >/sys/devices/system/cpu/cpu2/online

Normally on Haswell Desktop part,  CPU2 gets thread_id 0 on boot, CPU6
gets thread_id 1.
If you offline CPU2 and bring it back in, it will get thread_id 1 and
thus both sibling will point
to the same exclusive state. The fix is, indeed, to check if the
sibling is not already assigned 1,
and if so to keep 0 for the CPU being online'd.

[toc] | [prev] | [next] | [standalone]

#1560478 — Re: [PATCH] fix race caused by hyperthreads when online an offline cpu

On Mon, 16 Jan 2017, Stephane Eranian wrote:
> On Mon, Jan 16, 2017 at 1:53 AM, zhouchengming
> <zhouchengming1@huawei.com> wrote:
> > On 2017/1/16 17:05, Thomas Gleixner wrote:
> >>
> >> On Mon, 16 Jan 2017, Zhou Chengming wrote:
> >>
> >> Can you please stop sending the same patch over and over every other day?
> >>
> >> Granted, things get forgotten, but sending a polite reminder after a week
> >> is definitely enough.
> >>
> >> Maintainers are not machines responding within a split second on every
> >> mail
> >> they get. And that patch is not so substantial that it justifies that kind
> >> of spam.
> >>
> >
> > Very sorry for the noise. We are just not sure this is the right fix because
> > it's
> > hard to reproduce.
> >
> I believe this is the right fixed. I tried it and instrumented the
> code to verify thread_id
> assignment. The problem is easy to reproduce.
> 
> $ echo 0 >/sys/devices/system/cpu/cpu2/online
> $ echo 1 >/sys/devices/system/cpu/cpu2/online
> 
> Normally on Haswell Desktop part,  CPU2 gets thread_id 0 on boot, CPU6
> gets thread_id 1.
> If you offline CPU2 and bring it back in, it will get thread_id 1 and
> thus both sibling will point
> to the same exclusive state. The fix is, indeed, to check if the
> sibling is not already assigned 1,
> and if so to keep 0 for the CPU being online'd.

Right. So it's a simple static fully reproducible problem and not a race of
some sorts. I'll amend the changelog ....

Btw, this code has the hardcoded assumption two threads per core. So
anything which has more than two threads is broken vs. that exclusive
access. No idea whether that matters in practice, but I just noticed.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1560429 — [tip:perf/urgent] perf/x86/intel: Handle exclusive threadid correctly on CPU hotplug

Commit-ID:  4e71de7986386d5fd3765458f27d612931f27f5e
Gitweb:     http://git.kernel.org/tip/4e71de7986386d5fd3765458f27d612931f27f5e
Author:     Zhou Chengming <zhouchengming1@huawei.com>
AuthorDate: Mon, 16 Jan 2017 11:21:11 +0800
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 17 Jan 2017 11:08:36 +0100

perf/x86/intel: Handle exclusive threadid correctly on CPU hotplug

The CPU hotplug function intel_pmu_cpu_starting() sets
cpu_hw_events.excl_thread_id unconditionally to 1 when the shared exclusive
counters data structure is already availabe for the sibling thread.

This works during the boot process because the first sibling gets threadid
0 assigned and the second sibling which shares the data structure gets 1.

But when the first thread of the core is offlined and onlined again it
shares the data structure with the second thread and gets exclusive thread
id 1 assigned as well.

Prevent this by checking the threadid of the already online thread.

[ tglx: Rewrote changelog ]

Signed-off-by: Zhou Chengming <zhouchengming1@huawei.com>
Cc: NuoHan Qiao <qiaonuohan@huawei.com>
Cc: ak@linux.intel.com
Cc: peterz@infradead.org
Cc: kan.liang@intel.com
Cc: dave.hansen@linux.intel.com
Cc: eranian@google.com
Cc: qiaonuohan@huawei.com
Cc: davidcc@google.com
Cc: guohanjun@huawei.com
Link: http://lkml.kernel.org/r/1484536871-3131-1-git-send-email-zhouchengming1@huawei.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---					---
 arch/x86/events/intel/core.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

---
 arch/x86/events/intel/core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index d611cab..eb1484c 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3176,13 +3176,16 @@ static void intel_pmu_cpu_starting(int cpu)
 
 	if (x86_pmu.flags & PMU_FL_EXCL_CNTRS) {
 		for_each_cpu(i, topology_sibling_cpumask(cpu)) {
+			struct cpu_hw_events *sibling;
 			struct intel_excl_cntrs *c;
 
-			c = per_cpu(cpu_hw_events, i).excl_cntrs;
+			sibling = &per_cpu(cpu_hw_events, i);
+			c = sibling->excl_cntrs;
 			if (c && c->core_id == core_id) {
 				cpuc->kfree_on_online[1] = cpuc->excl_cntrs;
 				cpuc->excl_cntrs = c;
-				cpuc->excl_thread_id = 1;
+				if (!sibling->excl_thread_id)
+					cpuc->excl_thread_id = 1;
 				break;
 			}
 		}

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web