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

Groups > linux.kernel > #1312174 > unrolled thread

Re: [PATCH v2 5/5] perf/x86/amd/power: Add AMD accumulated power reporting mechanism

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 5/5] perf/x86/amd/power: Add AMD accumulated power  reporting mechanism Peter Zijlstra <peterz@infradead.org> - 2016-01-19 13:20 +0100
    Re: [PATCH v2 5/5] perf/x86/amd/power: Add AMD accumulated power  reporting mechanism Peter Zijlstra <peterz@infradead.org> - 2016-01-20 10:30 +0100
      Re: [PATCH v2 5/5] perf/x86/amd/power: Add AMD accumulated power  reporting mechanism Peter Zijlstra <peterz@infradead.org> - 2016-01-21 10:10 +0100
        Re: [PATCH v2 5/5] perf/x86/amd/power: Add AMD accumulated power  reporting mechanism Peter Zijlstra <peterz@infradead.org> - 2016-01-21 16:20 +0100
          Re: [PATCH v2 5/5] perf/x86/amd/power: Add AMD accumulated power  reporting mechanism Peter Zijlstra <peterz@infradead.org> - 2016-01-21 17:00 +0100
          Re: [PATCH v2 5/5] perf/x86/amd/power: Add AMD accumulated power  reporting mechanism Borislav Petkov <bp@alien8.de> - 2016-01-21 18:10 +0100
            Re: [PATCH v2 5/5] perf/x86/amd/power: Add AMD accumulated power  reporting mechanism Borislav Petkov <bp@alien8.de> - 2016-01-22 19:00 +0100

#1312174 — Re: [PATCH v2 5/5] perf/x86/amd/power: Add AMD accumulated power reporting mechanism

On Thu, Jan 14, 2016 at 10:50:08AM +0800, Huang Rui wrote:
> +struct power_pmu {
> +	spinlock_t		lock;

This should be a raw_spinlock_t, as it'll be nested under other
raw_spinlock_t's.

> +	struct list_head	active_list;
> +	struct pmu		*pmu; /* pointer to power_pmu_class */
> +	local64_t		cpu_sw_pwr_ptsc;
> +};

> +static void pmu_event_start(struct perf_event *event, int mode)
> +{
> +	struct power_pmu *pmu = __this_cpu_read(amd_power_pmu);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&pmu->lock, flags);

IRQs will be disabled here.

> +	__pmu_event_start(pmu, event);
> +	spin_unlock_irqrestore(&pmu->lock, flags);
> +}
> +
> +static void pmu_event_stop(struct perf_event *event, int mode)
> +{
> +	struct power_pmu *pmu = __this_cpu_read(amd_power_pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&pmu->lock, flags);

idem

> +
> +	/* mark event as deactivated and stopped */
> +	if (!(hwc->state & PERF_HES_STOPPED)) {
> +		list_del(&event->active_entry);
> +		hwc->state |= PERF_HES_STOPPED;
> +	}
> +
> +	/* check if update of sw counter is necessary */
> +	if ((mode & PERF_EF_UPDATE) && !(hwc->state & PERF_HES_UPTODATE)) {
> +		/*
> +		 * Drain the remaining delta count out of a event
> +		 * that we are disabling:
> +		 */
> +		event_update(event, pmu);
> +		hwc->state |= PERF_HES_UPTODATE;
> +	}
> +
> +	spin_unlock_irqrestore(&pmu->lock, flags);
> +}
> +
> +static int pmu_event_add(struct perf_event *event, int mode)
> +{
> +	struct power_pmu *pmu = __this_cpu_read(amd_power_pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&pmu->lock, flags);
> +

idem

> +	hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
> +
> +	if (mode & PERF_EF_START)
> +		__pmu_event_start(pmu, event);
> +
> +	spin_unlock_irqrestore(&pmu->lock, flags);
> +
> +	return 0;
> +}


