All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 4/7] perf: Check if HT is supported and enabled
@ 2010-12-27 15:36 Lin Ming
       [not found] ` <AANLkTimp9VgD4qhOVmq-k1Ckzti9eeV8pf6Jvt5YB6nx@mail.gmail.com>
  0 siblings, 1 reply; 15+ messages in thread
From: Lin Ming @ 2010-12-27 15:36 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Andi Kleen, Stephane Eranian; +Cc: lkml

Avoid the percore allocations if HT is not supported or disabled.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 arch/x86/kernel/cpu/perf_event_intel.c |   44 +++++++++++++++++++++++++++----
 1 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index bc4afb1..354d1de 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -1053,9 +1053,38 @@ static __initconst const struct x86_pmu core_pmu = {
 	.event_constraints	= intel_core_event_constraints,
 };
 
+/*
+ * Check if HT is capable and enabled
+ * TBD: move it to generic place, so it can be used by others
+ */
+
+static bool ht_enabled(int cpu)
+{
+	int total_logical_processors, total_cores;
+	unsigned int eax, ebx, ecx, edx;
+
+	cpuid(1, &eax, &ebx, &ecx, &edx);
+
+	/* Bit 28 in EDX indicates if it's HT capable */
+	if (!(edx & 0x10000000))
+		return false;
+
+	total_logical_processors = (ebx >> 16) & 0xff;
+
+	ecx = 0;
+	cpuid(4, &eax, &ebx, &ecx, &edx);
+	total_cores = ((eax >> 26) & 0x3f) + 1;
+
+	/* Thread nums per core */
+	return (total_logical_processors / total_cores) > 1;
+}
+
 static int intel_pmu_cpu_prepare(int cpu)
 {
 	struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
+
+	if (!ht_enabled(cpu))
+		return NOTIFY_OK;
 	
 	cpuc->per_core = kzalloc_node(sizeof(struct intel_percore), 
 				      GFP_KERNEL, cpu_to_node(cpu));
@@ -1073,6 +1102,15 @@ static void intel_pmu_cpu_starting(int cpu)
 	int core_id = topology_core_id(cpu);
 	int i;
 
+	init_debug_store_on_cpu(cpu);
+	/*
+	 * Deal with CPUs that don't clear their LBRs on power-up.
+	 */
+	intel_pmu_lbr_reset();
+
+	if (!ht_enabled(cpu))
+		return;
+
 	for_each_online_cpu(i) {
 		struct intel_percore *pc = per_cpu(cpu_hw_events, i).per_core;
 
@@ -1085,12 +1123,6 @@ static void intel_pmu_cpu_starting(int cpu)
 
 	cpuc->per_core->core_id = core_id;
 	cpuc->per_core->refcnt++;
-
-	init_debug_store_on_cpu(cpu);
-	/*
-	 * Deal with CPUs that don't clear their LBRs on power-up.
-	 */
-	intel_pmu_lbr_reset();
 }
 
 static void intel_pmu_cpu_dying(int cpu)
-- 
1.7.3






^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/7] perf: Check if HT is supported and enabled
       [not found] ` <AANLkTimp9VgD4qhOVmq-k1Ckzti9eeV8pf6Jvt5YB6nx@mail.gmail.com>
@ 2010-12-28  8:51   ` Lin Ming
  2011-01-03 10:58     ` Peter Zijlstra
  0 siblings, 1 reply; 15+ messages in thread
From: Lin Ming @ 2010-12-28  8:51 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: Ingo Molnar, Andi Kleen, Stephane Eranian, lkml

