From patchwork Thu Sep 16 15:42:53 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Thompson X-Patchwork-Id: 512954 Delivered-To: patches@linaro.org Received: by 2002:a02:c816:0:0:0:0:0 with SMTP id p22csp1332534jao; Thu, 16 Sep 2021 08:43:11 -0700 (PDT) X-Received: by 2002:a5d:59ae:: with SMTP id p14mr6992403wrr.278.1631806990953; Thu, 16 Sep 2021 08:43:10 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1631806990; cv=none; d=google.com; s=arc-20160816; b=JBRynIz0fB5pghyoX6En3/R4UJymORkKlw1Ap6F5QpqAJ0nJDc6Yqfc1SHd5t+26YK l9wN0dQ3xs6+GFQFr61rIVt7xsGJ3Pl+eqkMsx7EJKBDCmvm6cSRiEwxzZvp32yNBPKO qNmVEbjOSPHHb0zjT/akyqPNaOKsXNfJC3kyDGRXZFxi9DO4MT0gttDKBaYuwYec5q2w NqzJCsyOG+bHC2DMMyVtNZMZTUJKhJQTr4r4j9nb2LkxL3hXTuhidwZnfPcCX2JD9MFj xYf0F7UwZqnDeIiXG8ZjU9Vrvwo+m2yLZBbS31/q9s9Zpvrn/hNNeWehVZO+NzRV9GJh hSrg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:dkim-signature; bh=0hpx6vfCRxVwPdycjFPNAg/3cxPnhkKULjjr9EJ/26Q=; b=rL4sxvUDMkOFk7qM4/OiDOTpa2zIsItoV1E+tqqOGfIBg3OzwT3AuGjfnr0phrvm+F Yu4AARmOMQKhywmq3zsOpbl5W6+roA3KJwjQFjm/Pb5B6x79lm9kVOsU4mZ6tfDeh3YD FgQHzSpPGWUerH2Amu527vnQcCxMSAiODSEQBofdbPRoTSsaeCuSnSofiiULiHnQJMwB VryWYlGZJVQBVFYMgWHkinBThQ++dJCzYqfSvkRXAzvItSiRhbEOzw+YPoAxwhZxMTM1 vkqSXzgnj0mOGTT5sOOtUdH0Ic7jJrWF9cMX//GQbYJZYuaBmMMPgWKBOA8SUc5RVX7c B2Tg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=TybvUSI9; spf=pass (google.com: domain of daniel.thompson@linaro.org designates 209.85.220.41 as permitted sender) smtp.mailfrom=daniel.thompson@linaro.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from mail-sor-f41.google.com (mail-sor-f41.google.com. [209.85.220.41]) by mx.google.com with SMTPS id r10sor2042583wrz.64.2021.09.16.08.43.10 for (Google Transport Security); Thu, 16 Sep 2021 08:43:10 -0700 (PDT) Received-SPF: pass (google.com: domain of daniel.thompson@linaro.org designates 209.85.220.41 as permitted sender) client-ip=209.85.220.41; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=TybvUSI9; spf=pass (google.com: domain of daniel.thompson@linaro.org designates 209.85.220.41 as permitted sender) smtp.mailfrom=daniel.thompson@linaro.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=0hpx6vfCRxVwPdycjFPNAg/3cxPnhkKULjjr9EJ/26Q=; b=TybvUSI9fnxtIF/GZIgP57KLQxhOljFZSDGPRDuhWfHU1UY5J2xN19iQWmZaKT9qWa xWdBmCjWM0VSm134JTgMlUomqV94mROhoDq5wcOI0C5Vv/66CBpDSW9D4ap3VbkduUHD F2skswF8KddaZpoeJB3ExUN957kP76riJoxIgSSdwdtQhHXqJuszx17ssGFiyYThPETA SjY+oChXT9Zp55rO2orgs8CdGI65geLbIbUw7d5Rcr3eQq8PdirfIZmhZZ3wUSPRpKym L1y5LxB6QwwR6q8bFgXY8biBJpHH0A6KWB00BaSYCINpOzkIX8Ia2Y/VH/D4YmaMwpiG lq4g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=0hpx6vfCRxVwPdycjFPNAg/3cxPnhkKULjjr9EJ/26Q=; b=LwgcCJkUMB0WqcQEu7yAtfeNwN+Ba7HouUqxqgm2i57vyUy6KEh7R6uksYG3243NsA RfyOqD04t+VDHI8xg6ZJcVQVl1go1euo8dKxRdVlL1sg2tccDMHPxgQ6YscgmuH+BpKx fOsVkJIWOBJTLNv0ihwIsg69h/M2a0m+Su71NABrQOEGy9+5n8I2d8Cj8gC9k2sArR6j 6e7NmHH5fUeJt+/Vm1i4R4dY+WE3MTRQYx8dDN5mcpzHda//XsIfxFH/HLUhfdYTGXks PtrA0d5TkIzBIw1lgX0xdObvzK0w0ZTxQ85uoF+djvTkN7Rpgf/wnf+4EgcKiqAvPpA+ 0d6g== X-Gm-Message-State: AOAM530A4Z1HovVhwiEqE1aGuwLceV8xVJIzdpM5M3ZSIsQDT3sxQTa1 XG3zNECSCODlYRLnwfjD/Nq3m648 X-Google-Smtp-Source: ABdhPJzSX53JlDN51HkyU+eD/EVcBNONINm0LkcIxNShii7GO/vonuPKkOQ0EFKaaorPgEdstgsUrg== X-Received: by 2002:a5d:59a4:: with SMTP id p4mr6887049wrr.149.1631806990393; Thu, 16 Sep 2021 08:43:10 -0700 (PDT) Return-Path: Received: from wychelm.lan (cpc141216-aztw34-2-0-cust174.18-1.cable.virginm.net. [80.7.220.175]) by smtp.gmail.com with ESMTPSA id y64sm1104168wmc.38.2021.09.16.08.43.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 16 Sep 2021 08:43:09 -0700 (PDT) From: Daniel Thompson To: Jason Wessel , Daniel Thompson , Douglas Anderson Cc: Xiang wangx , jing yangyang , kgdb-bugreport@lists.sourceforge.net, linux-kernel@vger.kernel.org, patches@linaro.org Subject: [PATCH] kdb: Adopt scheduler's task clasification Date: Thu, 16 Sep 2021 16:42:53 +0100 Message-Id: <20210916154253.2731609-1-daniel.thompson@linaro.org> X-Mailer: git-send-email 2.31.1 MIME-Version: 1.0 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 --- 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(-) base-commit: 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f -- 2.31.1 Reported-by: kernel test robot Reported-by: kernel test robot 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