Re: [PATCH v6 2/6] hwmon: (fam15h_power) Add compute unit accumulated power

[Guenter Roeck <linux@roeck-us.net>]

On 04/06/2016 10:05 PM, Huang Rui wrote:
> On Wed, Apr 06, 2016 at 08:30:25AM -0700, Guenter Roeck wrote:
>> On Wed, Apr 06, 2016 at 03:44:11PM +0800, Huang Rui wrote:
>>>
>>> +static void do_read_registers_on_cu(void *_data)
>>> +{
>>> +	struct fam15h_power_data *data = _data;
>>> +	int cpu, cu;
>>> +
>>> +	cpu = smp_processor_id();
>>> +
>>
>> Is this function now defined in non-SMP code ? If so, can you point me to the
>> patch or branch introducing it ? It doesn't seem to be in mainline or in -next
>> unless I am missing it.
>>
>
> In include/linux/smp.h
>
>
> #else /* !SMP */
>
> static inline void smp_send_stop(void) { }
>
> /*
>   *      These macros fold the SMP functionality into a single CPU system
>   */
> #define raw_smp_processor_id()                  0
>
> ...
>
> /*
>   * smp_processor_id(): get the current CPU ID.
>   *
>   * if DEBUG_PREEMPT is enabled then we check whether it is
>   * used in a preemption-safe way. (smp_processor_id() is safe
>   * if it's used in a preemption-off critical section, or in
>   * a thread that is bound to the current CPU.)
>   *
>   * NOTE: raw_smp_processor_id() is for internal use only
>   * (smp_processor_id() is the preferred variant), but in rare
>   * instances it might also be used to turn off false positives
>   * (i.e. smp_processor_id() use that the debugging code reports but
>   * which use for some reason is legal). Don't use this to hack around
>   * the warning message, as your code might not work under PREEMPT.
>   */
> #ifdef CONFIG_DEBUG_PREEMPT
>    extern unsigned int debug_smp_processor_id(void);
> # define smp_processor_id() debug_smp_processor_id()
> #else
> # define smp_processor_id() raw_smp_processor_id()
> #endif
>
>
> Actually smp_processor_id() should returns 0 if we disable CONFIG_SMP.
>
>>> +	/*
>>> +	 * With the new x86 topology modelling, cpu core id actually
>>> +	 * is compute unit id.
>>> +	 */
>>> +	cu = cpu_data(cpu).cpu_core_id;
>>> +
>>> +	rdmsrl_safe(MSR_F15H_CU_PWR_ACCUMULATOR, &data->cu_acc_power[cu]);
>>> +}
>>> +
>>> +/*
>>> + * This function is only able to be called when CPUID
>>> + * Fn8000_0007:EDX[12] is set.
>>> + */
>>> +static int read_registers(struct fam15h_power_data *data)
>>> +{
>>> +	int this_cpu, ret, cpu;
>>> +	int core, this_core;
>>> +	cpumask_var_t mask;
>>> +
>>> +	ret = zalloc_cpumask_var(&mask, GFP_KERNEL);
>>> +	if (!ret)
>>> +		return -ENOMEM;
>>> +
>>> +	get_online_cpus();
>>> +	this_cpu = smp_processor_id();
>>> +
>>> +	/*
>>> +	 * Choose the first online core of each compute unit, and then
>>> +	 * read their MSR value of power and ptsc in a single IPI,
>>> +	 * because the MSR value of CPU core represent the compute
>>> +	 * unit's.
>>> +	 */
>>> +	core = -1;
>>> +
>>> +	for_each_online_cpu(cpu) {
>>> +		this_core = topology_core_id(cpu);
>>> +
>>> +		if (this_core == core)
>>> +			continue;
>>> +
>>> +		core = this_core;
>>> +
>> Sorry if I missed some context - is it guaranteed that all cores in the same
>> compute unit are returned next to each other from for_each_online_cpu() ?
>>
>
> Yes, there is a documentation which introduced from v4.6-rc2:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=f7be8610bca88e59dd2fd5d98fcbc5031ef0e079
>
>     - topology_core_id();
>
>      The ID of the core to which a thread belongs. It is also printed in /proc/cpuinfo
>      "core_id."
>
> ...
>
>        AMD nomenclature for CMT systems:
>
>          [node 0] -> [Compute Unit 0] -> [Compute Unit Core 0] -> Linux CPU 0
>                                       -> [Compute Unit Core 1] -> Linux CPU 1
>                   -> [Compute Unit 1] -> [Compute Unit Core 0] -> Linux CPU 2
>                                       -> [Compute Unit Core 1] -> Linux CPU 3
>
> ray@hr-ub:~/tip$ cat /proc/cpuinfo | grep "core id"
> core id         : 0
> core id         : 0
> core id         : 1
> core id         : 1
>
> "this_core" here actually means the [Compute Unit] id which current
> [Compute Unit Core] belongs to. And "cpu" here means the [Compute Unit Core].
>
Ok, thanks.

Guenter