On Tue, 2010-12-28 at 16:44 +0800, Lin Ming wrote:
> 
> Avoid the percore allocations if HT is not supported or disabled.
> 
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
>  arch/x86/kernel/cpu/perf_event_intel.c |   44 +++++++++++++++++++++++++++----
>  1 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/perf_event_intel.c
> b/arch/x86/kernel/cpu/perf_event_intel.c
> index bc4afb1..354d1de 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -1053,9 +1053,38 @@ static __initconst const struct x86_pmu core_pmu = {
>        .event_constraints      = intel_core_event_constraints,
>  };
> 
> +/*
> + * Check if HT is capable and enabled
> + * TBD: move it to generic place, so it can be used by others
> + */
> +
> +static bool ht_enabled(int cpu)
> +{
> +       int total_logical_processors, total_cores;
> +       unsigned int eax, ebx, ecx, edx;
> +
> +       cpuid(1, &eax, &ebx, &ecx, &edx);
> +
> +       /* Bit 28 in EDX indicates if it's HT capable */
> +       if (!(edx & 0x10000000))
> +               return false;
> +
> +       total_logical_processors = (ebx >> 16) & 0xff;
> +
> +       ecx = 0;
> +       cpuid(4, &eax, &ebx, &ecx, &edx);
> +       total_cores = ((eax >> 26) & 0x3f) + 1;
> +
> +       /* Thread nums per core */
> +       return (total_logical_processors / total_cores) > 1;
> +}
> +

This function can be simplified as below,

static bool ht_enabled()
{
        if (!cpu_has(&boot_cpu_data, X86_FEATURE_HT))
                return false;

        return smp_num_siblings > 1;
}

But this still can't detect if HT is on or off.
smp_num_siblings is always 2 even if HT is disabled in BIOS.

Any idea how to detect if HT is on or not?

Thanks,
Lin Ming

>  static int intel_pmu_cpu_prepare(int cpu)
>  {
>        struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
> +
> +       if (!ht_enabled(cpu))
> +               return NOTIFY_OK;
> 
>        cpuc->per_core = kzalloc_node(sizeof(struct intel_percore),
>                                      GFP_KERNEL, cpu_to_node(cpu));
> @@ -1073,6 +1102,15 @@ static void intel_pmu_cpu_starting(int cpu)
>        int core_id = topology_core_id(cpu);
>        int i;
> 
> +       init_debug_store_on_cpu(cpu);
> +       /*
> +        * Deal with CPUs that don't clear their LBRs on power-up.
> +        */
> +       intel_pmu_lbr_reset();
> +
> +       if (!ht_enabled(cpu))
> +               return;
> +
>        for_each_online_cpu(i) {
>                struct intel_percore *pc = per_cpu(cpu_hw_events, i).per_core;
> 
> @@ -1085,12 +1123,6 @@ static void intel_pmu_cpu_starting(int cpu)
> 
>        cpuc->per_core->core_id = core_id;
>        cpuc->per_core->refcnt++;
> -
> -       init_debug_store_on_cpu(cpu);
> -       /*
> -        * Deal with CPUs that don't clear their LBRs on power-up.
> -        */
> -       intel_pmu_lbr_reset();
>  }
> 
>  static void intel_pmu_cpu_dying(int cpu)
> --
> 1.7.3
> 
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/7] perf: Check if HT is supported and enabled
  2010-12-28  8:51   ` Lin Ming
@ 2011-01-03 10:58     ` Peter Zijlstra
  2011-01-03 15:21       ` Andi Kleen
  2011-01-03 19:53       ` H. Peter Anvin
  0 siblings, 2 replies; 15+ messages in thread
From: Peter Zijlstra @ 2011-01-03 10:58 UTC (permalink / raw)
  To: Lin Ming; +Cc: Ingo Molnar, Andi Kleen, Stephane Eranian, lkml, H. Peter Anvin

On Tue, 2010-12-28 at 16:51 +0800, Lin Ming wrote:
> > +static bool ht_enabled(int cpu)
> > +{
> > +       int total_logical_processors, total_cores;
> > +       unsigned int eax, ebx, ecx, edx;
> > +
> > +       cpuid(1, &eax, &ebx, &ecx, &edx);
> > +
> > +       /* Bit 28 in EDX indicates if it's HT capable */
> > +       if (!(edx & 0x10000000))
> > +               return false;
> > +
> > +       total_logical_processors = (ebx >> 16) & 0xff;
> > +
> > +       ecx = 0;
> > +       cpuid(4, &eax, &ebx, &ecx, &edx);
> > +       total_cores = ((eax >> 26) & 0x3f) + 1;
> > +
> > +       /* Thread nums per core */
> > +       return (total_logical_processors / total_cores) > 1;
> > +}
> > +
> 
> This function can be simplified as below,
> 
> static bool ht_enabled()
> {
>         if (!cpu_has(&boot_cpu_data, X86_FEATURE_HT))
>                 return false;
> 
>         return smp_num_siblings > 1;
> }
> 
> But this still can't detect if HT is on or off.
> smp_num_siblings is always 2 even if HT is disabled in BIOS.
> 
> Any idea how to detect if HT is on or not?

