x86: Reduce pressure on stack from cpumask usage -v2

PATCH In review [email protected] 2 participants 5 messages Nov 21, 2007

Subsystems (5)

Memory management x86 Scheduler x86 MM +1 more subsystems

lore

Share & tools

lore

[email protected]

author

2007-11-21 10:02 UTC #0 lore raw

cover x86: Reduce pressure on stack from cpumask usage -v2

v2:
- fix some compile errors when NR_CPUS > default for ia386 (128 & 4096)
- remove unneccessary includes

Convert cpumask_of_cpu to use a static percpu data array and set_cpus_allowed to pass the cpumask_t arg as a pointer.

Conditioned on NR_CPUS > BITS_PER_LONG.

Compiled and tested for i386 and x86_64. I'd appreciate feedback on other architectures.

(Note: there are still compile/test errors when NR_CPUS > 256 due to cpu id being 8 bits among other things.)

Signature · 5 lines

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

[email protected]

author

2007-11-21 10:02 UTC #1 lore raw

PATCH 1/2 cpumask: Convert cpumask_of_cpu to static array -v2

Here is a simple patch to use a per cpu cpumask instead of constructing one on the stack. I have been running awhile with this one:

Do not use stack to allocate cpumask for cpumask_of_cpu

Signed-off-by Christoph Lameter <[email protected]>

Modified to be used only if NR_CPUS is greater than the BITS_PER_LONG as well as fix cases where !SMP and both NR_CPUS > and < BITS_PER_LONG.

Signed-off-by Mike Travis <[email protected]>

---
 arch/x86/kernel/apic_32.c      |    1 +
 arch/x86/kernel/apic_64.c      |    1 +
 arch/x86/kernel/cpu/intel.c    |    1 +
 arch/x86/kernel/smp_32.c       |    1 +
 arch/x86/kernel/smp_64.c       |    1 +
 arch/x86/kernel/smpcommon_32.c |    1 +
 arch/x86/mach-generic/bigsmp.c |    1 +
 arch/x86/mach-generic/summit.c |    1 +
 arch/x86/mm/numa_64.c          |    1 +
 include/linux/cpumask.h        |    4 ++++
 include/linux/sched.h          |    4 ++++
 kernel/sched.c                 |    9 +++++++++
 12 files changed, 26 insertions(+)

include/linux/cpumask.h +4 -0

