LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] kdb: Adopt scheduler's task clasification
@ 2021-09-16 15:42 Daniel Thompson
  2021-09-16 16:28 ` Doug Anderson
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Daniel Thompson @ 2021-09-16 15:42 UTC (permalink / raw)
  To: Jason Wessel, Daniel Thompson, Douglas Anderson
  Cc: Xiang wangx, jing yangyang, kgdb-bugreport, linux-kernel, patches

Currently kdb contains some open-coded routines to generate a summary
character for each task. This code currently issues warnings, is
almost certainly broken and won't make any sense to any kernel dev who
has ever used /proc to examine tasks (D means uninterruptible?).

Fix both the warning and the potential for confusion but adopting the
scheduler's task clasification. Whilst doing this we also simplify the
filtering by using mask strings directly (this means we don't have to
guess all the characters the scheduler might give us).

Unfortunately we can't quite adopt the scheudler classification it in
its entirity because, whilst we can tolerate some changes to the filter
characters, we need to keep I as a means to identify idle CPUs rather than
system daemons that don't contribute to the load average! Naturally there
is quite a large comment discussing this.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 kernel/debug/kdb/kdb_bt.c      |  14 ++--
 kernel/debug/kdb/kdb_main.c    |  15 ++---
 kernel/debug/kdb/kdb_private.h |   4 +-
 kernel/debug/kdb/kdb_support.c | 118 ++++++++-------------------------
 4 files changed, 43 insertions(+), 108 deletions(-)

diff --git a/kernel/debug/kdb/kdb_bt.c b/kernel/debug/kdb/kdb_bt.c
index 1f9f0e47aedaa..3368a2d15d73b 100644
--- a/kernel/debug/kdb/kdb_bt.c
+++ b/kernel/debug/kdb/kdb_bt.c
@@ -74,7 +74,7 @@ static void kdb_show_stack(struct task_struct *p, void *addr)
  */

 static int
-kdb_bt1(struct task_struct *p, unsigned long mask, bool btaprompt)
+kdb_bt1(struct task_struct *p, const char *mask, bool btaprompt)
 {
 	char ch;

@@ -120,7 +120,7 @@ kdb_bt_cpu(unsigned long cpu)
 		return;
 	}

-	kdb_bt1(kdb_tsk, ~0UL, false);
+	kdb_bt1(kdb_tsk, "A", false);
 }

 int
@@ -138,8 +138,8 @@ kdb_bt(int argc, const char **argv)
 	if (strcmp(argv[0], "bta") == 0) {
 		struct task_struct *g, *p;
 		unsigned long cpu;
-		unsigned long mask = kdb_task_state_string(argc ? argv[1] :
-							   NULL);
+		const char *mask = argc ? argv[1] : kdbgetenv("PS");
+
 		if (argc == 0)
 			kdb_ps_suppressed();
 		/* Run the active tasks first */
@@ -167,7 +167,7 @@ kdb_bt(int argc, const char **argv)
 			return diag;
 		p = find_task_by_pid_ns(pid, &init_pid_ns);
 		if (p)
-			return kdb_bt1(p, ~0UL, false);
+			return kdb_bt1(p, "A", false);
 		kdb_printf("No process with pid == %ld found\n", pid);
 		return 0;
 	} else if (strcmp(argv[0], "btt") == 0) {
@@ -176,7 +176,7 @@ kdb_bt(int argc, const char **argv)
 		diag = kdbgetularg((char *)argv[1], &addr);
 		if (diag)
 			return diag;
-		return kdb_bt1((struct task_struct *)addr, ~0UL, false);
+		return kdb_bt1((struct task_struct *)addr, "A", false);
 	} else if (strcmp(argv[0], "btc") == 0) {
 		unsigned long cpu = ~0;
 		if (argc > 1)
@@ -212,7 +212,7 @@ kdb_bt(int argc, const char **argv)
 			kdb_show_stack(kdb_current_task, (void *)addr);
 			return 0;
 		} else {
-			return kdb_bt1(kdb_current_task, ~0UL, false);
+			return kdb_bt1(kdb_current_task, "A", false);
 		}
 	}

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index fa6deda894a17..b2aadc23f4ca1 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -2271,17 +2271,15 @@ static int kdb_cpu(int argc, const char **argv)
 void kdb_ps_suppressed(void)
 {
 	int idle = 0, daemon = 0;
-	unsigned long mask_I = kdb_task_state_string("I"),
-		      mask_M = kdb_task_state_string("M");
 	unsigned long cpu;
 	const struct task_struct *p, *g;
 	for_each_online_cpu(cpu) {
 		p = kdb_curr_task(cpu);
-		if (kdb_task_state(p, mask_I))
+		if (kdb_task_state(p, "I"))
 			++idle;
 	}
 	for_each_process_thread(g, p) {
-		if (kdb_task_state(p, mask_M))
+		if (kdb_task_state(p, "M"))
 			++daemon;
 	}
 	if (idle || daemon) {
@@ -2300,7 +2298,7 @@ void kdb_ps_suppressed(void)
 /*
  * kdb_ps - This function implements the 'ps' command which shows a
  *	list of the active processes.
- *		ps [DRSTCZEUIMA]   All processes, optionally filtered by state
+ *		ps [RSDTtXZPIMA]   All processes, optionally filtered by state
  */
 void kdb_ps1(const struct task_struct *p)
 {
@@ -2333,14 +2331,15 @@ void kdb_ps1(const struct task_struct *p)
 static int kdb_ps(int argc, const char **argv)
 {
 	struct task_struct *g, *p;
-	unsigned long mask, cpu;
+	const char *mask;
+	unsigned long cpu;

 	if (argc == 0)
 		kdb_ps_suppressed();
 	kdb_printf("%-*s      Pid   Parent [*] cpu State %-*s Command\n",
 		(int)(2*sizeof(void *))+2, "Task Addr",
 		(int)(2*sizeof(void *))+2, "Thread");
-	mask = kdb_task_state_string(argc ? argv[1] : NULL);
+	mask = argc ? argv[1] : kdbgetenv("PS");
 	/* Run the active tasks first */
 	for_each_online_cpu(cpu) {
 		if (KDB_FLAG(CMD_INTERRUPT))
@@ -2742,7 +2741,7 @@ static kdbtab_t maintab[] = {
 	},
 	{	.name = "bta",
 		.func = kdb_bt,
-		.usage = "[D|R|S|T|C|Z|E|U|I|M|A]",
+		.usage = "[R|S|D|T|t|X|Z|P|I|M|A]",
 		.help = "Backtrace all processes matching state flag",
 		.flags = KDB_ENABLE_INSPECT,
 	},
diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
index 629590084a0dc..0d2f9feea0a46 100644
--- a/kernel/debug/kdb/kdb_private.h
+++ b/kernel/debug/kdb/kdb_private.h
@@ -190,10 +190,8 @@ extern char kdb_grep_string[];
 extern int kdb_grep_leading;
 extern int kdb_grep_trailing;
 extern char *kdb_cmds[];
-extern unsigned long kdb_task_state_string(const char *);
 extern char kdb_task_state_char (const struct task_struct *);
-extern unsigned long kdb_task_state(const struct task_struct *p,
-				    unsigned long mask);
+extern bool kdb_task_state(const struct task_struct *p, const char *mask);
 extern void kdb_ps_suppressed(void);
 extern void kdb_ps1(const struct task_struct *p);
 extern void kdb_send_sig(struct task_struct *p, int sig);
diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
index 7507d9a8dc6ac..2070f2527337d 100644
--- a/kernel/debug/kdb/kdb_support.c
+++ b/kernel/debug/kdb/kdb_support.c
@@ -473,82 +473,7 @@ int kdb_putword(unsigned long addr, unsigned long word, size_t size)
 	return diag;
 }

-/*
- * kdb_task_state_string - Convert a string containing any of the
- *	letters DRSTCZEUIMA to a mask for the process state field and
- *	return the value.  If no argument is supplied, return the mask
- *	that corresponds to environment variable PS, DRSTCZEU by
- *	default.
- * Inputs:
- *	s	String to convert
- * Returns:
- *	Mask for process state.
- * Notes:
- *	The mask folds data from several sources into a single long value, so
- *	be careful not to overlap the bits.  TASK_* bits are in the LSB,
- *	special cases like UNRUNNABLE are in the MSB.  As of 2.6.10-rc1 there
- *	is no overlap between TASK_* and EXIT_* but that may not always be
- *	true, so EXIT_* bits are shifted left 16 bits before being stored in
- *	the mask.
- */

-/* unrunnable is < 0 */
-#define UNRUNNABLE	(1UL << (8*sizeof(unsigned long) - 1))
-#define RUNNING		(1UL << (8*sizeof(unsigned long) - 2))
-#define IDLE		(1UL << (8*sizeof(unsigned long) - 3))
-#define DAEMON		(1UL << (8*sizeof(unsigned long) - 4))
-
-unsigned long kdb_task_state_string(const char *s)
-{
-	long res = 0;
-	if (!s) {
-		s = kdbgetenv("PS");
-		if (!s)
-			s = "DRSTCZEU";	/* default value for ps */
-	}
-	while (*s) {
-		switch (*s) {
-		case 'D':
-			res |= TASK_UNINTERRUPTIBLE;
-			break;
-		case 'R':
-			res |= RUNNING;
-			break;
-		case 'S':
-			res |= TASK_INTERRUPTIBLE;
-			break;
-		case 'T':
-			res |= TASK_STOPPED;
-			break;
-		case 'C':
-			res |= TASK_TRACED;
-			break;
-		case 'Z':
-			res |= EXIT_ZOMBIE << 16;
-			break;
-		case 'E':
-			res |= EXIT_DEAD << 16;
-			break;
-		case 'U':
-			res |= UNRUNNABLE;
-			break;
-		case 'I':
-			res |= IDLE;
-			break;
-		case 'M':
-			res |= DAEMON;
-			break;
-		case 'A':
-			res = ~0UL;
-			break;
-		default:
-			  kdb_func_printf("unknown flag '%c' ignored\n", *s);
-			  break;
-		}
-		++s;
-	}
-	return res;
-}

 /*
  * kdb_task_state_char - Return the character that represents the task state.
@@ -559,7 +484,6 @@ unsigned long kdb_task_state_string(const char *s)
  */
 char kdb_task_state_char (const struct task_struct *p)
 {
-	unsigned int p_state;
 	unsigned long tmp;
 	char state;
 	int cpu;
@@ -568,16 +492,20 @@ char kdb_task_state_char (const struct task_struct *p)
 	    copy_from_kernel_nofault(&tmp, (char *)p, sizeof(unsigned long)))
 		return 'E';

-	cpu = kdb_process_cpu(p);
-	p_state = READ_ONCE(p->__state);
-	state = (p_state == 0) ? 'R' :
-		(p_state < 0) ? 'U' :
-		(p_state & TASK_UNINTERRUPTIBLE) ? 'D' :
-		(p_state & TASK_STOPPED) ? 'T' :
-		(p_state & TASK_TRACED) ? 'C' :
-		(p->exit_state & EXIT_ZOMBIE) ? 'Z' :
-		(p->exit_state & EXIT_DEAD) ? 'E' :
-		(p_state & TASK_INTERRUPTIBLE) ? 'S' : '?';
+	state = task_state_to_char((struct task_struct *) p);
+
+	/*
+	 * task_state_to_char() uses I(dle) differently to is_idle_task().
+	 * I(dle) tasks are (U)ninterruptible tasks that do not
+	 * contribute to the load average and have nothing to do with
+	 * code that runs on idle CPUs.
+	 *
+	 * For historic reasons we'd like to reserve I for idle CPUs in
+	 * kdb so we must reclassify (I)dle tasks.
+	 */
+	if (state == 'I')
+		state = 'U';
+
 	if (is_idle_task(p)) {
 		/* Idle task.  Is it really idle, apart from the kdb
 		 * interrupt? */
@@ -596,14 +524,24 @@ char kdb_task_state_char (const struct task_struct *p)
  *	given by the mask.
  * Inputs:
  *	p	struct task for the process
- *	mask	mask from kdb_task_state_string to select processes
+ *	mask	set of characters used to select processes; both NULL
+ *	        and the empty string mean adopt a default filter, which
+ *	        is to suppress sleeping system daemons and the idle tasks
  * Returns:
  *	True if the process matches at least one criteria defined by the mask.
  */
-unsigned long kdb_task_state(const struct task_struct *p, unsigned long mask)
+bool kdb_task_state(const struct task_struct *p, const char *mask)
 {
-	char state[] = { kdb_task_state_char(p), '\0' };
-	return (mask & kdb_task_state_string(state)) != 0;
+	char state = kdb_task_state_char(p);
+
+	if (!mask || mask[0] == '\0')
+		return state != 'I' && state != 'M';
+
+	/* A is a special case that matches all states */
+	if (strchr(mask, 'A'))
+		return true;
+
+	return strchr(mask, state);
 }

 /* Maintain a small stack of kdb_flags to allow recursion without disturbing

base-commit: 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f
--
2.31.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] kdb: Adopt scheduler's task clasification
  2021-09-16 15:42 [PATCH] kdb: Adopt scheduler's task clasification Daniel Thompson
@ 2021-09-16 16:28 ` Doug Anderson
  2021-09-17  8:18   ` Daniel Thompson
  2021-09-17  0:42 ` kernel test robot
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Doug Anderson @ 2021-09-16 16:28 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: Jason Wessel, Xiang wangx, jing yangyang, kgdb-bugreport, LKML,
	Patch Tracking

