Messages in this thread | | | Date | Fri, 17 Sep 2021 09:18:16 +0100 | From | Daniel Thompson <> | Subject | Re: [PATCH] kdb: Adopt scheduler's task clasification |
| |
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.
| |