Re: [RFC] Add Arm cpu topology definition

[Vincent Guittot]

On 16 June 2011 23:13, Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx> wrote:
> On Thu, Jun 16, 2011 at 10:49:13AM +0200, Vincent Guittot wrote:
>> @@ -219,6 +219,24 @@ source "kernel/Kconfig.freezer"
>>
>>  menu "System Type"
>>
>> +config SCHED_MC
>> +     bool "Multi-core scheduler support"
>> +     depends on SMP && ARM_CPU_TOPOLOGY
>> +     default n
>> +     help
>> +       Multi-core scheduler support improves the CPU scheduler's decision
>> +       making when dealing with multi-core CPU chips at a cost of slightly
>> +       increased overhead in some places. If unsure say N here.
>> +
>> +config SCHED_SMT
>> +     bool "SMT scheduler support"
>> +     depends on SMP && ARM_CPU_TOPOLOGY
>> +     default n
>> +     help
>> +       Improves the CPU scheduler's decision making when dealing with
>> +       MultiThreading at a cost of slightly increased overhead in some
>> +       places. If unsure say N here.
>
> Why place these in the "system type" menu?  Wouldn't it be better to
> place them along side ARM_CPU_TOPOLOGY, and place that along side
> the SMP option too?
>

yes, it's a better place

>> +
>>  config MMU
>>       bool "MMU-based Paged Memory Management Support"
>>       default y
>> @@ -1062,6 +1080,14 @@ if !MMU
>>  source "arch/arm/Kconfig-nommu"
>>  endif
>>
>> +config ARM_CPU_TOPOLOGY
>> +     bool "Support cpu topology definition"
>> +     depends on SMP && CPU_V7
>> +     help
>> +       Support Arm cpu topology definition. The MPIDR register defines
>> +       affinity between processors which is used to set the cpu
>> +       topology of an Arm System.
>
> Is there a reason you'd want to disable this?
>

In fact, I only want to disable sched_mc and sched_smt. I'm going to
add default y for ARM_CPU_TOPOLOGY

> Please also note that it's "ARM" not "Arm".

OK

>
>> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
>> index accbd7c..cb90d0a 100644
>> --- a/arch/arm/include/asm/topology.h
>> +++ b/arch/arm/include/asm/topology.h
>> @@ -1,6 +1,39 @@
>>  #ifndef _ASM_ARM_TOPOLOGY_H
>>  #define _ASM_ARM_TOPOLOGY_H
>>
>> +#ifdef CONFIG_ARM_CPU_TOPOLOGY
>> +
>> +#include <linux/cpumask.h>
>> +
>> +struct cputopo_arm {
>> +     int thread_id;
>> +     int core_id;
>> +     int socket_id;
>> +     cpumask_t thread_sibling;
>> +     cpumask_t core_sibling;
>> +};
>> +
>> +extern struct cputopo_arm cpu_topology[NR_CPUS];
>> +
>> +#define topology_physical_package_id(cpu)    (cpu_topology[cpu].socket_id)
>> +#define topology_core_id(cpu)                (cpu_topology[cpu].core_id)
>> +#define topology_core_cpumask(cpu)   (&(cpu_topology[cpu].core_sibling))
>> +#define topology_thread_cpumask(cpu) (&(cpu_topology[cpu].thread_sibling))
>
> The thing which naggs me about this is that its a static array, and we
> know from x86 that static arrays of per-cpu data have various issues
> (cache line bouncing, as well as limitations when we end up with large
> numbers of CPUs.)
>

The array is updated sequentially by each processor during boot. Then,
it should be used by one cpu when building the sched_domain and when
calling the topology sysfs entry.
We should not have so many cases where several cpu are accessing
simultaneously the cells of the array.

> Lastly, x86 seems to do this:
>
> #define arch_provides_topology_pointers         yes
>
> and the only effect I can find of that define is in drivers/base/topology.c:
>
> #ifdef arch_provides_topology_pointers
> ...
>        unsigned int cpu = dev->id;                                     \
>        return show_cpumap(0, topology_##name(cpu), buf);               \
> ...
> #else
> ...
>        return show_cpumap(0, topology_##name(dev->id), buf);           \
> ...
> #endif
>
> dev->id is type 'u32' which devolves to 'unsigned int' on all supported
> arches.  So it looks to me like this arch_provides_topology_pointers
> define is completely pointless and we just have useless obfuscation for
> the hell of it.
>
> That's not a criticism of your patch, it's pointing out a difference
> between x86 and our implementation.
>

I haven't set arch_provides_topology_pointers because I can't find a
difference between setting it or not

>> +#include <linux/cpu.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/init.h>
>> +#include <linux/percpu.h>
>> +#include <linux/node.h>
>> +#include <linux/nodemask.h>
>> +#include <linux/sched.h>
>> +
>> +#include <asm/cacheflush.h>
>> +#include <asm/topology.h>
>> +
>> +#define hard_smp_mpidr() \
>> +     ({ \
>> +             unsigned int cpunum; \
>> +             __asm__("mrc p15, 0, %0, c0, c0, 5"     \
>> +                     : "=r" (cpunum)); \
>> +             cpunum; \
>> +     })
>
> Please add a definition for CPUID_MPIDR to arch/arm/include/asm/cputype.h
> and a read_cpuid_mpidr() function, and use that instead.
>

ok
 I can directly use read_cpuid with a definition for CPUID_MPIDR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/