> +static int power_cpu_init(int cpu)
> +{
> +	int i, cu, ret = 0;
> +	cpumask_var_t mask, dummy_mask;
> +
> +	cu = cpu / cores_per_cu;
> +
> +	if (!zalloc_cpumask_var(&mask, GFP_KERNEL))
> +		return -ENOMEM;
> +
> +	if (!zalloc_cpumask_var(&dummy_mask, GFP_KERNEL)) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	for (i = 0; i < cores_per_cu; i++)
> +		cpumask_set_cpu(i, mask);
> +
> +	cpumask_shift_left(mask, mask, cu * cores_per_cu);
> +
> +	if (!cpumask_and(dummy_mask, mask, &cpu_mask))
> +		cpumask_set_cpu(cpu, &cpu_mask);
> +
> +	free_cpumask_var(dummy_mask);
> +out:
> +	free_cpumask_var(mask);
> +
> +	return ret;
> +}

> +static int power_cpu_notifier(struct notifier_block *self,
> +			      unsigned long action, void *hcpu)
> +{
> +	unsigned int cpu = (long)hcpu;
> +
> +	switch (action & ~CPU_TASKS_FROZEN) {
> +	case CPU_UP_PREPARE:
> +		if (power_cpu_prepare(cpu))
> +			return NOTIFY_BAD;
> +		break;
> +	case CPU_STARTING:
> +		if (power_cpu_init(cpu))
> +			return NOTIFY_BAD;

this is called with IRQs disabled, which makes those GFP_KERNEL allocs
above a pretty bad idea.

Also, note that -rt cannot actually do _any_ allocations/frees from
STARTING.

Please move the allocs/frees to PREPARE/ONLINE.

> +		break;
> +	case CPU_ONLINE:
> +	case CPU_DEAD:
> +		power_cpu_kfree(cpu);
> +		break;
> +	case CPU_DOWN_PREPARE:
> +		if (power_cpu_exit(cpu))
> +			return NOTIFY_BAD;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}

[toc] | [next] | [standalone]

#1313032

On Wed, Jan 20, 2016 at 12:48:24PM +0800, Huang Rui wrote:
> Hi Peter,
> 
> Thanks so much to your comments.
> 
> On Tue, Jan 19, 2016 at 01:12:50PM +0100, Peter Zijlstra wrote:
> > On Thu, Jan 14, 2016 at 10:50:08AM +0800, Huang Rui wrote:
> > > +struct power_pmu {
> > > +	spinlock_t		lock;
> > 
> > This should be a raw_spinlock_t, as it'll be nested under other
> > raw_spinlock_t's.
> > 
> 
> Do you mean the following spinlock operations are in hardware
> interrupts disabled case, so I need use raw_spinlock_t instead, right?


			mainline		-rt

raw_spinlock_t		spin-waits		spin-waits
spinlock_t		spin-waits		blocks (rt-mutex)
struct mutex		blocks			blocks (rt-mutex)


since these functions are themselves called with raw_spinlock_t held
(perf_event_context::lock for example, but also rq::lock), any lock
nested inside them must also be raw_spinlock_t.

I have a lockdep patch somewhere that checks these ordering things; I
should rebase and post that again.

> Use raw_spin_lock_irqsave/raw_spin_unlock_irqrestore?

pmu::{start,stop,add,del} will be called with IRQs already disabled.

> > > +static int power_cpu_init(int cpu)
> > > +{
> > > +	int i, cu, ret = 0;
> > > +	cpumask_var_t mask, dummy_mask;
> > > +
> > > +	cu = cpu / cores_per_cu;
> > > +
> > > +	if (!zalloc_cpumask_var(&mask, GFP_KERNEL))
> > > +		return -ENOMEM;
> > > +
> > > +	if (!zalloc_cpumask_var(&dummy_mask, GFP_KERNEL)) {
> > > +		ret = -ENOMEM;
> > > +		goto out;
> > > +	}
> > > +
> > > +	for (i = 0; i < cores_per_cu; i++)
> > > +		cpumask_set_cpu(i, mask);
> > > +
> > > +	cpumask_shift_left(mask, mask, cu * cores_per_cu);
> > > +
> > > +	if (!cpumask_and(dummy_mask, mask, &cpu_mask))
> > > +		cpumask_set_cpu(cpu, &cpu_mask);
> > > +
> > > +	free_cpumask_var(dummy_mask);
> > > +out:
> > > +	free_cpumask_var(mask);
> > > +
> > > +	return ret;
> > > +}
> > 
> > > +static int power_cpu_notifier(struct notifier_block *self,
> > > +			      unsigned long action, void *hcpu)
> > > +{
> > > +	unsigned int cpu = (long)hcpu;
> > > +
> > > +	switch (action & ~CPU_TASKS_FROZEN) {
> > > +	case CPU_UP_PREPARE:
> > > +		if (power_cpu_prepare(cpu))
> > > +			return NOTIFY_BAD;
> > > +		break;
> > > +	case CPU_STARTING:
> > > +		if (power_cpu_init(cpu))
> > > +			return NOTIFY_BAD;
> > 
> > this is called with IRQs disabled, which makes those GFP_KERNEL allocs
> > above a pretty bad idea.
> > 
> 
> Right, so should I use GFP_ATOMIC to allocate cpumask here?

One should not use GFP_ATOMIC if at all possible, also no, -rt cannot do
_any_ allocations from this site.

> > Also, note that -rt cannot actually do _any_ allocations/frees from
> > STARTING.
> > 
> > Please move the allocs/frees to PREPARE/ONLINE.
> > 
> 
> How about add two cpumask_var_t at power_pmu structure? Then allocate
> the two cpumask_var_t (pmu->mask, pmu->dummy_mask), and they can be
> also used on power_cpu_init.

That would work.

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

#1314014

On Thu, Jan 21, 2016 at 03:04:38PM +0800, Huang Rui wrote:
> I just quickly looked at about the spinlock on -rt mode. Because
> realtime linux kernel provides two kinds of spinlock, the original
> spinlock_t will be replaced the one which is able to sleep, actually,
> like mutex. And another one (you mentioned here, raw_spinlock_t) can
> keep on non-sleep behavior, that is the real spinlock.
> 
> And my lock here also will be nested under perf_event_context::lock,
> right?

Yep.

> > I have a lockdep patch somewhere that checks these ordering things; I
> > should rebase and post that again.
> > 
> 
> Can you CC me when you post that patch next time?

Sure.

> > One should not use GFP_ATOMIC if at all possible, also no, -rt cannot do
> > _any_ allocations from this site.
> > 
> 
> OK, that's because allocation might sleep when IRQ disabled. That's
> incorrect.

Right.

Its related to the above, the allocator locks are spinlock_t and as a
consequence of them becoming a blocking lock, spin_lock_irq() will also
no longer disable IRQs.

The CPU_STARTING notifier however will still be called with IRQs
disabled because it is CPU bringup.

So on -rt even GFP_ATOMIC will no longer work here.

> I draft an update diff that based on original patch, please take a
> look.
> 
> 8<--------------------------------------------------------------------------
> 
> diff --git a/arch/x86/kernel/cpu/perf_event_amd_power.c b/arch/x86/kernel/cpu/perf_event_amd_power.c
> index 69ef234..e71d993 100644
> --- a/arch/x86/kernel/cpu/perf_event_amd_power.c
> +++ b/arch/x86/kernel/cpu/perf_event_amd_power.c
> @@ -46,10 +46,17 @@ static unsigned int cu_num;
>  static u64 max_cu_acc_power;
>  
>  struct power_pmu {
> -	spinlock_t		lock;
> +	raw_spinlock_t		lock;
>  	struct list_head	active_list;
>  	struct pmu		*pmu; /* pointer to power_pmu_class */
>  	local64_t		cpu_sw_pwr_ptsc;
> +	/*
> +	 * These two cpumasks is used for avoiding the allocations on
> +	 * CPU_STARTING phase. Because power_cpu_prepare will be
> +	 * called on IRQs disabled status.
> +	 */
> +	cpumask_var_t		mask;
> +	cpumask_var_t		tmp_mask;
>  };
>  
>  static struct pmu pmu_class;
> @@ -126,9 +133,9 @@ static void pmu_event_start(struct perf_event *event, int mode)
>  	struct power_pmu *pmu = __this_cpu_read(amd_power_pmu);
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&pmu->lock, flags);
> +	raw_spin_lock_irqsave(&pmu->lock, flags);
>  	__pmu_event_start(pmu, event);
> -	spin_unlock_irqrestore(&pmu->lock, flags);
> +	raw_spin_unlock_irqrestore(&pmu->lock, flags);
>  }
>  
>  static void pmu_event_stop(struct perf_event *event, int mode)
> @@ -137,7 +144,7 @@ static void pmu_event_stop(struct perf_event *event, int mode)
>  	struct hw_perf_event *hwc = &event->hw;
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&pmu->lock, flags);
> +	raw_spin_lock_irqsave(&pmu->lock, flags);
>  
>  	/* mark event as deactivated and stopped */
>  	if (!(hwc->state & PERF_HES_STOPPED)) {
> @@ -155,7 +162,7 @@ static void pmu_event_stop(struct perf_event *event, int mode)
>  		hwc->state |= PERF_HES_UPTODATE;
>  	}
>  
> -	spin_unlock_irqrestore(&pmu->lock, flags);
> +	raw_spin_unlock_irqrestore(&pmu->lock, flags);
>  }
>  
>  static int pmu_event_add(struct perf_event *event, int mode)
> @@ -164,14 +171,14 @@ static int pmu_event_add(struct perf_event *event, int mode)
>  	struct hw_perf_event *hwc = &event->hw;
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&pmu->lock, flags);
> +	raw_spin_lock_irqsave(&pmu->lock, flags);
>  
>  	hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
>  
>  	if (mode & PERF_EF_START)
>  		__pmu_event_start(pmu, event);
>  
> -	spin_unlock_irqrestore(&pmu->lock, flags);
> +	raw_spin_unlock_irqrestore(&pmu->lock, flags);
>  
>  	return 0;
>  }

