Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211102173158.3315227-1-daniel.thompson@linaro.org>
In-Reply-To: <20211102173158.3315227-1-daniel.thompson@linaro.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Tue, 2 Nov 2021 13:12:01 -0700
Message-ID: <CAD=FV=Uk43Y3L0oTAxR1YDBUO3axz3joK2NGkGNXQPbUPwerig@mail.gmail.com>
Subject: Re: [PATCH v4] kdb: Adopt scheduler's task classification
To:     Daniel Thompson <daniel.thompson@linaro.org>
Cc:     Jason Wessel <jason.wessel@windriver.com>,
        Xiang wangx <wangxiang@cdjrlc.com>,
        jing yangyang <jing.yangyang@zte.com.cn>,
        kgdb-bugreport@lists.sourceforge.net, linux-kernel@vger.kernel.org,
        patches@linaro.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Tue, Nov 2, 2021 at 10:32 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 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.
>
> Note: The Fixes below is the last point the original dubious code was
>       moved; it was not introduced by that patch. However it gives us
>       the last point to which this patch can be easily backported.
>       Happily that should be enough to cover the introduction of
>       CONFIG_WERROR!
>
> Fixes: 2f064a59a11f ("sched: Change task_struct::state")
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
>
> Notes:
>     v4:
>     - Get rid of the final `DRSTCZEUIMA` from the comments (Doug)
>     - Change "state [ism]" to "state [ims]" to match other uses
>       of IMS/ims (Doug)
>     - Fix broken english in the bta online help (Doug)
>     - Update ps online help to use <state_chars> to match other
>       synopses (me)
>
>     v3:
>     - Fix the uninitialized cpu variable (Doug and 0-day CI bot)
>     - Added a Fixes: (Doug)
>     - Changed "state I" -> "state -" and "state M" to "state [ism]"
>
>     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      |  16 ++---
>  kernel/debug/kdb/kdb_main.c    |  37 ++++++-----
>  kernel/debug/kdb/kdb_private.h |   4 +-
>  kernel/debug/kdb/kdb_support.c | 118 +++++++--------------------------
>  4 files changed, 53 insertions(+), 122 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>