[PATCH] kdb: avoid on-stack cpumask, old-style accessors

[PATCH] kdb: avoid on-stack cpumask, old-style accessors

[Rusty Russell]

That's quite a big stack already; I assume moving it to a static bitmap
is OK for this usage (a cpumask_var_t might require kmalloc, not sure
that's a good idea here).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Cc: kgdb-bugreport@lists.sourceforge.net
Cc: Jason Wessel <jason.wessel@windriver.com>
---
 kernel/debug/kdb/kdb_main.c |   22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -2598,6 +2598,9 @@ static int kdb_summary(int argc, const c
 	return 0;
 }
 
+/* Not sure when to initialize this, so make it a static bitmap. */
+static DECLARE_BITMAP(per_cpu_suppress, NR_CPUS);
+
 /*
  * kdb_per_cpu - This function implements the 'per_cpu' command.
  */
@@ -2605,7 +2608,7 @@ static int kdb_per_cpu(int argc, const c
 {
 	char buf[256], fmtstr[64];
 	kdb_symtab_t symtab;
-	cpumask_t suppress = CPU_MASK_NONE;
+	struct cpumask *suppress = to_cpumask(per_cpu_suppress);
 	int cpu, diag;
 	unsigned long addr, val, bytesperword = 0, whichcpu = ~0UL;
 
@@ -2649,7 +2652,7 @@ static int kdb_per_cpu(int argc, const c
 #define KDB_PCU(cpu) 0
 #endif
 #endif
-
+	cpumask_clear(suppress);
 	for_each_online_cpu(cpu) {
 		if (whichcpu != ~0UL && whichcpu != cpu)
 			continue;
@@ -2662,7 +2665,7 @@ static int kdb_per_cpu(int argc, const c
 		}
 #ifdef	CONFIG_SMP
 		if (!val) {
-			cpu_set(cpu, suppress);
+			cpumask_set_cpu(cpu, suppress);
 			continue;
 		}
 #endif	/* CONFIG_SMP */
@@ -2671,17 +2674,16 @@ static int kdb_per_cpu(int argc, const c
 			bytesperword == KDB_WORD_SIZE,
 			1, bytesperword, 1, 1, 0);
 	}
-	if (cpus_weight(suppress) == 0)
+	if (cpumask_weight(suppress) == 0)
 		return 0;
 	kdb_printf("Zero suppressed cpu(s):");
-	for (cpu = first_cpu(suppress); cpu < num_possible_cpus();
-	     cpu = next_cpu(cpu, suppress)) {
+	for_each_cpu(cpu, suppress) {
 		kdb_printf(" %d", cpu);
-		if (cpu == num_possible_cpus() - 1 ||
-		    next_cpu(cpu, suppress) != cpu + 1)
+		if (cpu == nr_cpu_ids - 1 ||
+		    cpumask_next(cpu, suppress) != cpu + 1)
 			continue;
-		while (cpu < num_possible_cpus() &&
-		       next_cpu(cpu, suppress) == cpu + 1)
+		while (cpu < nr_cpu_ids &&
+		       cpumask_next(cpu, suppress) == cpu + 1)
 			++cpu;
 		kdb_printf("-%d", cpu);
 	}

Re: [PATCH] kdb: avoid on-stack cpumask, old-style accessors

[Jason Wessel]

On 10/27/2010 05:00 AM, Rusty Russell wrote:
> That's quite a big stack already; I assume moving it to a static bitmap
> is OK for this usage (a cpumask_var_t might require kmalloc, not sure
> that's a good idea here).
>
>   
Hi Rusty,

This patch seems fine to me, and I'll gladly put it in the merge queue.

The kdb shell has a 2 phase initialization, in order to support early
debugging (before the kernel allocators are ready as well as before
console init).  In the later initialization phase, kmalloc() is
available.  We could return an error up until the structure is allocated
by the kdb late init phase.  It is also possible to avoid allocating the
structure at all if kdb is not configured and free it if you deactivate kdb.

If you are interested or think we should go down that path, let me know.

Thanks,
Jason.

Re: [PATCH] kdb: avoid on-stack cpumask, old-style accessors

[Rusty Russell]

On Wed, 27 Oct 2010 10:14:32 pm Jason Wessel wrote:
> On 10/27/2010 05:00 AM, Rusty Russell wrote:
> > That's quite a big stack already; I assume moving it to a static bitmap
> > is OK for this usage (a cpumask_var_t might require kmalloc, not sure
> > that's a good idea here).
> >
> >   
> Hi Rusty,
> 
> This patch seems fine to me, and I'll gladly put it in the merge queue.

Thanks!

> The kdb shell has a 2 phase initialization, in order to support early
> debugging (before the kernel allocators are ready as well as before
> console init).  In the later initialization phase, kmalloc() is
> available.  We could return an error up until the structure is allocated
> by the kdb late init phase.  It is also possible to avoid allocating the
> structure at all if kdb is not configured and free it if you deactivate kdb.

We could, or look harder at that code to see if the mask is really needed?

Thanks,
Rusty.

Re: [PATCH] kdb: avoid on-stack cpumask, old-style accessors

[Jason Wessel]

On 10/29/2010 04:48 AM, Rusty Russell wrote:
> 
> We could, or look harder at that code to see if the mask is really needed?

How right you are!  We don't even need the mask in the first place.
The code can be refactored at the same time it is made to actually
work.  It turns out that the per_cpu command doesn't work properly at
the moment in the mainline.

That means I will drop your patch, but I really do appreciate you
pointing this out.

Thanks!
Jason.

--

From: Jason Wessel <jason.wessel@windriver.com>
Subject: [PATCH] kdb: fix per_cpu command to remove supress mask
Date: Fri Oct 29 08:04:16 CDT 2010

Rusty pointed out that the per_cpu command uses up lots of space on
the stack and the cpu supress mask is probably not needed.

This patch removes the need for the supress mask as well as fixing up
the following problems with the kdb per_cpu command:
  * The per_cpu command should allow an address as an argument
  * When you have more data than can be displayed on one screen allow
    the user to break out of the print loop.

Reported-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 kernel/debug/kdb/kdb_main.c |   46 ++++++++++----------------------------------
 1 file changed, 11 insertions(+), 35 deletions(-)

--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -2603,20 +2603,17 @@ static int kdb_summary(int argc, const c
  */
 static int kdb_per_cpu(int argc, const char **argv)
 {
-	char buf[256], fmtstr[64];
-	kdb_symtab_t symtab;
-	cpumask_t suppress = CPU_MASK_NONE;
-	int cpu, diag;
-	unsigned long addr, val, bytesperword = 0, whichcpu = ~0UL;
+	char fmtstr[64];
+	int cpu, diag, nextarg = 1;
+	unsigned long addr, symaddr, val, bytesperword = 0, whichcpu = ~0UL;
 
 	if (argc < 1 || argc > 3)
 		return KDB_ARGCOUNT;
 
-	snprintf(buf, sizeof(buf), "per_cpu__%s", argv[1]);
-	if (!kdbgetsymval(buf, &symtab)) {
-		kdb_printf("%s is not a per_cpu variable\n", argv[1]);
-		return KDB_BADADDR;
-	}
+	diag = kdbgetaddrarg(argc, argv, &nextarg, &symaddr, NULL, NULL);
+	if (diag)
+		return diag;
+
 	if (argc >= 2) {
 		diag = kdbgetularg(argv[2], &bytesperword);
 		if (diag)
@@ -2649,46 +2646,25 @@ static int kdb_per_cpu(int argc, const c
 #define KDB_PCU(cpu) 0
 #endif
 #endif
-
 	for_each_online_cpu(cpu) {
+		if (KDB_FLAG(CMD_INTERRUPT))
+			return 0;
+
 		if (whichcpu != ~0UL && whichcpu != cpu)
 			continue;
-		addr = symtab.sym_start + KDB_PCU(cpu);
+		addr = symaddr + KDB_PCU(cpu);
 		diag = kdb_getword(&val, addr, bytesperword);
 		if (diag) {
 			kdb_printf("%5d " kdb_bfd_vma_fmt0 " - unable to "
 				   "read, diag=%d\n", cpu, addr, diag);
 			continue;
 		}
-#ifdef	CONFIG_SMP
-		if (!val) {
-			cpu_set(cpu, suppress);
-			continue;
-		}
-#endif	/* CONFIG_SMP */
 		kdb_printf("%5d ", cpu);
 		kdb_md_line(fmtstr, addr,
 			bytesperword == KDB_WORD_SIZE,
 			1, bytesperword, 1, 1, 0);
 	}
-	if (cpus_weight(suppress) == 0)
-		return 0;
-	kdb_printf("Zero suppressed cpu(s):");
-	for (cpu = first_cpu(suppress); cpu < num_possible_cpus();
-	     cpu = next_cpu(cpu, suppress)) {
-		kdb_printf(" %d", cpu);
-		if (cpu == num_possible_cpus() - 1 ||
-		    next_cpu(cpu, suppress) != cpu + 1)
-			continue;
-		while (cpu < num_possible_cpus() &&
-		       next_cpu(cpu, suppress) == cpu + 1)
-			++cpu;
-		kdb_printf("-%d", cpu);
-	}
-	kdb_printf("\n");
-
 #undef KDB_PCU
-
 	return 0;
 }