Hi,

On Thu, Sep 16, 2021 at 8:43 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> Currently kdb contains some open-coded routines to generate a summary
> character for each task. This code currently issues warnings, is
> almost certainly broken and won't make any sense to any kernel dev who
> has ever used /proc to examine tasks (D means uninterruptible?).
>
> Fix both the warning and the potential for confusion but adopting the
> scheduler's task clasification. Whilst doing this we also simplify the

s/clasification/classification/


> filtering by using mask strings directly (this means we don't have to
> guess all the characters the scheduler might give us).
>
> Unfortunately we can't quite adopt the scheudler classification it in

s/scheudler/scheduler/


> its entirity because, whilst we can tolerate some changes to the filter

s/entirity/entirety/


> characters, we need to keep I as a means to identify idle CPUs rather than
> system daemons that don't contribute to the load average! Naturally there
> is quite a large comment discussing this.

I'm a bit curious why we're OK with changing other characters but not
'I'. Even if the scheduler use of the character 'I' is a bit
confusing, it still seems like it might be nice to match it just to
avoid confusion. Couldn't we use lowercase 'i' for idle CPUs?
Alternatively beef up the commit message justifying why exactly we
need to keep 'I' as-is.


> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

Worth having a "Fixes" for the patch that introduced the warning?


> @@ -74,7 +74,7 @@ static void kdb_show_stack(struct task_struct *p, void *addr)
>   */
>
>  static int
> -kdb_bt1(struct task_struct *p, unsigned long mask, bool btaprompt)
> +kdb_bt1(struct task_struct *p, const char *mask, bool btaprompt)