So for these 4 {start,stop,add,del} you can drop the irqsave/irqrestore
thing as its guaranteed that IRQs will be disabled.

> +	cpumask_clear(pmu->mask);
> +	cpumask_clear(pmu->tmp_mask);
>  
>  	for (i = 0; i < cores_per_cu; i++)
> +		cpumask_set_cpu(i, pmu->mask);
>  
> +	cpumask_shift_left(pmu->mask, pmu->mask, cu * cores_per_cu);

Couldn't you simply use topology_sibling_cpumask(cpu) instead?

>  
>  static int power_cpu_init(int cpu)
>  {
> +	struct power_pmu *pmu = per_cpu(amd_power_pmu, cpu);
> +	int i, cu;
>  
> +	if (pmu)
> +		return 0;
>  
> +	cu = cpu / cores_per_cu;
>  
>  	for (i = 0; i < cores_per_cu; i++)
> +		cpumask_set_cpu(i, pmu->mask);
>  
> +	cpumask_shift_left(pmu->mask, pmu->mask, cu * cores_per_cu);

topology_sibling_cpumask(cpu) again?

>  
> +	if (!cpumask_and(pmu->tmp_mask, pmu->mask, &cpu_mask))
>  		cpumask_set_cpu(cpu, &cpu_mask);
>  
> +	return 0;
>  }
>  
>  static int power_cpu_prepare(int cpu)
>  {
>  	struct power_pmu *pmu = per_cpu(amd_power_pmu, cpu);
>  	int phys_id = topology_physical_package_id(cpu);
> +	int ret = 0;
>  
>  	if (pmu)
>  		return 0;
> @@ -391,7 +380,17 @@ static int power_cpu_prepare(int cpu)
>  	if (!pmu)
>  		return -ENOMEM;
>  
> +	if (!zalloc_cpumask_var(&pmu->mask, GFP_KERNEL)) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	if (!zalloc_cpumask_var(&pmu->tmp_mask, GFP_KERNEL)) {
> +		ret = -ENOMEM;
> +		goto out1;
> +	}
> +
> +	raw_spin_lock_init(&pmu->lock);
>  
>  	INIT_LIST_HEAD(&pmu->active_list);
>  
> @@ -400,12 +399,21 @@ static int power_cpu_prepare(int cpu)
>  	per_cpu(amd_power_pmu, cpu) = pmu;
>  
>  	return 0;
> +
> +out1:
> +	free_cpumask_var(pmu->mask);
> +out:
> +	kfree(pmu);
> +
> +	return ret;
>  }
>  
>  static void power_cpu_kfree(int cpu)
>  {
>  	struct power_pmu *pmu = per_cpu(amd_power_pmu, cpu);
>  
> +	free_cpumask_var(pmu->mask);
> +	free_cpumask_var(pmu->tmp_mask);
>  	kfree(pmu);
>  }

