From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7301FC433F5 for ; Wed, 13 Oct 2021 02:05:45 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A285360C49 for ; Wed, 13 Oct 2021 02:05:44 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org A285360C49 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.alibaba.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.ozlabs.org Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4HTbTy6g4yz2yQB for ; Wed, 13 Oct 2021 13:05:42 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=linux.alibaba.com (client-ip=115.124.30.43; helo=out30-43.freemail.mail.aliyun.com; envelope-from=yun.wang@linux.alibaba.com; receiver=) Received: from out30-43.freemail.mail.aliyun.com (out30-43.freemail.mail.aliyun.com [115.124.30.43]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4HTbTQ20JHz2yL7 for ; Wed, 13 Oct 2021 13:05:11 +1100 (AEDT) X-Alimail-AntiSpam: AC=PASS; BC=-1|-1; BR=01201311R171e4; CH=green; DM=||false|; DS=||; FP=0|-1|-1|-1|0|-1|-1|-1; HT=e01e04394; MF=yun.wang@linux.alibaba.com; NM=1; PH=DS; RN=31; SR=0; TI=SMTPD_---0UrdlKN._1634090692; Received: from testdeMacBook-Pro.local(mailfrom:yun.wang@linux.alibaba.com fp:SMTPD_---0UrdlKN._1634090692) by smtp.aliyun-inc.com(127.0.0.1); Wed, 13 Oct 2021 10:04:54 +0800 Subject: Re: [PATCH 1/2] ftrace: disable preemption on the testing of recursion To: Steven Rostedt References: <8c7de46d-9869-aa5e-2bb9-5dbc2eda395e@linux.alibaba.com> <20211012084331.06b8dd23@gandalf.local.home> From: =?UTF-8?B?546L6LSH?= Message-ID: <1eab20c1-d69b-f94b-92ff-4329d0aff6a2@linux.alibaba.com> Date: Wed, 13 Oct 2021 10:04:52 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <20211012084331.06b8dd23@gandalf.local.home> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "Peter Zijlstra \(Intel\)" , Paul Walmsley , "James E.J. Bottomley" , Guo Ren , Jisheng Zhang , "H. Peter Anvin" , live-patching@vger.kernel.org, linux-riscv@lists.infradead.org, Miroslav Benes , Paul Mackerras , Joe Lawrence , Helge Deller , x86@kernel.org, linux-csky@vger.kernel.org, Ingo Molnar , Petr Mladek , Albert Ou , Jiri Kosina , Nicholas Piggin , Borislav Petkov , Josh Poimboeuf , Thomas Gleixner , linux-parisc@vger.kernel.org, linux-kernel@vger.kernel.org, Palmer Dabbelt , Masami Hiramatsu , Colin Ian King , linuxppc-dev@lists.ozlabs.org Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On 2021/10/12 δΈ‹εˆ8:43, Steven Rostedt wrote: > On Tue, 12 Oct 2021 13:40:08 +0800 > ηŽ‹θ΄‡ wrote: > >> --- a/include/linux/trace_recursion.h >> +++ b/include/linux/trace_recursion.h >> @@ -214,7 +214,14 @@ static __always_inline void trace_clear_recursion(int bit) >> static __always_inline int ftrace_test_recursion_trylock(unsigned long ip, >> unsigned long parent_ip) >> { >> - return trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX); >> + int bit; >> + >> + preempt_disable_notrace(); > > The recursion test does not require preemption disabled, it uses the task > struct, not per_cpu variables, so you should not disable it before the test. > > bit = trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX); > if (bit >= 0) > preempt_disable_notrace(); > > And if the bit is zero, it means a recursion check was already done by > another caller (ftrace handler does the check, followed by calling perf), > and you really don't even need to disable preemption in that case. > > if (bit > 0) > preempt_disable_notrace(); > > And on the unlock, have: > > static __always_inline void ftrace_test_recursion_unlock(int bit) > { > if (bit) > preempt_enable_notrace(); > trace_clear_recursion(bit); > } > > But maybe that's over optimizing ;-) I see, while the user can still check smp_processor_id() after trylock return bit 0... I guess Peter's point at very beginning is to prevent such cases, since kernel for production will not have preemption debug on, and such issue won't get report but could cause trouble which really hard to trace down , way to eliminate such issue once for all sounds attractive, isn't it? Regards, Michael Wang > > -- Steve > > >> + bit = trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX); >> + if (bit < 0) >> + preempt_enable_notrace(); >> + >> + return bit; >> }