In the comment above this function there is still a reference to
"DRSTCZEUIMA". Update that?


> @@ -2300,7 +2298,7 @@ void kdb_ps_suppressed(void)
>  /*
>   * kdb_ps - This function implements the 'ps' command which shows a
>   *     list of the active processes.
> - *             ps [DRSTCZEUIMA]   All processes, optionally filtered by state
> + *             ps [RSDTtXZPIMA]   All processes, optionally filtered by state

What about "U"? What about "E"?


> @@ -2742,7 +2741,7 @@ static kdbtab_t maintab[] = {
>         },
>         {       .name = "bta",
>                 .func = kdb_bt,
> -               .usage = "[D|R|S|T|C|Z|E|U|I|M|A]",
> +               .usage = "[R|S|D|T|t|X|Z|P|I|M|A]",

What about "U"? What about "E"?


> @@ -559,7 +484,6 @@ unsigned long kdb_task_state_string(const char *s)
>   */
>  char kdb_task_state_char (const struct task_struct *p)
>  {
> -       unsigned int p_state;
>         unsigned long tmp;
>         char state;
>         int cpu;
> @@ -568,16 +492,20 @@ char kdb_task_state_char (const struct task_struct *p)
>             copy_from_kernel_nofault(&tmp, (char *)p, sizeof(unsigned long)))
>                 return 'E';
>
> -       cpu = kdb_process_cpu(p);

Don't you still need this? You still have the `cpu` variable and you
still use it in the idle task case.


> -       p_state = READ_ONCE(p->__state);
> -       state = (p_state == 0) ? 'R' :
> -               (p_state < 0) ? 'U' :
> -               (p_state & TASK_UNINTERRUPTIBLE) ? 'D' :
> -               (p_state & TASK_STOPPED) ? 'T' :
> -               (p_state & TASK_TRACED) ? 'C' :
> -               (p->exit_state & EXIT_ZOMBIE) ? 'Z' :
> -               (p->exit_state & EXIT_DEAD) ? 'E' :
> -               (p_state & TASK_INTERRUPTIBLE) ? 'S' : '?';
> +       state = task_state_to_char((struct task_struct *) p);

Casting away constness is fine for now and likely makes this easier to
land, but maybe you can send a patch up to change the API to have
"const" in it?


-Doug

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] kdb: Adopt scheduler's task clasification
  2021-09-16 15:42 [PATCH] kdb: Adopt scheduler's task clasification Daniel Thompson
  2021-09-16 16:28 ` Doug Anderson
@ 2021-09-17  0:42 ` kernel test robot
  2021-09-17  5:19 ` kernel test robot
  2021-10-29 17:19 ` [PATCH v2] " Daniel Thompson
  3 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2021-09-17  0:42 UTC (permalink / raw)
  To: Daniel Thompson, Jason Wessel, Douglas Anderson
  Cc: llvm, kbuild-all, Xiang wangx, jing yangyang, kgdb-bugreport,
	linux-kernel, patches