Yes this should work I think.

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

#1314261

On Thu, Jan 21, 2016 at 10:42:35PM +0800, Huang Rui wrote:
> > > @@ -164,14 +171,14 @@ static int pmu_event_add(struct perf_event *event, int mode)
> > >  	struct hw_perf_event *hwc = &event->hw;
> > >  	unsigned long flags;
> > >  
> > > -	spin_lock_irqsave(&pmu->lock, flags);
> > > +	raw_spin_lock_irqsave(&pmu->lock, flags);
> > >  
> > >  	hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
> > >  
> > >  	if (mode & PERF_EF_START)
> > >  		__pmu_event_start(pmu, event);
> > >  
> > > -	spin_unlock_irqrestore(&pmu->lock, flags);
> > > +	raw_spin_unlock_irqrestore(&pmu->lock, flags);
> > >  
> > >  	return 0;
> > >  }
> > 
> > So for these 4 {start,stop,add,del} you can drop the irqsave/irqrestore
> > thing as its guaranteed that IRQs will be disabled.
> > 
> 
> OK, I will remove the lock.

No, the lock seems needed, as the list is global. Just the
irqsave/irqrestore part is superfluous.

> > > +	cpumask_clear(pmu->mask);
> > > +	cpumask_clear(pmu->tmp_mask);
> > >  
> > >  	for (i = 0; i < cores_per_cu; i++)
> > > +		cpumask_set_cpu(i, pmu->mask);
> > >  
> > > +	cpumask_shift_left(pmu->mask, pmu->mask, cu * cores_per_cu);
> > 
> > Couldn't you simply use topology_sibling_cpumask(cpu) instead?
> > 
> 
> Looks like we couldn't. That's because cores number per cu (compute
> unit) is got by CPUID 0x8000001e EBX. That relies on the CPU hardware.