Not quite sure, the intel docs aren't really clear on how the HW
supports HT, has 2 siblings but BIOS disabled it thing works. I just
tried reading the arch/x86 code but that only got me more confused.

hpa, could you comment?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/7] perf: Check if HT is supported and enabled
  2011-01-03 10:58     ` Peter Zijlstra
@ 2011-01-03 15:21       ` Andi Kleen
  2011-01-03 19:53       ` H. Peter Anvin
  1 sibling, 0 replies; 15+ messages in thread
From: Andi Kleen @ 2011-01-03 15:21 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Lin Ming, Ingo Molnar, Andi Kleen, Stephane Eranian, lkml,
	H. Peter Anvin

> Not quite sure, the intel docs aren't really clear on how the HW
> supports HT, has 2 siblings but BIOS disabled it thing works. I just
> tried reading the arch/x86 code but that only got me more confused.

... or you just keep the original code which works fine in any case
without this.

-Andi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/7] perf: Check if HT is supported and enabled
  2011-01-03 10:58     ` Peter Zijlstra
  2011-01-03 15:21       ` Andi Kleen
@ 2011-01-03 19:53       ` H. Peter Anvin
  2011-01-04 11:10         ` Peter Zijlstra
  1 sibling, 1 reply; 15+ messages in thread
From: H. Peter Anvin @ 2011-01-03 19:53 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: Lin Ming, Ingo Molnar, Andi Kleen, Stephane Eranian, lkml

On 01/03/2011 02:58 AM, Peter Zijlstra wrote:
>>
>> This function can be simplified as below,
>>
>> static bool ht_enabled()
>> {
>>         if (!cpu_has(&boot_cpu_data, X86_FEATURE_HT))
>>                 return false;
>>
>>         return smp_num_siblings > 1;
>> }
>>
>> But this still can't detect if HT is on or off.
>> smp_num_siblings is always 2 even if HT is disabled in BIOS.
>>
>> Any idea how to detect if HT is on or not?
> 
> Not quite sure, the intel docs aren't really clear on how the HW
> supports HT, has 2 siblings but BIOS disabled it thing works. I just
> tried reading the arch/x86 code but that only got me more confused.
> 
> hpa, could you comment?
> 

Zeroeth of all: anyone who writes () in a function prototype in C needs
to get severely napalmed, maimed, hung and then really hurt.  It is
(void) in C, () means (...) which is literally NEVER what you want.

Now, *first* of all: smp_num_siblings as it sits is obviously broken;
the whole notion of a global variable for what is inherently a per-cpu
property is just braindead.  At least theoretically there could be cores
with and without HT or with different thread count in the same system.

Second, perf should get this from /proc/cpuinfo, not by grotting around
cpuid by itself.

Third, the code in the kernel is indeed pretty confusing, and it also
has the global variable braindamage... but either it works (in which
case getting the data from /proc/cpuinfo works) or it needs fixing in
addition to the global variable issue.

	-hpa

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/7] perf: Check if HT is supported and enabled
  2011-01-03 19:53       ` H. Peter Anvin