[-- Attachment #1: Type: text/plain, Size: 5445 bytes --]

Hi Daniel,

I love your patch! Yet something to improve:

[auto build test ERROR on 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f]

url:    https://github.com/0day-ci/linux/commits/Daniel-Thompson/kdb-Adopt-scheduler-s-task-clasification/20210917-004549
base:   6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f
config: hexagon-buildonly-randconfig-r005-20210916 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c8b3d7d6d6de37af68b2f379d0e37304f78e115f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/d315f14b7a044983f76f08221be33c2900c58e37
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Thompson/kdb-Adopt-scheduler-s-task-clasification/20210917-004549
        git checkout d315f14b7a044983f76f08221be33c2900c58e37
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=hexagon 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> kernel/debug/kdb/kdb_support.c:512:41: error: variable 'cpu' is uninitialized when used here [-Werror,-Wuninitialized]
                   if (!kdb_task_has_cpu(p) || kgdb_info[cpu].irq_depth == 1) {
                                                         ^~~
   kernel/debug/kdb/kdb_support.c:489:9: note: initialize the variable 'cpu' to silence this warning
           int cpu;
                  ^
                   = 0
   1 error generated.


vim +/cpu +512 kernel/debug/kdb/kdb_support.c

5d5314d6795f3c1 Jason Wessel      2010-05-20  475  
5d5314d6795f3c1 Jason Wessel      2010-05-20  476  
5d5314d6795f3c1 Jason Wessel      2010-05-20  477  
5d5314d6795f3c1 Jason Wessel      2010-05-20  478  /*
5d5314d6795f3c1 Jason Wessel      2010-05-20  479   * kdb_task_state_char - Return the character that represents the task state.
5d5314d6795f3c1 Jason Wessel      2010-05-20  480   * Inputs:
5d5314d6795f3c1 Jason Wessel      2010-05-20  481   *	p	struct task for the process
5d5314d6795f3c1 Jason Wessel      2010-05-20  482   * Returns:
5d5314d6795f3c1 Jason Wessel      2010-05-20  483   *	One character to represent the task state.
5d5314d6795f3c1 Jason Wessel      2010-05-20  484   */
5d5314d6795f3c1 Jason Wessel      2010-05-20  485  char kdb_task_state_char (const struct task_struct *p)
5d5314d6795f3c1 Jason Wessel      2010-05-20  486  {
5d5314d6795f3c1 Jason Wessel      2010-05-20  487  	unsigned long tmp;
2f064a59a11ff9b Peter Zijlstra    2021-06-11  488  	char state;
2f064a59a11ff9b Peter Zijlstra    2021-06-11  489  	int cpu;
5d5314d6795f3c1 Jason Wessel      2010-05-20  490  
fe557319aa06c23 Christoph Hellwig 2020-06-17  491  	if (!p ||
fe557319aa06c23 Christoph Hellwig 2020-06-17  492  	    copy_from_kernel_nofault(&tmp, (char *)p, sizeof(unsigned long)))
5d5314d6795f3c1 Jason Wessel      2010-05-20  493  		return 'E';
5d5314d6795f3c1 Jason Wessel      2010-05-20  494  
d315f14b7a04498 Daniel Thompson   2021-09-16  495  	state = task_state_to_char((struct task_struct *) p);
d315f14b7a04498 Daniel Thompson   2021-09-16  496  
d315f14b7a04498 Daniel Thompson   2021-09-16  497  	/*
d315f14b7a04498 Daniel Thompson   2021-09-16  498  	 * task_state_to_char() uses I(dle) differently to is_idle_task().
d315f14b7a04498 Daniel Thompson   2021-09-16  499  	 * I(dle) tasks are (U)ninterruptible tasks that do not
d315f14b7a04498 Daniel Thompson   2021-09-16  500  	 * contribute to the load average and have nothing to do with
d315f14b7a04498 Daniel Thompson   2021-09-16  501  	 * code that runs on idle CPUs.
d315f14b7a04498 Daniel Thompson   2021-09-16  502  	 *
d315f14b7a04498 Daniel Thompson   2021-09-16  503  	 * For historic reasons we'd like to reserve I for idle CPUs in
d315f14b7a04498 Daniel Thompson   2021-09-16  504  	 * kdb so we must reclassify (I)dle tasks.
d315f14b7a04498 Daniel Thompson   2021-09-16  505  	 */
d315f14b7a04498 Daniel Thompson   2021-09-16  506  	if (state == 'I')
d315f14b7a04498 Daniel Thompson   2021-09-16  507  		state = 'U';
d315f14b7a04498 Daniel Thompson   2021-09-16  508  
7fc20c5cbdd184f Paul E. McKenney  2011-11-10  509  	if (is_idle_task(p)) {
5d5314d6795f3c1 Jason Wessel      2010-05-20  510  		/* Idle task.  Is it really idle, apart from the kdb
5d5314d6795f3c1 Jason Wessel      2010-05-20  511  		 * interrupt? */
5d5314d6795f3c1 Jason Wessel      2010-05-20 @512  		if (!kdb_task_has_cpu(p) || kgdb_info[cpu].irq_depth == 1) {
5d5314d6795f3c1 Jason Wessel      2010-05-20  513  			if (cpu != kdb_initial_cpu)
5d5314d6795f3c1 Jason Wessel      2010-05-20  514  				state = 'I';	/* idle task */
5d5314d6795f3c1 Jason Wessel      2010-05-20  515  		}
5d5314d6795f3c1 Jason Wessel      2010-05-20  516  	} else if (!p->mm && state == 'S') {
5d5314d6795f3c1 Jason Wessel      2010-05-20  517  		state = 'M';	/* sleeping system daemon */
5d5314d6795f3c1 Jason Wessel      2010-05-20  518  	}
5d5314d6795f3c1 Jason Wessel      2010-05-20  519  	return state;
5d5314d6795f3c1 Jason Wessel      2010-05-20  520  }
5d5314d6795f3c1 Jason Wessel      2010-05-20  521  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24168 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] kdb: Adopt scheduler's task clasification
  2021-09-16 15:42 [PATCH] kdb: Adopt scheduler's task clasification Daniel Thompson
  2021-09-16 16:28 ` Doug Anderson
  2021-09-17  0:42 ` kernel test robot
@ 2021-09-17  5:19 ` kernel test robot
  2021-10-29 17:19 ` [PATCH v2] " Daniel Thompson
  3 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2021-09-17  5:19 UTC (permalink / raw)
  To: Daniel Thompson, Jason Wessel, Douglas Anderson
  Cc: llvm, kbuild-all, Xiang wangx, jing yangyang, kgdb-bugreport,
	linux-kernel, patches

[-- Attachment #1: Type: text/plain, Size: 5867 bytes --]

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f]

url:    https://github.com/0day-ci/linux/commits/Daniel-Thompson/kdb-Adopt-scheduler-s-task-clasification/20210917-004549
base:   6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f
config: hexagon-randconfig-r001-20210916 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c8b3d7d6d6de37af68b2f379d0e37304f78e115f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/d315f14b7a044983f76f08221be33c2900c58e37
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Thompson/kdb-Adopt-scheduler-s-task-clasification/20210917-004549
        git checkout d315f14b7a044983f76f08221be33c2900c58e37
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=hexagon 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> kernel/debug/kdb/kdb_support.c:512:41: warning: variable 'cpu' is uninitialized when used here [-Wuninitialized]
                   if (!kdb_task_has_cpu(p) || kgdb_info[cpu].irq_depth == 1) {
                                                         ^~~
   include/linux/compiler.h:56:47: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                                                 ^~~~
   include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                      ^~~~
   kernel/debug/kdb/kdb_support.c:489:9: note: initialize the variable 'cpu' to silence this warning
           int cpu;
                  ^
                   = 0
   1 warning generated.


vim +/cpu +512 kernel/debug/kdb/kdb_support.c

5d5314d6795f3c1 Jason Wessel      2010-05-20  475  
5d5314d6795f3c1 Jason Wessel      2010-05-20  476  
5d5314d6795f3c1 Jason Wessel      2010-05-20  477  
5d5314d6795f3c1 Jason Wessel      2010-05-20  478  /*
5d5314d6795f3c1 Jason Wessel      2010-05-20  479   * kdb_task_state_char - Return the character that represents the task state.
5d5314d6795f3c1 Jason Wessel      2010-05-20  480   * Inputs:
5d5314d6795f3c1 Jason Wessel      2010-05-20  481   *	p	struct task for the process
5d5314d6795f3c1 Jason Wessel      2010-05-20  482   * Returns:
5d5314d6795f3c1 Jason Wessel      2010-05-20  483   *	One character to represent the task state.
5d5314d6795f3c1 Jason Wessel      2010-05-20  484   */
5d5314d6795f3c1 Jason Wessel      2010-05-20  485  char kdb_task_state_char (const struct task_struct *p)
5d5314d6795f3c1 Jason Wessel      2010-05-20  486  {
5d5314d6795f3c1 Jason Wessel      2010-05-20  487  	unsigned long tmp;
2f064a59a11ff9b Peter Zijlstra    2021-06-11  488  	char state;
2f064a59a11ff9b Peter Zijlstra    2021-06-11  489  	int cpu;
5d5314d6795f3c1 Jason Wessel      2010-05-20  490  
fe557319aa06c23 Christoph Hellwig 2020-06-17  491  	if (!p ||
fe557319aa06c23 Christoph Hellwig 2020-06-17  492  	    copy_from_kernel_nofault(&tmp, (char *)p, sizeof(unsigned long)))
5d5314d6795f3c1 Jason Wessel      2010-05-20  493  		return 'E';
5d5314d6795f3c1 Jason Wessel      2010-05-20  494  
d315f14b7a04498 Daniel Thompson   2021-09-16  495  	state = task_state_to_char((struct task_struct *) p);
d315f14b7a04498 Daniel Thompson   2021-09-16  496  
d315f14b7a04498 Daniel Thompson   2021-09-16  497  	/*
d315f14b7a04498 Daniel Thompson   2021-09-16  498  	 * task_state_to_char() uses I(dle) differently to is_idle_task().
d315f14b7a04498 Daniel Thompson   2021-09-16  499  	 * I(dle) tasks are (U)ninterruptible tasks that do not
d315f14b7a04498 Daniel Thompson   2021-09-16  500  	 * contribute to the load average and have nothing to do with
d315f14b7a04498 Daniel Thompson   2021-09-16  501  	 * code that runs on idle CPUs.
d315f14b7a04498 Daniel Thompson   2021-09-16  502  	 *
d315f14b7a04498 Daniel Thompson   2021-09-16  503  	 * For historic reasons we'd like to reserve I for idle CPUs in
d315f14b7a04498 Daniel Thompson   2021-09-16  504  	 * kdb so we must reclassify (I)dle tasks.
d315f14b7a04498 Daniel Thompson   2021-09-16  505  	 */
d315f14b7a04498 Daniel Thompson   2021-09-16  506  	if (state == 'I')
d315f14b7a04498 Daniel Thompson   2021-09-16  507  		state = 'U';
d315f14b7a04498 Daniel Thompson   2021-09-16  508  
7fc20c5cbdd184f Paul E. McKenney  2011-11-10  509  	if (is_idle_task(p)) {
5d5314d6795f3c1 Jason Wessel      2010-05-20  510  		/* Idle task.  Is it really idle, apart from the kdb
5d5314d6795f3c1 Jason Wessel      2010-05-20  511  		 * interrupt? */
5d5314d6795f3c1 Jason Wessel      2010-05-20 @512  		if (!kdb_task_has_cpu(p) || kgdb_info[cpu].irq_depth == 1) {
5d5314d6795f3c1 Jason Wessel      2010-05-20  513  			if (cpu != kdb_initial_cpu)
5d5314d6795f3c1 Jason Wessel      2010-05-20  514  				state = 'I';	/* idle task */
5d5314d6795f3c1 Jason Wessel      2010-05-20  515  		}
5d5314d6795f3c1 Jason Wessel      2010-05-20  516  	} else if (!p->mm && state == 'S') {
5d5314d6795f3c1 Jason Wessel      2010-05-20  517  		state = 'M';	/* sleeping system daemon */
5d5314d6795f3c1 Jason Wessel      2010-05-20  518  	}
5d5314d6795f3c1 Jason Wessel      2010-05-20  519  	return state;
5d5314d6795f3c1 Jason Wessel      2010-05-20  520  }
5d5314d6795f3c1 Jason Wessel      2010-05-20  521  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27402 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] kdb: Adopt scheduler's task clasification
  2021-09-16 16:28 ` Doug Anderson
@ 2021-09-17  8:18   ` Daniel Thompson
  2021-09-17 14:36     ` Doug Anderson
  0 siblings, 1 reply; 8+ messages in thread
From: Daniel Thompson @ 2021-09-17  8:18 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Jason Wessel, Xiang wangx, jing yangyang, kgdb-bugreport, LKML,
	Patch Tracking

On Thu, Sep 16, 2021 at 09:28:22AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, Sep 16, 2021 at 8:43 AM Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> >
> > Currently kdb contains some open-coded routines to generate a summary
> > character for each task. This code currently issues warnings, is
> > almost certainly broken and won't make any sense to any kernel dev who
> > has ever used /proc to examine tasks (D means uninterruptible?).
> >
> > Fix both the warning and the potential for confusion but adopting the
> > scheduler's task clasification. Whilst doing this we also simplify the
> 
> s/clasification/classification/
> [...]
> s/scheudler/scheduler/
> [...]
> s/entirity/entirety/

Will do. Thanks.


> > characters, we need to keep I as a means to identify idle CPUs rather than
> > system daemons that don't contribute to the load average! Naturally there
> > is quite a large comment discussing this.
> 
> I'm a bit curious why we're OK with changing other characters but not
> 'I'. Even if the scheduler use of the character 'I' is a bit
> confusing, it still seems like it might be nice to match it just to
> avoid confusion. Couldn't we use lowercase 'i' for idle CPUs?
> Alternatively beef up the commit message justifying why exactly we
> need to keep 'I' as-is.

I've been though a couple of iterations and nothing felt 100% right
(to the extent I should probably have marked the patch RFC).

There is another thing I left for a later patch... and that is that
the logic to hide sleeping kernel threads (called system daemons
in the comments) is also rather broken at present since, in the modern
kernel, the majority of sleeping system deamons today tend to be doing
uninterruptible sleeps (and many are marked no load and are reported
as idle). That means that the S -> M translation needs to change since
the way it hides processes is too unpredictable. I think it needs to
become an S -> s, D -> d and, if we keep I, I -> i.

Or, putting it another way, once we fix the S -> M translations, then
finding a character that implies idle and does not collide with the
existing set is very hard.

Perhaps '-' might be a good way to mark idle tasks? It's different that
the not-really-a-task nature of idle tasks might be obvious.

Let me take a second look!


> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> 
> Worth having a "Fixes" for the patch that introduced the warning?

I'm never sure how useful Fixes: that point to the dawn of time
actually are.


> > @@ -74,7 +74,7 @@ static void kdb_show_stack(struct task_struct *p, void *addr)
> >   */
> >
> >  static int
> > -kdb_bt1(struct task_struct *p, unsigned long mask, bool btaprompt)
> > +kdb_bt1(struct task_struct *p, const char *mask, bool btaprompt)
> 
> In the comment above this function there is still a reference to
> "DRSTCZEUIMA". Update that?

We spotted. I'm inclined to change this and the one for ps to
<filter> and not attempt to maintain a list of valid characters.


> > @@ -2300,7 +2298,7 @@ void kdb_ps_suppressed(void)
> >  /*
> >   * kdb_ps - This function implements the 'ps' command which shows a
> >   *     list of the active processes.
> > - *             ps [DRSTCZEUIMA]   All processes, optionally filtered by state
> > + *             ps [RSDTtXZPIMA]   All processes, optionally filtered by state
> 
> What about "U"? What about "E"?

As above... keeping these comments maintained seems a little pointless.
I'll switch this to filter.
> 
> 
> > @@ -2742,7 +2741,7 @@ static kdbtab_t maintab[] = {
> >         },
> >         {       .name = "bta",
> >                 .func = kdb_bt,
> > -               .usage = "[D|R|S|T|C|Z|E|U|I|M|A]",
> > +               .usage = "[R|S|D|T|t|X|Z|P|I|M|A]",
> 
> What about "U"? What about "E"?

I might even consider <filter> here (and a few extra hints). The output
of ps (or ps A) is a much more useful way to figure out the interesting
tasks to filter.


> > @@ -559,7 +484,6 @@ unsigned long kdb_task_state_string(const char *s)
> >   */
> >  char kdb_task_state_char (const struct task_struct *p)
> >  {
> > -       unsigned int p_state;
> >         unsigned long tmp;
> >         char state;
> >         int cpu;
> > @@ -568,16 +492,20 @@ char kdb_task_state_char (const struct task_struct *p)
> >             copy_from_kernel_nofault(&tmp, (char *)p, sizeof(unsigned long)))
> >                 return 'E';
> >
> > -       cpu = kdb_process_cpu(p);
> 
> Don't you still need this? You still have the `cpu` variable and you
> still use it in the idle task case.

Not sure what happened here. I have to assume fat fingers post testing
since I tested the code paths to recognise idle threads before posting.


> > -       p_state = READ_ONCE(p->__state);
> > -       state = (p_state == 0) ? 'R' :
> > -               (p_state < 0) ? 'U' :
> > -               (p_state & TASK_UNINTERRUPTIBLE) ? 'D' :
> > -               (p_state & TASK_STOPPED) ? 'T' :
> > -               (p_state & TASK_TRACED) ? 'C' :
> > -               (p->exit_state & EXIT_ZOMBIE) ? 'Z' :
> > -               (p->exit_state & EXIT_DEAD) ? 'E' :
> > -               (p_state & TASK_INTERRUPTIBLE) ? 'S' : '?';
> > +       state = task_state_to_char((struct task_struct *) p);
> 
> Casting away constness is fine for now and likely makes this easier to
> land, but maybe you can send a patch up to change the API to have
> "const" in it?

I already have the patch written but I'd like to keep it decoupled from
the this one due to the warning fix aspect (I'll note in header).


Daniel.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] kdb: Adopt scheduler's task clasification
  2021-09-17  8:18   ` Daniel Thompson
@ 2021-09-17 14:36     ` Doug Anderson
  0 siblings, 0 replies; 8+ messages in thread
From: Doug Anderson @ 2021-09-17 14:36 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: Jason Wessel, Xiang wangx, jing yangyang, kgdb-bugreport, LKML,
	Patch Tracking

Hi,

On Fri, Sep 17, 2021 at 1:18 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> > Worth having a "Fixes" for the patch that introduced the warning?
>
> I'm never sure how useful Fixes: that point to the dawn of time
> actually are.

I was just thinking of:

Fixes: 2f064a59a11f ("sched: Change task_struct::state")

I know that the logic was still wrong before that patch, but before
that patch at least there was no Warning reported.

-Doug

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] kdb: Adopt scheduler's task clasification
  2021-09-16 15:42 [PATCH] kdb: Adopt scheduler's task clasification Daniel Thompson
                   ` (2 preceding siblings ...)
  2021-09-17  5:19 ` kernel test robot
@ 2021-10-29 17:19 ` Daniel Thompson
  2021-10-31  3:49   ` kernel test robot
  3 siblings, 1 reply; 8+ messages in thread
From: Daniel Thompson @ 2021-10-29 17:19 UTC (permalink / raw)
  To: Jason Wessel, Daniel Thompson, Douglas Anderson
  Cc: Xiang wangx, jing yangyang, kgdb-bugreport, linux-kernel, patches

Currently kdb contains some open-coded routines to generate a summary
character for each task. This code currently issues warnings, is
almost certainly broken and won't make sense to any kernel dev who
has ever used /proc to examine task states.

Fix both the warning and the potential for confusion by adopting the
scheduler's task classification. Whilst doing this we also simplify the
filtering by using mask strings directly (which means we don't have to
guess all the characters the scheduler might give us).

Unfortunately we can't quite match the scheduler classification completely.
We add four extra states: - for idle loops and i, m and s sleeping system
daemons (which means kthreads in one of the I, M and S states). These
extra states are used to manage the filters for tools to make the output
of ps and bta less noisy.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---

Notes:
    Sorry it's taken so long to respin this patch.
    
    v2:
    - Fix the typos in the description (Doug)
    - Stop trying to bend to world so I can keep 'I' exactly as
      it was before. Instead we now replace 'I' with '-' and
      fully adopt the scheduler description of tasks. kdb
      it an interactive tool, not ABI so this is OK. (Doug)
    - Don't try to enumerate all possible letters in the
      comments and help. You can learn what to filter from
      the output of ps anyway, (Doug)
    - Fix the sleeping system daemon stuff.

 kernel/debug/kdb/kdb_bt.c      |  14 ++--
 kernel/debug/kdb/kdb_main.c    |  31 ++++-----
 kernel/debug/kdb/kdb_private.h |   4 +-
 kernel/debug/kdb/kdb_support.c | 117 +++++++--------------------------
 4 files changed, 48 insertions(+), 118 deletions(-)

diff --git a/kernel/debug/kdb/kdb_bt.c b/kernel/debug/kdb/kdb_bt.c
index 1f9f0e47aeda..3368a2d15d73 100644
--- a/kernel/debug/kdb/kdb_bt.c
+++ b/kernel/debug/kdb/kdb_bt.c
@@ -74,7 +74,7 @@ static void kdb_show_stack(struct task_struct *p, void *addr)
  */

 static int
-kdb_bt1(struct task_struct *p, unsigned long mask, bool btaprompt)
+kdb_bt1(struct task_struct *p, const char *mask, bool btaprompt)
 {
 	char ch;

@@ -120,7 +120,7 @@ kdb_bt_cpu(unsigned long cpu)
 		return;
 	}

-	kdb_bt1(kdb_tsk, ~0UL, false);
+	kdb_bt1(kdb_tsk, "A", false);
 }

 int
@@ -138,8 +138,8 @@ kdb_bt(int argc, const char **argv)
 	if (strcmp(argv[0], "bta") == 0) {
 		struct task_struct *g, *p;
 		unsigned long cpu;
-		unsigned long mask = kdb_task_state_string(argc ? argv[1] :
-							   NULL);
+		const char *mask = argc ? argv[1] : kdbgetenv("PS");
+
 		if (argc == 0)
 			kdb_ps_suppressed();
 		/* Run the active tasks first */
@@ -167,7 +167,7 @@ kdb_bt(int argc, const char **argv)
 			return diag;
 		p = find_task_by_pid_ns(pid, &init_pid_ns);
 		if (p)
-			return kdb_bt1(p, ~0UL, false);
+			return kdb_bt1(p, "A", false);
 		kdb_printf("No process with pid == %ld found\n", pid);
 		return 0;
 	} else if (strcmp(argv[0], "btt") == 0) {
@@ -176,7 +176,7 @@ kdb_bt(int argc, const char **argv)
 		diag = kdbgetularg((char *)argv[1], &addr);
 		if (diag)
 			return diag;
-		return kdb_bt1((struct task_struct *)addr, ~0UL, false);
+		return kdb_bt1((struct task_struct *)addr, "A", false);
 	} else if (strcmp(argv[0], "btc") == 0) {
 		unsigned long cpu = ~0;
 		if (argc > 1)
@@ -212,7 +212,7 @@ kdb_bt(int argc, const char **argv)
 			kdb_show_stack(kdb_current_task, (void *)addr);
 			return 0;
 		} else {
-			return kdb_bt1(kdb_current_task, ~0UL, false);
+			return kdb_bt1(kdb_current_task, "A", false);
 		}
 	}

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index fa6deda894a1..2c1abb86a06b 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -2203,8 +2203,8 @@ static void kdb_cpu_status(void)
 			state = 'D';	/* cpu is online but unresponsive */
 		} else {
 			state = ' ';	/* cpu is responding to kdb */
-			if (kdb_task_state_char(KDB_TSK(i)) == 'I')
-				state = 'I';	/* idle task */
+			if (kdb_task_state_char(KDB_TSK(i)) == '-')
+				state = '-';	/* idle task */
 		}
 		if (state != prev_state) {
 			if (prev_state != '?') {
@@ -2271,17 +2271,15 @@ static int kdb_cpu(int argc, const char **argv)
 void kdb_ps_suppressed(void)
 {
 	int idle = 0, daemon = 0;
-	unsigned long mask_I = kdb_task_state_string("I"),
-		      mask_M = kdb_task_state_string("M");
 	unsigned long cpu;
 	const struct task_struct *p, *g;
 	for_each_online_cpu(cpu) {
 		p = kdb_curr_task(cpu);
-		if (kdb_task_state(p, mask_I))
+		if (kdb_task_state(p, "-"))
 			++idle;
 	}
 	for_each_process_thread(g, p) {
-		if (kdb_task_state(p, mask_M))
+		if (kdb_task_state(p, "ims"))
 			++daemon;
 	}
 	if (idle || daemon) {
@@ -2297,11 +2295,6 @@ void kdb_ps_suppressed(void)
 	}
 }

-/*
- * kdb_ps - This function implements the 'ps' command which shows a
- *	list of the active processes.
- *		ps [DRSTCZEUIMA]   All processes, optionally filtered by state
- */
 void kdb_ps1(const struct task_struct *p)
 {
 	int cpu;
@@ -2330,17 +2323,25 @@ void kdb_ps1(const struct task_struct *p)
 	}
 }

+/*
+ * kdb_ps - This function implements the 'ps' command which shows a
+ *	    list of the active processes.
+ *
+ * ps [<state_chars>]   Show processes, optionally selecting only those whose
+ *                      state character is found in <state_chars>.
+ */
 static int kdb_ps(int argc, const char **argv)
 {
 	struct task_struct *g, *p;
-	unsigned long mask, cpu;
+	const char *mask;
+	unsigned long cpu;

 	if (argc == 0)
 		kdb_ps_suppressed();
 	kdb_printf("%-*s      Pid   Parent [*] cpu State %-*s Command\n",
 		(int)(2*sizeof(void *))+2, "Task Addr",
 		(int)(2*sizeof(void *))+2, "Thread");
-	mask = kdb_task_state_string(argc ? argv[1] : NULL);
+	mask = argc ? argv[1] : kdbgetenv("PS");
 	/* Run the active tasks first */
 	for_each_online_cpu(cpu) {
 		if (KDB_FLAG(CMD_INTERRUPT))
@@ -2742,8 +2743,8 @@ static kdbtab_t maintab[] = {
 	},
 	{	.name = "bta",
 		.func = kdb_bt,
-		.usage = "[D|R|S|T|C|Z|E|U|I|M|A]",
-		.help = "Backtrace all processes matching state flag",
+		.usage = "[<state_chars>|A]",
+		.help = "Backtrace all processes matching whose state matches",
 		.flags = KDB_ENABLE_INSPECT,
 	},
 	{	.name = "btc",
diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
index 629590084a0d..0d2f9feea0a4 100644
--- a/kernel/debug/kdb/kdb_private.h
+++ b/kernel/debug/kdb/kdb_private.h
@@ -190,10 +190,8 @@ extern char kdb_grep_string[];
 extern int kdb_grep_leading;
 extern int kdb_grep_trailing;
 extern char *kdb_cmds[];
-extern unsigned long kdb_task_state_string(const char *);
 extern char kdb_task_state_char (const struct task_struct *);
-extern unsigned long kdb_task_state(const struct task_struct *p,
-				    unsigned long mask);
+extern bool kdb_task_state(const struct task_struct *p, const char *mask);
 extern void kdb_ps_suppressed(void);
 extern void kdb_ps1(const struct task_struct *p);
 extern void kdb_send_sig(struct task_struct *p, int sig);
diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
index 7507d9a8dc6a..19f5c893580b 100644
--- a/kernel/debug/kdb/kdb_support.c
+++ b/kernel/debug/kdb/kdb_support.c
@@ -24,6 +24,7 @@
 #include <linux/uaccess.h>
 #include <linux/kdb.h>
 #include <linux/slab.h>
+#include <linux/ctype.h>
 #include "kdb_private.h"

 /*
@@ -473,82 +474,7 @@ int kdb_putword(unsigned long addr, unsigned long word, size_t size)
 	return diag;
 }

-/*
- * kdb_task_state_string - Convert a string containing any of the
- *	letters DRSTCZEUIMA to a mask for the process state field and
- *	return the value.  If no argument is supplied, return the mask
- *	that corresponds to environment variable PS, DRSTCZEU by
- *	default.
- * Inputs:
- *	s	String to convert
- * Returns:
- *	Mask for process state.
- * Notes:
- *	The mask folds data from several sources into a single long value, so
- *	be careful not to overlap the bits.  TASK_* bits are in the LSB,
- *	special cases like UNRUNNABLE are in the MSB.  As of 2.6.10-rc1 there
- *	is no overlap between TASK_* and EXIT_* but that may not always be
- *	true, so EXIT_* bits are shifted left 16 bits before being stored in
- *	the mask.
- */
-
-/* unrunnable is < 0 */
-#define UNRUNNABLE	(1UL << (8*sizeof(unsigned long) - 1))
-#define RUNNING		(1UL << (8*sizeof(unsigned long) - 2))
-#define IDLE		(1UL << (8*sizeof(unsigned long) - 3))
-#define DAEMON		(1UL << (8*sizeof(unsigned long) - 4))

-unsigned long kdb_task_state_string(const char *s)
-{
-	long res = 0;
-	if (!s) {
-		s = kdbgetenv("PS");
-		if (!s)
-			s = "DRSTCZEU";	/* default value for ps */
-	}
-	while (*s) {
-		switch (*s) {
-		case 'D':
-			res |= TASK_UNINTERRUPTIBLE;
-			break;
-		case 'R':
-			res |= RUNNING;
-			break;
-		case 'S':
-			res |= TASK_INTERRUPTIBLE;
-			break;
-		case 'T':
-			res |= TASK_STOPPED;
-			break;
-		case 'C':
-			res |= TASK_TRACED;
-			break;
-		case 'Z':
-			res |= EXIT_ZOMBIE << 16;
-			break;
-		case 'E':
-			res |= EXIT_DEAD << 16;
-			break;
-		case 'U':
-			res |= UNRUNNABLE;
-			break;
-		case 'I':
-			res |= IDLE;
-			break;
-		case 'M':
-			res |= DAEMON;
-			break;
-		case 'A':
-			res = ~0UL;
-			break;
-		default:
-			  kdb_func_printf("unknown flag '%c' ignored\n", *s);
-			  break;
-		}
-		++s;
-	}
-	return res;
-}

 /*
  * kdb_task_state_char - Return the character that represents the task state.
@@ -559,7 +485,6 @@ unsigned long kdb_task_state_string(const char *s)
  */
 char kdb_task_state_char (const struct task_struct *p)
 {
-	unsigned int p_state;
 	unsigned long tmp;
 	char state;
 	int cpu;
@@ -568,25 +493,17 @@ char kdb_task_state_char (const struct task_struct *p)
 	    copy_from_kernel_nofault(&tmp, (char *)p, sizeof(unsigned long)))
 		return 'E';

-	cpu = kdb_process_cpu(p);
-	p_state = READ_ONCE(p->__state);
-	state = (p_state == 0) ? 'R' :
-		(p_state < 0) ? 'U' :
-		(p_state & TASK_UNINTERRUPTIBLE) ? 'D' :
-		(p_state & TASK_STOPPED) ? 'T' :
-		(p_state & TASK_TRACED) ? 'C' :
-		(p->exit_state & EXIT_ZOMBIE) ? 'Z' :
-		(p->exit_state & EXIT_DEAD) ? 'E' :
-		(p_state & TASK_INTERRUPTIBLE) ? 'S' : '?';
+	state = task_state_to_char((struct task_struct *) p);
+
 	if (is_idle_task(p)) {
 		/* Idle task.  Is it really idle, apart from the kdb
 		 * interrupt? */
 		if (!kdb_task_has_cpu(p) || kgdb_info[cpu].irq_depth == 1) {
 			if (cpu != kdb_initial_cpu)
-				state = 'I';	/* idle task */
+				state = '-';	/* idle task */
 		}
-	} else if (!p->mm && state == 'S') {
-		state = 'M';	/* sleeping system daemon */
+	} else if (!p->mm && strchr("IMS", state)) {
+		state = tolower(state);		/* sleeping system daemon */
 	}
 	return state;
 }
@@ -596,14 +513,28 @@ char kdb_task_state_char (const struct task_struct *p)
  *	given by the mask.
  * Inputs:
  *	p	struct task for the process
- *	mask	mask from kdb_task_state_string to select processes
+ *	mask	set of characters used to select processes; both NULL
+ *	        and the empty string mean adopt a default filter, which
+ *	        is to suppress sleeping system daemons and the idle tasks
  * Returns:
  *	True if the process matches at least one criteria defined by the mask.
  */
-unsigned long kdb_task_state(const struct task_struct *p, unsigned long mask)
+bool kdb_task_state(const struct task_struct *p, const char *mask)
 {
-	char state[] = { kdb_task_state_char(p), '\0' };
-	return (mask & kdb_task_state_string(state)) != 0;
+	char state = kdb_task_state_char(p);
+
+	/* If there is no mask, then we will filter code that runs when the
+	 * scheduler is idling and any system daemons that are currently
+	 * sleeping.
+	 */
+	if (!mask || mask[0] == '\0')
+		return !strchr("-ims", state);
+
+	/* A is a special case that matches all states */
+	if (strchr(mask, 'A'))
+		return true;
+
+	return strchr(mask, state);
 }

 /* Maintain a small stack of kdb_flags to allow recursion without disturbing

base-commit: 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f
--
2.31.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] kdb: Adopt scheduler's task clasification
  2021-10-29 17:19 ` [PATCH v2] " Daniel Thompson
@ 2021-10-31  3:49   ` kernel test robot
  0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2021-10-31  3:49 UTC (permalink / raw)
  To: Daniel Thompson, Jason Wessel, Douglas Anderson
  Cc: llvm, kbuild-all, Xiang wangx, jing yangyang, kgdb-bugreport,
	linux-kernel, patches

[-- Attachment #1: Type: text/plain, Size: 8253 bytes --]

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f]

url:    https://github.com/0day-ci/linux/commits/Daniel-Thompson/kdb-Adopt-scheduler-s-task-clasification/20211030-012127
base:   6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f
config: riscv-randconfig-r042-20211031 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 82ed106567063ea269c6d5669278b733e173a42f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/9f7ea8ffacb27d8b7fe10190fa91e2803c985611
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Thompson/kdb-Adopt-scheduler-s-task-clasification/20211030-012127
        git checkout 9f7ea8ffacb27d8b7fe10190fa91e2803c985611
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from kernel/debug/kdb/kdb_support.c:21:
   In file included from include/linux/highmem.h:10:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:36:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from kernel/debug/kdb/kdb_support.c:21:
   In file included from include/linux/highmem.h:10:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:34:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from kernel/debug/kdb/kdb_support.c:21:
   In file included from include/linux/highmem.h:10:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:1024:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
                                                     ~~~~~~~~~~ ^
>> kernel/debug/kdb/kdb_support.c:501:41: warning: variable 'cpu' is uninitialized when used here [-Wuninitialized]
                   if (!kdb_task_has_cpu(p) || kgdb_info[cpu].irq_depth == 1) {
                                                         ^~~
   kernel/debug/kdb/kdb_support.c:490:9: note: initialize the variable 'cpu' to silence this warning
           int cpu;
                  ^
                   = 0
   8 warnings generated.


vim +/cpu +501 kernel/debug/kdb/kdb_support.c

5d5314d6795f3c Jason Wessel      2010-05-20  476  
5d5314d6795f3c Jason Wessel      2010-05-20  477  
5d5314d6795f3c Jason Wessel      2010-05-20  478  
5d5314d6795f3c Jason Wessel      2010-05-20  479  /*
5d5314d6795f3c Jason Wessel      2010-05-20  480   * kdb_task_state_char - Return the character that represents the task state.
5d5314d6795f3c Jason Wessel      2010-05-20  481   * Inputs:
5d5314d6795f3c Jason Wessel      2010-05-20  482   *	p	struct task for the process
5d5314d6795f3c Jason Wessel      2010-05-20  483   * Returns:
5d5314d6795f3c Jason Wessel      2010-05-20  484   *	One character to represent the task state.
5d5314d6795f3c Jason Wessel      2010-05-20  485   */
5d5314d6795f3c Jason Wessel      2010-05-20  486  char kdb_task_state_char (const struct task_struct *p)
5d5314d6795f3c Jason Wessel      2010-05-20  487  {
5d5314d6795f3c Jason Wessel      2010-05-20  488  	unsigned long tmp;
2f064a59a11ff9 Peter Zijlstra    2021-06-11  489  	char state;
2f064a59a11ff9 Peter Zijlstra    2021-06-11  490  	int cpu;
5d5314d6795f3c Jason Wessel      2010-05-20  491  
fe557319aa06c2 Christoph Hellwig 2020-06-17  492  	if (!p ||
fe557319aa06c2 Christoph Hellwig 2020-06-17  493  	    copy_from_kernel_nofault(&tmp, (char *)p, sizeof(unsigned long)))
5d5314d6795f3c Jason Wessel      2010-05-20  494  		return 'E';
5d5314d6795f3c Jason Wessel      2010-05-20  495  
9f7ea8ffacb27d Daniel Thompson   2021-10-29  496  	state = task_state_to_char((struct task_struct *) p);
9f7ea8ffacb27d Daniel Thompson   2021-10-29  497  
7fc20c5cbdd184 Paul E. McKenney  2011-11-10  498  	if (is_idle_task(p)) {
5d5314d6795f3c Jason Wessel      2010-05-20  499  		/* Idle task.  Is it really idle, apart from the kdb
5d5314d6795f3c Jason Wessel      2010-05-20  500  		 * interrupt? */
5d5314d6795f3c Jason Wessel      2010-05-20 @501  		if (!kdb_task_has_cpu(p) || kgdb_info[cpu].irq_depth == 1) {
5d5314d6795f3c Jason Wessel      2010-05-20  502  			if (cpu != kdb_initial_cpu)
9f7ea8ffacb27d Daniel Thompson   2021-10-29  503  				state = '-';	/* idle task */
5d5314d6795f3c Jason Wessel      2010-05-20  504  		}
9f7ea8ffacb27d Daniel Thompson   2021-10-29  505  	} else if (!p->mm && strchr("IMS", state)) {
9f7ea8ffacb27d Daniel Thompson   2021-10-29  506  		state = tolower(state);		/* sleeping system daemon */
5d5314d6795f3c Jason Wessel      2010-05-20  507  	}
5d5314d6795f3c Jason Wessel      2010-05-20  508  	return state;
5d5314d6795f3c Jason Wessel      2010-05-20  509  }
5d5314d6795f3c Jason Wessel      2010-05-20  510  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33587 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-10-31  3:50 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-16 15:42 [PATCH] kdb: Adopt scheduler's task clasification Daniel Thompson
2021-09-16 16:28 ` Doug Anderson
2021-09-17  8:18   ` Daniel Thompson
2021-09-17 14:36     ` Doug Anderson
2021-09-17  0:42 ` kernel test robot
2021-09-17  5:19 ` kernel test robot
2021-10-29 17:19 ` [PATCH v2] " Daniel Thompson
2021-10-31  3:49   ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).