Borislav? I thought the AMD compute unit stuff was modeled as the SMT
topology.

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

#1314293

On Thu, Jan 21, 2016 at 11:24:14PM +0800, Huang Rui wrote:
> On Thu, Jan 21, 2016 at 04:10:40PM +0100, Peter Zijlstra wrote:
> > On Thu, Jan 21, 2016 at 10:42:35PM +0800, Huang Rui wrote:
> > > > > @@ -164,14 +171,14 @@ static int pmu_event_add(struct perf_event *event, int mode)
> > > > >  	struct hw_perf_event *hwc = &event->hw;
> > > > >  	unsigned long flags;
> > > > >  
> > > > > -	spin_lock_irqsave(&pmu->lock, flags);
> > > > > +	raw_spin_lock_irqsave(&pmu->lock, flags);
> > > > >  
> > > > >  	hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
> > > > >  
> > > > >  	if (mode & PERF_EF_START)
> > > > >  		__pmu_event_start(pmu, event);
> > > > >  
> > > > > -	spin_unlock_irqrestore(&pmu->lock, flags);
> > > > > +	raw_spin_unlock_irqrestore(&pmu->lock, flags);
> > > > >  
> > > > >  	return 0;
> > > > >  }
> > > > 
> > > > So for these 4 {start,stop,add,del} you can drop the irqsave/irqrestore
> > > > thing as its guaranteed that IRQs will be disabled.
> > > > 
> > > 
> > > OK, I will remove the lock.
> > 
> > No, the lock seems needed, as the list is global. Just the
> > irqsave/irqrestore part is superfluous.
> > 
> 
> But actually, the lock is only used at {start,stop,add,del}. If we
> drop irqsave/irqrestore on these 4 things, there won't be any use
> cases.