@ 2011-01-04 11:10         ` Peter Zijlstra
  2011-01-04 13:38           ` Stephane Eranian
  2011-01-04 18:55           ` Valdis.Kletnieks
  0 siblings, 2 replies; 15+ messages in thread
From: Peter Zijlstra @ 2011-01-04 11:10 UTC (permalink / raw)
  To: H. Peter Anvin; +Cc: Lin Ming, Ingo Molnar, Andi Kleen, Stephane Eranian, lkml

On Mon, 2011-01-03 at 11:53 -0800, H. Peter Anvin wrote:
> On 01/03/2011 02:58 AM, Peter Zijlstra wrote:
> >>
> >> This function can be simplified as below,
> >>
> >> static bool ht_enabled()
> >> {
> >>         if (!cpu_has(&boot_cpu_data, X86_FEATURE_HT))
> >>                 return false;
> >>
> >>         return smp_num_siblings > 1;
> >> }
> >>
> >> But this still can't detect if HT is on or off.
> >> smp_num_siblings is always 2 even if HT is disabled in BIOS.
> >>
> >> Any idea how to detect if HT is on or not?
> > 
> > Not quite sure, the intel docs aren't really clear on how the HW
> > supports HT, has 2 siblings but BIOS disabled it thing works. I just
> > tried reading the arch/x86 code but that only got me more confused.
> > 
> > hpa, could you comment?
> > 
> 
> Zeroeth of all: anyone who writes () in a function prototype in C needs
> to get severely napalmed, maimed, hung and then really hurt.  It is
> (void) in C, () means (...) which is literally NEVER what you want.
> 
> Now, *first* of all: smp_num_siblings as it sits is obviously broken;
> the whole notion of a global variable for what is inherently a per-cpu
> property is just braindead.  At least theoretically there could be cores
> with and without HT or with different thread count in the same system.
> 
> Second, perf should get this from /proc/cpuinfo, not by grotting around
> cpuid by itself.

Its the kernel space bit that wants to know if HT is enabled on CPU
bringup, /proc/cpuinfo is kinda useless in that context.

> Third, the code in the kernel is indeed pretty confusing, and it also
> has the global variable braindamage... but either it works (in which
> case getting the data from /proc/cpuinfo works) or it needs fixing in
> addition to the global variable issue.

So you failed to address the primary question, how do we tell if HT is
enabled for a particular CPU?

X86_FEATURE_HT tells us the CPU supports telling us about HT,
smp_num_siblings > 1 tells us the CPU is capable of HT. But Lin found
that if you disable HT in the BIOS both those are true but we still
don't have HT enabled.

The problem we have to address is that Intel has some MSRs shared
between threads and if HT is enabled we need to allocate some memory to
manage this shared resource.

The simple check outlined above X86_FEATURE_HT && smp_num_siblings > 1,
will get us most of the way and will I think be good enough (false
positives but no false negatives). But it did get us wondering about how
all this works.

So could you, irrespective of Linux implementation details tell us how
to detect if HT is available and enabled from a HW and BIOS perspective?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/7] perf: Check if HT is supported and enabled
  2011-01-04 11:10         ` Peter Zijlstra
