Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220712021527.109921-1-lihuafei1@huawei.com> <20220712021527.109921-4-lihuafei1@huawei.com>
In-Reply-To: <20220712021527.109921-4-lihuafei1@huawei.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Mon, 18 Jul 2022 11:07:43 +0200
Message-ID: <CACRpkdYvOjfmf=Z3pGfD-UPxfTc9PXtOyw2x+ptYiSy=gmGnpQ@mail.gmail.com>
Subject: Re: [PATCH 3/5] ARM: stacktrace: Allow stack trace saving for
 non-current tasks
To:     Li Huafei <lihuafei1@huawei.com>
Cc:     linux@armlinux.org.uk, rmk+kernel@armlinux.org.uk, ardb@kernel.org,
        will@kernel.org, mark.rutland@arm.com, broonie@kernel.org,
        peterz@infradead.org, mingo@redhat.com, acme@kernel.org,
        alexander.shishkin@linux.intel.com, jolsa@kernel.org,
        namhyung@kernel.org, arnd@arndb.de, rostedt@goodmis.org,
        nick.hawkins@hpe.com, john@phrozen.org, mhiramat@kernel.org,
        ast@kernel.org, linyujun809@huawei.com, ndesaulniers@google.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-perf-users@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jul 12, 2022 at 4:18 AM Li Huafei <lihuafei1@huawei.com> wrote:

> The current ARM implementation of save_stack_trace_tsk() does not allow
> saving stack trace for non-current tasks, which may limit the scenarios
> in which stack_trace_save_tsk() can be used. Like other architectures,
> or like ARM's unwind_backtrace(), we can leave it up to the caller to
> ensure that the task that needs to be unwound is not running.
>
> Signed-off-by: Li Huafei <lihuafei1@huawei.com>

That sounds good, but:

>         if (tsk != current) {
> -#ifdef CONFIG_SMP
> -               /*
> -                * What guarantees do we have here that 'tsk' is not
> -                * running on another CPU?  For now, ignore it as we
> -                * can't guarantee we won't explode.
> -                */
> -               return;
> -#else
> +               /* task blocked in __switch_to */

The commit text is not consistent with the comment you are removing.

The commit is talking about "non-current" tasks which is one thing,
but the code is avoiding any tasks under SMP because they may be
running on another CPU. So you need to update the commit
message to say something like "non-current or running on another CPU".

If this condition will be checked at call sites in following patches,
then mention
that in the commit as well, so we know the end result is that we do
not break it,

I think Russell want to check this commit as well,

Yours,
Linus Walleij