But, but, the list is global !? something needs to serialize the access
to it, right?

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

#1314354

On Thu, Jan 21, 2016 at 04:10:40PM +0100, Peter Zijlstra wrote:
> > > > +	cpumask_clear(pmu->mask);
> > > > +	cpumask_clear(pmu->tmp_mask);
> > > >  
> > > >  	for (i = 0; i < cores_per_cu; i++)
> > > > +		cpumask_set_cpu(i, pmu->mask);
> > > >  
> > > > +	cpumask_shift_left(pmu->mask, pmu->mask, cu * cores_per_cu);
> > > 
> > > Couldn't you simply use topology_sibling_cpumask(cpu) instead?
> > > 
> > 
> > Looks like we couldn't. That's because cores number per cu (compute
> > unit) is got by CPUID 0x8000001e EBX. That relies on the CPU hardware.
> 
> Borislav? I thought the AMD compute unit stuff was modeled as the SMT
> topology.

I would think so too:

	smp_num_siblings = ((ebx >> 8) & 3) + 1;

gets set based on that CPUID leaf above. And that value is
CoresPerComputeUnit which needs to be incremented by 1 to get the actual
count of cores in a compute unit.

And that participates in the setting of topology_sibling_cpumask() in
set_cpu_sibling_map().

And that looks correct on my system here:

$ grep -EriIn . /sys/devices/system/cpu/cpu?/topology/* | grep thread_siblings
/sys/devices/system/cpu/cpu0/topology/thread_siblings:1:03
/sys/devices/system/cpu/cpu0/topology/thread_siblings_list:1:0-1
/sys/devices/system/cpu/cpu1/topology/thread_siblings:1:03
/sys/devices/system/cpu/cpu1/topology/thread_siblings_list:1:0-1
/sys/devices/system/cpu/cpu2/topology/thread_siblings:1:0c
/sys/devices/system/cpu/cpu2/topology/thread_siblings_list:1:2-3
/sys/devices/system/cpu/cpu3/topology/thread_siblings:1:0c
/sys/devices/system/cpu/cpu3/topology/thread_siblings_list:1:2-3
/sys/devices/system/cpu/cpu4/topology/thread_siblings:1:30
/sys/devices/system/cpu/cpu4/topology/thread_siblings_list:1:4-5
/sys/devices/system/cpu/cpu5/topology/thread_siblings:1:30
/sys/devices/system/cpu/cpu5/topology/thread_siblings_list:1:4-5
/sys/devices/system/cpu/cpu6/topology/thread_siblings:1:c0
/sys/devices/system/cpu/cpu6/topology/thread_siblings_list:1:6-7
/sys/devices/system/cpu/cpu7/topology/thread_siblings:1:c0
/sys/devices/system/cpu/cpu7/topology/thread_siblings_list:1:6-7

and when we look at what CPUID reports:

$ cpuid -r | grep -E "^\s+0x8000001e" | awk '{ print $4 }'
ebx=0x00000100
ebx=0x00000100
ebx=0x00000101
ebx=0x00000101
ebx=0x00000102
ebx=0x00000102
ebx=0x00000103
ebx=0x00000103

We see that [15:8] is CoresPerComputeUnit which is + 1, so 2 cores per
compute unit.

And slice [7:0] gives the compute unit (CU) id of each core, so cores 0
and 1 are CU0, 2 and 3 are CU1 and so on...

So Rui, why do you say you can't use topology_sibling_cpumask()?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

#1315179

On Fri, Jan 22, 2016 at 04:04:40PM +0800, Huang Rui wrote:
> OK, you're right. Peter, Boris, thanks for your information.
> I might need look at topology deeper. :-)
> 
> So how about below update:

Please send a full patch, not those diffs ontop - I don't know about
Peter but I absolutely can't grok it this way.

> BTW, "smp_num_siblings = ((ebx >> 8) & 3) + 1" should not put under
> init_amd(), we would better move it to bsp_init_amd(). Because the AMD
> "smp_num_siblings" number must be constant.

You can send me a fix once your other stuff hits tip and you can base it
ontop.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web