@ 2011-01-04 13:38           ` Stephane Eranian
  2011-01-04 13:44             ` Peter Zijlstra
  2011-01-04 18:55           ` Valdis.Kletnieks
  1 sibling, 1 reply; 15+ messages in thread
From: Stephane Eranian @ 2011-01-04 13:38 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: H. Peter Anvin, Lin Ming, Ingo Molnar, Andi Kleen, lkml

Hi,

I had to deal with this issue with perfmon back in 2.6.30 and earlier kernels.
I remember that I was surprised not to find any easy helper function to figure
this out back then. Being HT capable is different from having HT enabled.
Seems like the situation has not improved since then.

My solution at the time (2.6.30) was to do:
        ht_enabled = cpumask_weight(__get_cpu_var(cpu_sibling_map)) > 1;

Not too convinced the per-cpu variable is necessary because I don't
know of a BIOS that would allow you to turn on HT on only some of the
cores (AFAIK, this is an all or nothing feature).


On Tue, Jan 4, 2011 at 12:10 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> On Mon, 2011-01-03 at 11:53 -0800, H. Peter Anvin wrote:
>> On 01/03/2011 02:58 AM, Peter Zijlstra wrote:
>> >>
>> >> This function can be simplified as below,
>> >>
>> >> static bool ht_enabled()
>> >> {
>> >>         if (!cpu_has(&boot_cpu_data, X86_FEATURE_HT))
>> >>                 return false;
>> >>
>> >>         return smp_num_siblings > 1;
>> >> }
>> >>
>> >> But this still can't detect if HT is on or off.
>> >> smp_num_siblings is always 2 even if HT is disabled in BIOS.
>> >>
>> >> Any idea how to detect if HT is on or not?
>> >
>> > Not quite sure, the intel docs aren't really clear on how the HW
>> > supports HT, has 2 siblings but BIOS disabled it thing works. I just
>> > tried reading the arch/x86 code but that only got me more confused.
>> >
>> > hpa, could you comment?
>> >
>>
>> Zeroeth of all: anyone who writes () in a function prototype in C needs
>> to get severely napalmed, maimed, hung and then really hurt.  It is
>> (void) in C, () means (...) which is literally NEVER what you want.
>>
>> Now, *first* of all: smp_num_siblings as it sits is obviously broken;
>> the whole notion of a global variable for what is inherently a per-cpu
>> property is just braindead.  At least theoretically there could be cores
>> with and without HT or with different thread count in the same system.
>>
>> Second, perf should get this from /proc/cpuinfo, not by grotting around
>> cpuid by itself.
>
> Its the kernel space bit that wants to know if HT is enabled on CPU
> bringup, /proc/cpuinfo is kinda useless in that context.
>
>> Third, the code in the kernel is indeed pretty confusing, and it also
>> has the global variable braindamage... but either it works (in which
>> case getting the data from /proc/cpuinfo works) or it needs fixing in
>> addition to the global variable issue.
>
> So you failed to address the primary question, how do we tell if HT is
> enabled for a particular CPU?
>
> X86_FEATURE_HT tells us the CPU supports telling us about HT,
> smp_num_siblings > 1 tells us the CPU is capable of HT. But Lin found
> that if you disable HT in the BIOS both those are true but we still
> don't have HT enabled.
>
> The problem we have to address is that Intel has some MSRs shared
> between threads and if HT is enabled we need to allocate some memory to
> manage this shared resource.
>
> The simple check outlined above X86_FEATURE_HT && smp_num_siblings > 1,
> will get us most of the way and will I think be good enough (false
> positives but no false negatives). But it did get us wondering about how
> all this works.
>
> So could you, irrespective of Linux implementation details tell us how
> to detect if HT is available and enabled from a HW and BIOS perspective?
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/7] perf: Check if HT is supported and enabled
  2011-01-04 13:38           ` Stephane Eranian
@ 2011-01-04 13:44             ` Peter Zijlstra
  2011-01-04 13:52               ` Stephane Eranian
  0 siblings, 1 reply; 15+ messages in thread
From: Peter Zijlstra @ 2011-01-04 13:44 UTC (permalink / raw)
  To: Stephane Eranian; +Cc: H. Peter Anvin, Lin Ming, Ingo Molnar, Andi Kleen, lkml

On Tue, 2011-01-04 at 14:38 +0100, Stephane Eranian wrote:
> My solution at the time (2.6.30) was to do:
>         ht_enabled = cpumask_weight(__get_cpu_var(cpu_sibling_map)) > 1;

Won't that report a machine a HT disabled when you offline a sibling?
Which kinda defeats the purpose of our usage here, since we need to know
it before either sibling comes online.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/7] perf: Check if HT is supported and enabled
  2011-01-04 13:44             ` Peter Zijlstra
@ 2011-01-04 13:52               ` Stephane Eranian
  2011-01-04 13:58                 ` Peter Zijlstra
  2011-01-05  5:45                 ` Lin Ming
  0 siblings, 2 replies; 15+ messages in thread
From: Stephane Eranian @ 2011-01-04 13:52 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: H. Peter Anvin, Lin Ming, Ingo Molnar, Andi Kleen, lkml

On Tue, Jan 4, 2011 at 2:44 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> On Tue, 2011-01-04 at 14:38 +0100, Stephane Eranian wrote:
>> My solution at the time (2.6.30) was to do:
>>         ht_enabled = cpumask_weight(__get_cpu_var(cpu_sibling_map)) > 1;
>
> Won't that report a machine a HT disabled when you offline a sibling?

I think you're right. I was not dealing with hotplug CPU.

> Which kinda defeats the purpose of our usage here, since we need to know
> it before either sibling comes online.

Then, it seems the only hope is to peek at a MSR that reports the BIOS setting.
But I don't know which one it is.