@@ -222,6 +222,9 @@ int __next_cpu(int n, const cpumask_t *s
 #define next_cpu(n, src)	1
 #endif
 
+#if defined(CONFIG_SMP) && (NR_CPUS > BITS_PER_LONG)
+#define cpumask_of_cpu(cpu)    per_cpu(cpu_mask, cpu)
+#else
 #define cpumask_of_cpu(cpu)						\
 ({									\
 	typeof(_unused_cpumask_arg_) m;					\

@@ -233,6 +236,7 @@ int __next_cpu(int n, const cpumask_t *s
 	}								\
 	m;								\
 })
+#endif
 
 #define CPU_MASK_LAST_WORD BITMAP_LAST_WORD_MASK(NR_CPUS)

kernel/sched.c +9 -0

@@ -6732,6 +6732,11 @@ static void init_cfs_rq(struct cfs_rq *c
 	cfs_rq->min_vruntime = (u64)(-(1LL << 20));
 }
 
+#if NR_CPUS > BITS_PER_LONG
+DEFINE_PER_CPU(cpumask_t, cpu_mask);
+EXPORT_PER_CPU_SYMBOL(cpu_mask);
+#endif
+
 void __init sched_init(void)
 {
 	int highest_cpu = 0;

@@ -6741,6 +6746,10 @@ void __init sched_init(void)
 		struct rt_prio_array *array;
 		struct rq *rq;
 
+#if NR_CPUS > BITS_PER_LONG
+		/* This makes cpumask_of_cpu work */
+		cpu_set(i, per_cpu(cpu_mask, i));
+#endif
 		rq = cpu_rq(i);
 		spin_lock_init(&rq->lock);
 		lockdep_set_class(&rq->lock, &rq->rq_lock_key);

arch/x86/mm/numa_64.c +1 -0

@@ -11,6 +11,7 @@
 #include <linux/ctype.h>
 #include <linux/module.h>
 #include <linux/nodemask.h>
+#include <linux/sched.h>
 
 #include <asm/e820.h>
 #include <asm/proto.h>

include/linux/sched.h +4 -0

@@ -2024,6 +2024,10 @@ static inline void migration_init(void)
 #define TASK_SIZE_OF(tsk)	TASK_SIZE
 #endif
 
+#if defined(CONFIG_SMP) && (NR_CPUS > BITS_PER_LONG)
+DECLARE_PER_CPU(cpumask_t, cpu_mask);
+#endif
+
 #endif /* __KERNEL__ */
 
 #endif

arch/x86/kernel/apic_32.c +1 -0

@@ -28,6 +28,7 @@
 #include <linux/acpi_pmtmr.h>
 #include <linux/module.h>
 #include <linux/dmi.h>
+#include <linux/sched.h>
 
 #include <asm/atomic.h>
 #include <asm/smp.h>

arch/x86/kernel/apic_64.c +1 -0

@@ -27,6 +27,7 @@
 #include <linux/ioport.h>
 #include <linux/clockchips.h>
 #include <linux/acpi_pmtmr.h>
+#include <linux/sched.h>
 
 #include <asm/atomic.h>
 #include <asm/smp.h>

arch/x86/kernel/smp_32.c +1 -0

@@ -19,6 +19,7 @@
 #include <linux/interrupt.h>
 #include <linux/cpu.h>
 #include <linux/module.h>
+#include <linux/sched.h>
 
 #include <asm/mtrr.h>
 #include <asm/tlbflush.h>

arch/x86/kernel/smp_64.c +1 -0

@@ -18,6 +18,7 @@
 #include <linux/kernel_stat.h>
 #include <linux/mc146818rtc.h>
 #include <linux/interrupt.h>
+#include <linux/sched.h>
 
 #include <asm/mtrr.h>
 #include <asm/pgalloc.h>

arch/x86/kernel/smpcommon_32.c +1 -0

@@ -2,6 +2,7 @@
  * SMP stuff which is common to all sub-architectures.
  */
 #include <linux/module.h>
+#include <linux/sched.h>
 #include <asm/smp.h>
 
 DEFINE_PER_CPU(unsigned long, this_cpu_off);

arch/x86/mach-generic/bigsmp.c +1 -0

@@ -14,6 +14,7 @@
 #include <linux/smp.h>
 #include <linux/init.h>
 #include <linux/dmi.h>
+#include <linux/sched.h>
 #include <asm/mach-bigsmp/mach_apic.h>
 #include <asm/mach-bigsmp/mach_apicdef.h>
 #include <asm/mach-bigsmp/mach_ipi.h>

arch/x86/mach-generic/summit.c +1 -0

@@ -13,6 +13,7 @@
 #include <linux/string.h>
 #include <linux/smp.h>
 #include <linux/init.h>
+#include <linux/sched.h>
 #include <asm/mach-summit/mach_apic.h>
 #include <asm/mach-summit/mach_apicdef.h>
 #include <asm/mach-summit/mach_ipi.h>

arch/x86/kernel/cpu/intel.c +1 -0

@@ -6,6 +6,7 @@
 #include <linux/smp.h>
 #include <linux/thread_info.h>
 #include <linux/module.h>
+#include <linux/sched.h>
 
 #include <asm/processor.h>
 #include <asm/pgtable.h>

Signature · 5 lines

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

[email protected]

author

2007-11-21 10:02 UTC #2 lore raw

PATCH 2/2 cpumask: Convert set_cpus_allowed to use ptr for cpumask arg -v2

In reply to [email protected]

Avoid pushing cpumask variables onto stack when calling set_cpus_allowed when NR_CPUS > BITS_PER_LONG by passing cpumast_t arg as a pointer.

Signed-off-by Mike Travis <[email protected]>

---
 include/linux/sched.h |   11 +++++++++--
 kernel/sched.c        |   22 ++++++++++++++++------
 2 files changed, 25 insertions(+), 8 deletions(-)

include/linux/sched.h +9 -2

@@ -1435,8 +1435,15 @@ static inline void put_task_struct(struc
 #define tsk_used_math(p) ((p)->flags & PF_USED_MATH)
 #define used_math() tsk_used_math(current)
 
-#ifdef CONFIG_SMP
-extern int set_cpus_allowed(struct task_struct *p, cpumask_t new_mask);
+#if defined(CONFIG_SMP)
+#  if NR_CPUS > BITS_PER_LONG
+     /* avoid pushing cpumask variable onto the stack */
+#    define set_cpus_allowed(p, new_mask) __set_cpus_allowed((p), &(new_mask))
+     extern int __set_cpus_allowed(struct task_struct *p, cpumask_t *new_mask);
+#  else
+#    define set_cpus_allowed(p, new_mask) __set_cpus_allowed((p), (new_mask))
+     extern int __set_cpus_allowed(struct task_struct *p, cpumask_t new_mask);
+#  endif
 #else
 static inline int set_cpus_allowed(struct task_struct *p, cpumask_t new_mask)
 {

kernel/sched.c +16 -6

@@ -5036,7 +5036,16 @@ static inline void sched_init_granularit
  * task must not exit() & deallocate itself prematurely.  The
  * call is not atomic; no spinlocks may be held.
  */
-int set_cpus_allowed(struct task_struct *p, cpumask_t new_mask)
+#if NR_CPUS > BITS_PER_LONG
+/*
+ * avoid pushing a large CPU count cpumask_t variable onto stack
+ * (relies on the fact that new_mask is a const arg to subfunctions)
+ */
+#define CPU_MASK_VAR(v)		*v
+#else
+#define CPU_MASK_VAR(v)		v
+#endif
+int __set_cpus_allowed(struct task_struct *p, cpumask_t CPU_MASK_VAR(new_mask))
 {
 	struct migration_req req;
 	unsigned long flags;

@@ -5044,17 +5053,17 @@ int set_cpus_allowed(struct task_struct 
 	int ret = 0;
 
 	rq = task_rq_lock(p, &flags);
-	if (!cpus_intersects(new_mask, cpu_online_map)) {
+	if (!cpus_intersects(CPU_MASK_VAR(new_mask), cpu_online_map)) {
 		ret = -EINVAL;
 		goto out;
 	}
 
-	p->cpus_allowed = new_mask;
+	p->cpus_allowed = CPU_MASK_VAR(new_mask);
 	/* Can the task run on the task's current CPU? If so, we're done */
-	if (cpu_isset(task_cpu(p), new_mask))
+	if (cpu_isset(task_cpu(p), CPU_MASK_VAR(new_mask)))
 		goto out;
 
-	if (migrate_task(p, any_online_cpu(new_mask), &req)) {
+	if (migrate_task(p, any_online_cpu(CPU_MASK_VAR(new_mask)), &req)) {
 		/* Need help from migration thread: drop lock and wait. */
 		task_rq_unlock(rq, &flags);
 		wake_up_process(rq->migration_thread);

@@ -5067,7 +5076,8 @@ out:
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(set_cpus_allowed);
+#undef CPU_MASK_VAR	/* keep CPU_MASK_VAR local */
+EXPORT_SYMBOL_GPL(__set_cpus_allowed);
 
 /*
  * Move (not current) task off this cpu, onto dest cpu.  We're doing

Signature · 5 lines

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andi Kleen

2007-11-21 10:18 UTC #3 lore raw

In reply to [email protected]

Quoted 7 lines · On Wednesday 21 November 2007 11:02:01 [email protected] wrote:

v2:
- fix some compile errors when NR_CPUS > default for ia386 (128 & 4096)
- remove unneccessary includes
Convert cpumask_of_cpu to use a static percpu data array and set_cpus_allowed to pass the cpumask_t arg as a pointer.

I'm not sure that is too useful alone because you didn't solve the set_cpus_allowed(oldmask) problem.

-Andi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mike Travis

author

2007-11-21 16:30 UTC #4 lore raw

Quoted 10 lines · Andi Kleen

Quoted 6 lines · On Wednesday 21 November 2007 11:02:01 [email protected] wrote:
v2:
- fix some compile errors when NR_CPUS > default for ia386 (128 & 4096)
- remove unnecessary includes
Convert cpumask_of_cpu to use a static percpu data array and set_cpus_allowed to pass the cpumask_t arg as a pointer.
I'm not sure that is too useful alone because you didn't solve the set_cpus_allowed(oldmask) problem.

Passing by reference does reduce the amount of data pushed onto the stack for all cases.

I've been looking at a couple of options to fix up the rest of the cpumask_t users. The least disruptive would be to make cpumask_t a "const *cpumask" type and then fix up instances where a cpumask var is needed. It does get messy attempting to keep cpumask_t a simple integer in the NR_CPUS <= BITS_PER_LONG case, in a transparent manner.

The other is to continue to modify subroutines that expect cpumask_t arguments and work back up the call chain passing by reference instead. In this case I could use the idea of allocating a "scratch" cpumask_t array in the task struct (or somewhere else?) to accommodate trivial changes, to lessen local frame storage. Again though, the code gets messy in the same manner as above.

The set_cpus_allowed is about 30% of the "cpumask_t pass by value" code though much lower in call usage. The highest call count goes to the send_IPI_mask, irq and smp_call functions.

Btw, is there a good source code call trace analyzer tool around someplace?

Thanks,
Mike