On 16 June 2011 23:13, Russell King - ARM Linux <li...@arm.linux.org.uk> 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
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