Couldn't you simply over-provision, and then when the CPU is online, use my
ht_enabled statement to figure out whether or not you need to handle the sharing
issue?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/7] perf: Check if HT is supported and enabled
  2011-01-04 13:52               ` Stephane Eranian
@ 2011-01-04 13:58                 ` Peter Zijlstra
  2011-01-04 15:35                   ` Stephane Eranian
  2011-01-04 19:00                   ` H. Peter Anvin
  2011-01-05  5:45                 ` Lin Ming
  1 sibling, 2 replies; 15+ messages in thread
From: Peter Zijlstra @ 2011-01-04 13:58 UTC (permalink / raw)
  To: Stephane Eranian; +Cc: H. Peter Anvin, Lin Ming, Ingo Molnar, Andi Kleen, lkml

On Tue, 2011-01-04 at 14:52 +0100, Stephane Eranian wrote:
> 
> Couldn't you simply over-provision, and then when the CPU is online, use my
> ht_enabled statement to figure out whether or not you need to handle the sharing
> issue? 

That's pretty much what Lin's latest does:

 X86_FEATURE_HT && smp_num_siblings > 1

shows the CPU is capable of HT and will allocate the needed resources.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/7] perf: Check if HT is supported and enabled
  2011-01-04 13:58                 ` Peter Zijlstra
@ 2011-01-04 15:35                   ` Stephane Eranian
  2011-01-04 19:00                   ` H. Peter Anvin
  1 sibling, 0 replies; 15+ messages in thread
From: Stephane Eranian @ 2011-01-04 15:35 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: H. Peter Anvin, Lin Ming, Ingo Molnar, Andi Kleen, lkml

On Tue, Jan 4, 2011 at 2:58 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> On Tue, 2011-01-04 at 14:52 +0100, Stephane Eranian wrote:
>>
>> Couldn't you simply over-provision, and then when the CPU is online, use my
>> ht_enabled statement to figure out whether or not you need to handle the sharing
>> issue?
>
> That's pretty much what Lin's latest does:
>
>  X86_FEATURE_HT && smp_num_siblings > 1
>
> shows the CPU is capable of HT and will allocate the needed resources.
>
Ok, but once the CPU is online, you can still check whether or not HT is
enabled. If not, then you don't need to bother with the MSR sharing code.
So you may allocate a data structure which you won't use and I think that's
fine given we don't have a good way of figuring out whether HT is enabled
or not.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/7] perf: Check if HT is supported and enabled
  2011-01-04 11:10         ` Peter Zijlstra
  2011-01-04 13:38           ` Stephane Eranian
@ 2011-01-04 18:55           ` Valdis.Kletnieks
  1 sibling, 0 replies; 15+ messages in thread
From: Valdis.Kletnieks @ 2011-01-04 18:55 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: H. Peter Anvin, Lin Ming, Ingo Molnar, Andi Kleen,
	Stephane Eranian, lkml

[-- Attachment #1: Type: text/plain, Size: 417 bytes --]

On Tue, 04 Jan 2011 12:10:19 +0100, Peter Zijlstra said:

> X86_FEATURE_HT tells us the CPU supports telling us about HT,
> smp_num_siblings > 1 tells us the CPU is capable of HT.

A subtle but important distinction. Several laptops ago, I had a P4-M chip that
had FEATURE_HT, but num_siblings == 1.  ISTR trying to boot an SMP kernel on it
and it dying gloriously trying to bring up the non-existent other sibling.


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/7] perf: Check if HT is supported and enabled
  2011-01-04 13:58                 ` Peter Zijlstra
  2011-01-04 15:35                   ` Stephane Eranian
@ 2011-01-04 19:00                   ` H. Peter Anvin
  1 sibling, 0 replies; 15+ messages in thread
From: H. Peter Anvin @ 2011-01-04 19:00 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: Stephane Eranian, Lin Ming, Ingo Molnar, Andi Kleen, lkml

On 01/04/2011 05:58 AM, Peter Zijlstra wrote:
> On Tue, 2011-01-04 at 14:52 +0100, Stephane Eranian wrote:
>>
>> Couldn't you simply over-provision, and then when the CPU is online, use my
>> ht_enabled statement to figure out whether or not you need to handle the sharing
>> issue? 
> 
> That's pretty much what Lin's latest does:
> 
>  X86_FEATURE_HT && smp_num_siblings > 1
> 
> shows the CPU is capable of HT and will allocate the needed resources.
> 

