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;
Date:   Tue, 11 Apr 2023 22:20:37 -0700
From:   Josh Poimboeuf <jpoimboe@kernel.org>
To:     Qi Zheng <zhengqi.arch@bytedance.com>
Cc:     peterz@infradead.org, keescook@chromium.org,
        dave.hansen@linux.intel.com, bp@alien8.de, mingo@redhat.com,
        tglx@linutronix.de, rostedt@goodmis.org, x86@kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] x86: make __get_wchan() use arch_stack_walk()
Message-ID: <20230412052037.nz75jeoeqibx6otr@treble>
References: <20230330081552.54178-1-zhengqi.arch@bytedance.com>
 <20230330081552.54178-3-zhengqi.arch@bytedance.com>
 <20230408050822.ezdbhc6j7zuvutrm@treble>
 <fffd59ff-eaa8-bba7-03f2-9fc27620bd1c@bytedance.com>
 <20230408221206.mi7nsquoxklgword@treble>
 <4aba3e74-a205-5dd1-69bc-9186c04e733d@bytedance.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <4aba3e74-a205-5dd1-69bc-9186c04e733d@bytedance.com>
Precedence: bulk

On Sun, Apr 09, 2023 at 02:30:23PM +0800, Qi Zheng wrote:
> 
> 
> On 2023/4/9 06:12, Josh Poimboeuf wrote:
> > On Sat, Apr 08, 2023 at 01:36:06PM +0800, Qi Zheng wrote:
> > > 
> > > 
> > > On 2023/4/8 13:08, Josh Poimboeuf wrote:
> > > > On Thu, Mar 30, 2023 at 04:15:52PM +0800, Qi Zheng wrote:
> > > > > Make __get_wchan() use arch_stack_walk() directly to
> > > > > avoid open-coding of unwind logic.
> > > > > 
> > > > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
> > > > 
> > > > Can we just have a shared version of __get_wchan() for all
> > > > CONFIG_ARCH_STACKWALK arches?
> > > 
> > >  From a quick glance, I think it's ok, but we still need to define
> > > the arch's own get_wchan_cb(). I will try to do it.
> > 
> > Hm, why would we need to do that?
> 
> Because I see checks for count++ < 16 exist in __get_wchan() for some
> arches and some don't. So I'm not sure if this check can be discarded
> after using arch_stack_walk(). And I see that this check is retained in
> arm64, see [1] for details.
> 
> [1]. https://github.com/torvalds/linux/commit/4f62bb7cb165f3e7b0a91279fe9dd5c56daf3457

That difference seems to have nothing to do with individual arch
differences.

The 16-check limit looks like some ancient cargo cult ritual which was
copy-pasted decades ago, presumably to avoid some kind of infinite stack
recursion loop in scheduler code, which should never happen.  That
should definitely be removed.

Another good reason to unify them, to get rid of cruft like that.

-- 
Josh