Message ID | 20221206122357.309982-1-urezki@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp2790759wrr; Tue, 6 Dec 2022 04:31:09 -0800 (PST) X-Google-Smtp-Source: AA0mqf6Y/RN+tVUqIn3I1PCaxHw6x11mWlzolhEysKWlvt9ARE5yTGVU+vH0mFAArhDgmrcZVlOa X-Received: by 2002:a17:903:248c:b0:189:c8d9:42e9 with SMTP id p12-20020a170903248c00b00189c8d942e9mr15024998plw.124.1670329869015; Tue, 06 Dec 2022 04:31:09 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1670329869; cv=none; d=google.com; s=arc-20160816; b=lRQwIL6MqSCoVuwV4UYJERA1n0cUhXZpOWET8yXTNj6/rCIowwjS2gl70F1EOX9pep QTJ9y8lwj/btj8xMtwmAWbG98kUeX4ijBG2dnxpx/LZNRSDPZG84RGLoG0OaGndrixWj mgRpwZvmYTfSJdJcsyene/U7M4/HyrEtk/v3D/nOYUNTKByPkLfR1TPoq1nzAPoDurK2 djAd+FWNpYZVwYP24fsHnMl6s/YsCtU31pQuORPSoDaTpUOoXDI8FEaMLo939Yz+2DVe M3Mus/AmSwZzfaQjHdKv2abYcCFngZGPxskvbLt1z2i/HJDaGDv8SvKvXH/YeuzqwfBo x5pg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=OzQf6O96kQU/9b9Rxkvx78FK/If7e44i6Ub9+L4DsqA=; b=Nzg3bVODHDsGPx6Iq4xE8jZFevCyo1Qtk9+Y0/iAnit8wijnEUAqzDGxlYVTl6AbA8 EL8Sb54aSQT+44sPjMdtGH5V1mpYoS390hycSmxOR515H0Kimq84pPQBpekiuzm6cndX HKsuZO5dZgrieI6bd7mWyc4e0quGzcl6VpVMXq/EhbaisoX1TRfWj10q98ubQwGxZj1+ zKic/K6z/9XMpvdNxcL02FtKekDw6fC0+SPHVFKUBQWTsIa0JkLbnVBSr8fqAe9w914d TvAHSndiJybK4fIYgxUa7U0MSAambSxmVVkITzD6PH0RbXpgqMG/655qSFVYY0jldXWc 7/wA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=o7v93TF1; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id s25-20020a632159000000b004785a1141cbsi17218676pgm.485.2022.12.06.04.30.53; Tue, 06 Dec 2022 04:31:08 -0800 (PST) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=o7v93TF1; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234229AbiLFMYK (ORCPT <rfc822;jaysivo@gmail.com> + 99 others); Tue, 6 Dec 2022 07:24:10 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41598 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229449AbiLFMYG (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 6 Dec 2022 07:24:06 -0500 Received: from mail-ej1-x635.google.com (mail-ej1-x635.google.com [IPv6:2a00:1450:4864:20::635]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3CC5C28E27; Tue, 6 Dec 2022 04:24:02 -0800 (PST) Received: by mail-ej1-x635.google.com with SMTP id vv4so5801123ejc.2; Tue, 06 Dec 2022 04:24:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=OzQf6O96kQU/9b9Rxkvx78FK/If7e44i6Ub9+L4DsqA=; b=o7v93TF1sVonrEY7ulPfEf8o94p95sAJIVR6eq03qcEidh/Ysbh+du4MLAQa43Gx1H vVfNybv3GvG2C1mLya3T1JzIPLVPP6Ea7rtx1hJKyvzRivXNShVyTwjpmrLgir7GBv6K tPo6ixyK8PGA6OJbxPslhm0GgB5HyiPSXYG6AvUzIbBpP1ZGtB1weJxkElGoq4EmbkCb ETVFbW4pJnFtbpUxwtpG2iG/7JVv/+oOHvSm4oPljt0dUz/xIT2Gz0VS7zawGcyDLfoX ipQB23vWXyZ3uzLjHrw9Yfrl6flLF4VVj7M7fco/5TajYr5/GOUpExuGJ/6aiTfsDU3j sMcg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=OzQf6O96kQU/9b9Rxkvx78FK/If7e44i6Ub9+L4DsqA=; b=jJXvrHr1p/dGGhXuqSib8EfZ6jyH1sI0xgm9ciUkoVOxffzz6VuVRMSPEJOQpBb2t7 R5Z78DhHTEKOSGJO3FDhLyOohEyq3nvz5bJBzznmKUvT4+TUpqIxrv/mRM6arnZPqt3/ 8P0BvPVDMB7SLSc4I3tNRBssCH+9QbdLkz7188F26yx2rBCTGdLjVQLtytrgVm7IjVRg nZ9VCW+gP+uExca5A76IZ7c6LCqcKc3pwKBSw5FPQ0IB9hOOXYjhiffyC61HBGZb+82m 3l9hUTveDo4pOO2t4uyjpoSQ4QC+NPGgDIECZcF6Yy1KxcjoMUd+LRAZWhbFNZzYjvX8 7Epw== X-Gm-Message-State: ANoB5pl3Yy1+ydLErNGWxn+/JAT0ZZiUUuJsUM1s/4304n2/EtYwrlDC r9Ip0c1cjt9u8PEAjuBG2dRI9H7bA6Sv9g== X-Received: by 2002:a17:906:258d:b0:7c0:aea3:a9a8 with SMTP id m13-20020a170906258d00b007c0aea3a9a8mr19412181ejb.718.1670329440532; Tue, 06 Dec 2022 04:24:00 -0800 (PST) Received: from pc638.lan ([155.137.26.201]) by smtp.gmail.com with ESMTPSA id dy25-20020a05640231f900b004643f1524f3sm920681edb.44.2022.12.06.04.23.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 06 Dec 2022 04:24:00 -0800 (PST) From: "Uladzislau Rezki (Sony)" <urezki@gmail.com> To: LKML <linux-kernel@vger.kernel.org>, RCU <rcu@vger.kernel.org>, "Paul E . McKenney" <paulmck@kernel.org> Cc: Frederic Weisbecker <frederic@kernel.org>, Neeraj Upadhyay <neeraj.iitr10@gmail.com>, Joel Fernandes <joel@joelfernandes.org>, Uladzislau Rezki <urezki@gmail.com>, Oleksiy Avramchenko <oleksiy.avramchenko@sony.com> Subject: [PATCH 1/1] rcu/kvfree: Add runtime sleep check for single argument Date: Tue, 6 Dec 2022 13:23:57 +0100 Message-Id: <20221206122357.309982-1-urezki@gmail.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1751467812917954894?= X-GMAIL-MSGID: =?utf-8?q?1751467812917954894?= |
Series |
[1/1] rcu/kvfree: Add runtime sleep check for single argument
|
|
Commit Message
Uladzislau Rezki
Dec. 6, 2022, 12:23 p.m. UTC
A single argument can be invoked only from a sleepable
context. There is already a might_sleep() check to mitigate
such cases. The problem is that it requires a kernel to
be built with a CONFIG_DEBUG_ATOMIC_SLEEP option.
In order to improve an extra runtime_assert_can_sleep()
function is introduced by this patch. It is a run-time
checking. Please note it only covers PREEMPT kernels.
Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
kernel/rcu/tree.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
Comments
Hello Vlad, > On Dec 6, 2022, at 7:24 AM, Uladzislau Rezki (Sony) <urezki@gmail.com> wrote: > > A single argument can be invoked only from a sleepable > context. There is already a might_sleep() check to mitigate > such cases. > The problem is that it requires a kernel to > be built with a CONFIG_DEBUG_ATOMIC_SLEEP option. > > In order to improve an extra runtime_assert_can_sleep() > function is introduced by this patch. It is a run-time > checking. Please note it only covers PREEMPT I would call it preemptible kernels. > kernels. Also, It is not clear at all from the commit message about what we are checking and why. Neither is it clear why the might_sleep() is insufficient. The whole point of doing this is, the purpose of might_sleep() is to check whether we are blocking in an atomic context. That will not help Eric issue which is totally different - we would like to know if we are using an API where we can block instead of an API where we do not need to, by providing additional rcu_head space. > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > --- > kernel/rcu/tree.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index d155f2594317..bb798f07e768 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -3232,6 +3232,19 @@ add_ptr_to_bulk_krc_lock(struct kfree_rcu_cpu **krcp, > return true; > } > > +static void > +runtime_assert_can_sleep(void) > +{ > + if (!IS_ENABLED(CONFIG_PREEMPT_COUNT)) > + return; > + > + if (preemptible()) > + return; These 2 iffs can be combined into 2-3 lines in this function. No need to add more LOC than needed. > + > + WARN_ONCE(1, "in_atomic(): %d, irqs_disabled(): %d, pid: %d, name: %s\n", > + in_atomic(), irqs_disabled(), current->pid, current->comm); > +} > + > /* > * Queue a request for lazy invocation of the appropriate free routine > * after a grace period. Please note that three paths are maintained, > @@ -3257,8 +3270,10 @@ void kvfree_call_rcu(struct rcu_head *head, void *ptr) > * only. For other places please embed an rcu_head to > * your data. > */ > - if (!head) > + if (!head) { > + runtime_assert_can_sleep(); > might_sleep(); runtime_assert_preemptible() is a better name with a comment about big comment on top of the new function about false negatives for non-preemptible kernels. Can sleep sounds like might sleep which just adds confusion and does not help code readers. Also Paul raised a point about using 1-arg API in some sleepable contexts where the caller does not want to introduce new space for the head. Have we confirmed there are not any? If there are, the warning will fire for those, as false-positives. Cheers, - Joel > + } > > // Queue the object but don't yet schedule the batch. > if (debug_rcu_head_queue(ptr)) { > -- > 2.30.2 >
On Tue, Dec 06, 2022 at 09:45:11AM -0500, Joel Fernandes wrote: > Hello Vlad, > > > On Dec 6, 2022, at 7:24 AM, Uladzislau Rezki (Sony) <urezki@gmail.com> wrote: > > > > A single argument can be invoked only from a sleepable > > context. There is already a might_sleep() check to mitigate > > such cases. > > The problem is that it requires a kernel to > > be built with a CONFIG_DEBUG_ATOMIC_SLEEP option. > > > > In order to improve an extra runtime_assert_can_sleep() > > function is introduced by this patch. It is a run-time > > checking. Please note it only covers PREEMPT > > I would call it preemptible kernels. > > > kernels. > > Also, It is not clear at all from the commit message about what we are checking and why. Neither is it clear why the might_sleep() is insufficient. > > The whole point of doing this is, the purpose of might_sleep() is to check whether we are blocking in an atomic context. That will not help Eric issue which is totally different - we would like to know if we are using an API where we can block instead of an API where we do not need to, by providing additional rcu_head space. > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > > --- > > kernel/rcu/tree.c | 17 ++++++++++++++++- > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > index d155f2594317..bb798f07e768 100644 > > --- a/kernel/rcu/tree.c > > +++ b/kernel/rcu/tree.c > > @@ -3232,6 +3232,19 @@ add_ptr_to_bulk_krc_lock(struct kfree_rcu_cpu **krcp, > > return true; > > } > > > > +static void > > +runtime_assert_can_sleep(void) > > +{ > > + if (!IS_ENABLED(CONFIG_PREEMPT_COUNT)) > > + return; > > + > > + if (preemptible()) > > + return; > > These 2 iffs can be combined into 2-3 lines in this function. No need to add more LOC than needed. > It can be. > > + > > + WARN_ONCE(1, "in_atomic(): %d, irqs_disabled(): %d, pid: %d, name: %s\n", > > + in_atomic(), irqs_disabled(), current->pid, current->comm); > > +} > > + > > /* > > * Queue a request for lazy invocation of the appropriate free routine > > * after a grace period. Please note that three paths are maintained, > > @@ -3257,8 +3270,10 @@ void kvfree_call_rcu(struct rcu_head *head, void *ptr) > > * only. For other places please embed an rcu_head to > > * your data. > > */ > > - if (!head) > > + if (!head) { > > + runtime_assert_can_sleep(); > > might_sleep(); > > runtime_assert_preemptible() is a better name with a comment about big comment on top of the new function about false negatives for non-preemptible kernels. Can sleep sounds like might sleep which just adds confusion and does not help code readers. > > Also Paul raised a point about using 1-arg API in some sleepable contexts where the caller does not want to introduce new space for the head. Have we confirmed there are not any? If there are, the warning will fire for those, as false-positives. > single argument is only for sleepable contexts. This is a rule. -- Uladzislau Rezki
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index d155f2594317..bb798f07e768 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -3232,6 +3232,19 @@ add_ptr_to_bulk_krc_lock(struct kfree_rcu_cpu **krcp, return true; } +static void +runtime_assert_can_sleep(void) +{ + if (!IS_ENABLED(CONFIG_PREEMPT_COUNT)) + return; + + if (preemptible()) + return; + + WARN_ONCE(1, "in_atomic(): %d, irqs_disabled(): %d, pid: %d, name: %s\n", + in_atomic(), irqs_disabled(), current->pid, current->comm); +} + /* * Queue a request for lazy invocation of the appropriate free routine * after a grace period. Please note that three paths are maintained, @@ -3257,8 +3270,10 @@ void kvfree_call_rcu(struct rcu_head *head, void *ptr) * only. For other places please embed an rcu_head to * your data. */ - if (!head) + if (!head) { + runtime_assert_can_sleep(); might_sleep(); + } // Queue the object but don't yet schedule the batch. if (debug_rcu_head_queue(ptr)) {