>From the sound of it, it doesn't seem like it's worth optimizing out the
fairly small memory allocation.

	-hpa


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/7] perf: Check if HT is supported and enabled
  2011-01-04 13:52               ` Stephane Eranian
  2011-01-04 13:58                 ` Peter Zijlstra
@ 2011-01-05  5:45                 ` Lin Ming
  2011-01-05 10:08                   ` Peter Zijlstra
  1 sibling, 1 reply; 15+ messages in thread
From: Lin Ming @ 2011-01-05  5:45 UTC (permalink / raw)
  To: Stephane Eranian, Luck, Tony
  Cc: Peter Zijlstra, H. Peter Anvin, Ingo Molnar, Andi Kleen, lkml

On Tue, 2011-01-04 at 21:52 +0800, Stephane Eranian wrote:
> On Tue, Jan 4, 2011 at 2:44 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> > On Tue, 2011-01-04 at 14:38 +0100, Stephane Eranian wrote:
> >> My solution at the time (2.6.30) was to do:
> >>         ht_enabled = cpumask_weight(__get_cpu_var(cpu_sibling_map)) > 1;
> >
> > Won't that report a machine a HT disabled when you offline a sibling?
> 
> I think you're right. I was not dealing with hotplug CPU.
> 
> > Which kinda defeats the purpose of our usage here, since we need to know
> > it before either sibling comes online.
> 
> Then, it seems the only hope is to peek at a MSR that reports the BIOS setting.
> But I don't know which one it is.
> 
> Couldn't you simply over-provision, and then when the CPU is online, use my
> ht_enabled statement to figure out whether or not you need to handle the sharing
> issue?

I got an idea from Tony,

> If this is worth doing, then perhaps you could write a routine
> to check if another HT thread on this same core has already been
> brought online ... i.e. defer the allocation to when you see the
> second thread. If HT is off, then you'll never see a second thread
> so you'll avoid the allocation

We can't defer the allocation to CPU_UP_PREPARE
notifier(intel_pmu_cpu_prepare), because the thread cpumask for that
booting cpu has not been setup yet.

So below code defers the allocation to CPU_STARTING
notifier(intel_pmu_cpu_starting).

static bool ht_enabled(int cpu)
{
        struct cpumask *siblings = topology_thread_cpumask(cpu);

        if (!cpu_has(&boot_cpu_data, X86_FEATURE_HT))
                return false;

        return cpus_weight(*siblings) > 1;
}

static void intel_pmu_cpu_starting(int cpu)
{
        struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
        struct intel_percore *pc;
        int core_id = topology_core_id(cpu);
        int i;

        init_debug_store_on_cpu(cpu);
        /*
         * Deal with CPUs that don't clear their LBRs on power-up.
         */
        intel_pmu_lbr_reset();

        if (!ht_enabled(cpu))
                return;

        cpuc->per_core = kzalloc_node(sizeof(struct intel_percore),
                                      GFP_KERNEL, cpu_to_node(cpu));
        if (!cpuc->per_core)
                return;

        raw_spin_lock_init(&cpuc->per_core->lock);
        pc = cpuc->per_core;

        for_each_cpu(i, topology_thread_cpumask(cpu)) {
                cpuc = &per_cpu(cpu_hw_events, cpu);

                cpuc->per_core = pc;
                cpuc->per_core->core_id = core_id;
                cpuc->per_core->refcnt++;
        }
}

But when unplug/plug 2 HT threads, I got a BUG.

BUG: sleeping function called from invalid context at /opt/linux-2.6/mm/slub.c:793
in_atomic(): 0, irqs_disabled(): 1, pid: 0, name: kworker/0:1
INFO: lockdep is turned off.
irq event stamp: 0
hardirqs last  enabled at (0): [<          (null)>]           (null)
hardirqs last disabled at (0): [<ffffffff8106b4f0>] copy_process+0x59d/0x11a7
softirqs last  enabled at (0): [<ffffffff8106b4f0>] copy_process+0x59d/0x11a7
softirqs last disabled at (0): [<          (null)>]           (null)
Pid: 0, comm: kworker/0:1 Tainted: G        W   2.6.37-rc7-tip-mlin+ #267
Call Trace:
 [<ffffffff81060144>] ? __might_sleep+0xe9/0xee
 [<ffffffff81113e14>] ? kmem_cache_alloc_node_notrace+0x49/0xdd
 [<ffffffff8103f24d>] ? intel_pmu_cpu_starting+0xa8/0x18b
 [<ffffffff8103f24d>] ? intel_pmu_cpu_starting+0xa8/0x18b
 [<ffffffff8104669c>] ? generic_set_all+0x25f/0x294
 [<ffffffff81735d76>] ? x86_pmu_notifier+0x4b/0x52
 [<ffffffff81742e23>] ? notifier_call_chain+0x5e/0x92
 [<ffffffff8108cc55>] ? __raw_notifier_call_chain+0x9/0xb
 [<ffffffff8106e834>] ? __cpu_notify+0x1b/0x2d
 [<ffffffff8106e854>] ? cpu_notify+0xe/0x10
 [<ffffffff81739d43>] ? notify_cpu_starting+0x24/0x26
 [<ffffffff817381b4>] ? start_secondary+0x122/0x193



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/7] perf: Check if HT is supported and enabled
  2011-01-05  5:45                 ` Lin Ming
@ 2011-01-05 10:08                   ` Peter Zijlstra
  0 siblings, 0 replies; 15+ messages in thread
From: Peter Zijlstra @ 2011-01-05 10:08 UTC (permalink / raw)
  To: Lin Ming
  Cc: Stephane Eranian, Luck, Tony, H. Peter Anvin, Ingo Molnar,
	Andi Kleen, lkml

On Wed, 2011-01-05 at 13:45 +0800, Lin Ming wrote:
> static void intel_pmu_cpu_starting(int cpu)
> {
>         struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
>         struct intel_percore *pc;
>         int core_id = topology_core_id(cpu);
>         int i;
> 
>         init_debug_store_on_cpu(cpu);
>         /*
>          * Deal with CPUs that don't clear their LBRs on power-up.
>          */
>         intel_pmu_lbr_reset();
> 
>         if (!ht_enabled(cpu))
>                 return;
> 
>         cpuc->per_core = kzalloc_node(sizeof(struct intel_percore),
>                                       GFP_KERNEL, cpu_to_node(cpu));

#define CPU_STARTING            0x000A /* CPU (unsigned)v soon running.
                                        * Called on the new cpu, just before
                                        * enabling interrupts. Must not sleep,
                                        * must not fail */


>         if (!cpuc->per_core)
>                 return;
> 
>         raw_spin_lock_init(&cpuc->per_core->lock);
>         pc = cpuc->per_core;
> 
>         for_each_cpu(i, topology_thread_cpumask(cpu)) {
>                 cpuc = &per_cpu(cpu_hw_events, cpu);
> 
>                 cpuc->per_core = pc;
>                 cpuc->per_core->core_id = core_id;
>                 cpuc->per_core->refcnt++;
>         }
> } 

Anyway, I think something like:

static bool ht_capable(void)
{
	return boot_cpu_has(X86_FEATURE_HT) && smp_num_siblings > 1;
}

is sufficient, the whole hotplug issues makes relying on enumeration
results impractical.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2011-01-05 10:08 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-27 15:36 [PATCH 4/7] perf: Check if HT is supported and enabled Lin Ming
     [not found] ` <AANLkTimp9VgD4qhOVmq-k1Ckzti9eeV8pf6Jvt5YB6nx@mail.gmail.com>
2010-12-28  8:51   ` Lin Ming
2011-01-03 10:58     ` Peter Zijlstra
2011-01-03 15:21       ` Andi Kleen
2011-01-03 19:53       ` H. Peter Anvin
2011-01-04 11:10         ` Peter Zijlstra
2011-01-04 13:38           ` Stephane Eranian
2011-01-04 13:44             ` Peter Zijlstra
2011-01-04 13:52               ` Stephane Eranian
2011-01-04 13:58                 ` Peter Zijlstra
2011-01-04 15:35                   ` Stephane Eranian
2011-01-04 19:00                   ` H. Peter Anvin
2011-01-05  5:45                 ` Lin Ming
2011-01-05 10:08                   ` Peter Zijlstra
2011-01-04 18:55           ` Valdis.Kletnieks

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.