srcu: Make srcu_might_be_idle() take early return if rcu_gp_is_normal() return true
Message ID | 20230704082615.7415-1-qiang.zhang1211@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:9f45:0:b0:3ea:f831:8777 with SMTP id v5csp1071849vqx; Tue, 4 Jul 2023 01:56:12 -0700 (PDT) X-Google-Smtp-Source: APBJJlF+79MU0IjlO9xA8ff7ZfMOeLPNZi6DgHtMJP4zmGzpWo7flwdmer/UcJ7lis39o5RnT68C X-Received: by 2002:a05:6358:e81a:b0:133:ed8:1ccd with SMTP id gi26-20020a056358e81a00b001330ed81ccdmr6515155rwb.24.1688460971948; Tue, 04 Jul 2023 01:56:11 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1688460971; cv=none; d=google.com; s=arc-20160816; b=Inak2BEzoIpWenwutEsjG1RKG5H4V7KLkvI8G7jkd9SIRpVLqnG4JlOaIUONRSi3pE cSB85C7Ns3QYThDUqbZYU+L6+hSlhCy2OzlSTjF1+m4dA4Reho5dtvLUvvJCEbw18VkP LjaesKAgy1jso1TQUIC7wQQNJzwQ/iX88HRkk0wsSoZiNyInakOhsQRQ+r7zqvzTBdP8 fGrLkhHkZ411WroOl8KmXJdqFSLCyjhvPd9qbAH3gAuYvSxywNgVcmB8LPiIrKIgyElR 1ZdnI4LcHfThLqmGRom57J3spC353NnLZ3nBuqFKrlc6HID9PHWHdZues6OXVCdVWXzv suvw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:message-id:date:subject:cc:to:from :dkim-signature; bh=rnaNPjwtFpn4gloZKKRgwucu8ZcCmYCODm5Ceekydb4=; fh=gBHKmotWusbmxz5JEuHA/RTvfoLB0D9BR2NQ7/FrpLw=; b=L6FnB6dVbOxUa2Hb1zRmrPhXetkCQQPeAY7HgecspHeNCsuW7+MwSIUTcDwRC0cBWO QYoRnZyux5BS2CyyDFd3q+M/OjQTjFBFDyNQQH+bDP1tSTHJcbhgKEsUkyL/fzim8KHT kpxaNOns5XuC7YXNdwgxLy1wqifYbtnj3EZVXVxP7u3mJAXHegQnPgWIrP/r2SexVmhw kC0+7zhjKfHCzpfeLH3ulG8SeVqk/mrvJ7IqUswWANslWpta7GJB5q1V+WxlBlwDkCWn FeKxvFx9+bwI8hvaAkCtcT8yxvmyKo8oLxAo5vL8Bi4jfU+2hq7WofnyQKwJk6vvFAei 6DgA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20221208 header.b=EHdc7aRT; 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 e192-20020a6369c9000000b0055337508370si19806519pgc.889.2023.07.04.01.55.59; Tue, 04 Jul 2023 01:56:11 -0700 (PDT) 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=20221208 header.b=EHdc7aRT; 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 S231510AbjGDI02 (ORCPT <rfc822;ybw1215001957@gmail.com> + 99 others); Tue, 4 Jul 2023 04:26:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47952 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230200AbjGDI00 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 4 Jul 2023 04:26:26 -0400 Received: from mail-pg1-x52d.google.com (mail-pg1-x52d.google.com [IPv6:2607:f8b0:4864:20::52d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6785AE4F; Tue, 4 Jul 2023 01:26:25 -0700 (PDT) Received: by mail-pg1-x52d.google.com with SMTP id 41be03b00d2f7-55bac17b442so1285237a12.3; Tue, 04 Jul 2023 01:26:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1688459185; x=1691051185; h=message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=rnaNPjwtFpn4gloZKKRgwucu8ZcCmYCODm5Ceekydb4=; b=EHdc7aRT9XArgRM48tM2jVTxPdc5eLwVUjh+wRH2gQ4aEJCTpYAQMtDfq29n++y6Uu K2GVfTJpzF83Hin2I7Z2oC3SzNqQ6+xBw2D4v4u3tpNSE1KY/r1B4naCdr+S/09Hw1pP sv4RXUjhvKL7Vo8t4dVaSY8P7wDEzZsnjjtDuxQUVDKIN4h8BGLwukKFgDicdFWBLtNh hypVz1c+Ot2WVGP0l9bxKNx2ERDtWB2tbEsh3DjZpWbsZWx85p88pZg+aWSAj/+kyl1y VAqyeddyKqpWPtuqOChXTT9fi1OYiMTrsIteeJYN0kuGb10cOLxIZdIQvY5wC3/btFV3 Gi6A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688459185; x=1691051185; h=message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=rnaNPjwtFpn4gloZKKRgwucu8ZcCmYCODm5Ceekydb4=; b=e1eL0cEAeqfaxAeCyHKN9Ji5ySus0EEAeljYzChldOdg0x3dTe3exl3dHW4ni/xWZp buEdzsv+jipQ5M4CaHPVSdZJu03LCmRmCTSKb3ZBSKHBykf7QPkYA4nCG8vWpe1YwwyB 3N5bE08cHHjHS8xRBSz8HtrMAsbwnRRsbgGvqI48YDS1s2tun/CJNI1ePZi334TyuyyH jS+TsSfLyRvJObIu6bZeE4aY16tppYfXBj9uxpdj1aA+xbMaBaFkRz7IHVXGkBT2Bdj9 6IAsZAnjifqVyIc4ZpqSKtiD5NTasFavJkapsRMMUe/KCOVfjDA6JXxqmRQLc6DAHbjb 4uqA== X-Gm-Message-State: ABy/qLaenM0VN7lE1KRAViiAWGaWYGA3Hp8QpW3axzrqWOo6Dk1ncXW2 2Ffs9bTmwMeK8ra4IV9YkgKjjRviOaI= X-Received: by 2002:a17:902:c10c:b0:1a5:150f:8558 with SMTP id 12-20020a170902c10c00b001a5150f8558mr12509689pli.17.1688459184860; Tue, 04 Jul 2023 01:26:24 -0700 (PDT) Received: from MSCND1355B05.fareast.nevint.com ([183.242.39.186]) by smtp.gmail.com with ESMTPSA id w18-20020a170902e89200b001a5fccab02dsm15781764plg.177.2023.07.04.01.26.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 04 Jul 2023 01:26:24 -0700 (PDT) From: Zqiang <qiang.zhang1211@gmail.com> To: paulmck@kernel.org, frederic@kernel.org, quic_neeraju@quicinc.com, joel@joelfernandes.org, qiang.zhang1211@gmail.com Cc: rcu@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] srcu: Make srcu_might_be_idle() take early return if rcu_gp_is_normal() return true Date: Tue, 4 Jul 2023 16:26:15 +0800 Message-Id: <20230704082615.7415-1-qiang.zhang1211@gmail.com> X-Mailer: git-send-email 2.17.1 X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS, T_SCC_BODY_TEXT_LINE 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?1770479652106025920?= X-GMAIL-MSGID: =?utf-8?q?1770479652106025920?= |
Series |
srcu: Make srcu_might_be_idle() take early return if rcu_gp_is_normal() return true
|
|
Commit Message
Z qiang
July 4, 2023, 8:26 a.m. UTC
When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current
CPU's sdp->lock will be acquired to check whether there are pending
callbacks in the sdp->srcu_cblist, if there are no pending callbacks,
probabilistically probe global state to decide whether to convert to
synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1
kernels and after the rcu_set_runtime_mode() is called, invoke the
rcu_gp_is_normal() is always return true, this mean that invoke the
synchronize_srcu_expedited() always fall back to synchronize_srcu(),
so there is no need to acquire sdp->lock to check sdp->srcu_cblist and
probe global state in srcu_might_be_idle().
This commit therefore make srcu_might_be_idle() return immediately if the
rcu_gp_is_normal() return true.
Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
---
kernel/rcu/srcutree.c | 2 ++
1 file changed, 2 insertions(+)
Comments
On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > CPU's sdp->lock will be acquired to check whether there are pending > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > probabilistically probe global state to decide whether to convert to > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > kernels and after the rcu_set_runtime_mode() is called, invoke the > rcu_gp_is_normal() is always return true, this mean that invoke the > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > probe global state in srcu_might_be_idle(). > > This commit therefore make srcu_might_be_idle() return immediately if the > rcu_gp_is_normal() return true. > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > --- > kernel/rcu/srcutree.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > index 20d7a238d675..aea49cb60a45 100644 > --- a/kernel/rcu/srcutree.c > +++ b/kernel/rcu/srcutree.c > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > unsigned long tlast; > > check_init_srcu_struct(ssp); > + if (rcu_gp_is_normal()) > + return false; Again, thank you for looking into SRCU! I am not at all enthusiastic about this one. With this change, the name srcu_might_be_idle() is no longer accurate. Yes, the name could change, but any name would be longer and more confusing. So unless there is a measureable benefit to this one on a production workload, I cannot justify taking it. Is there a measureable benefit? Thanx, Paul > /* If the local srcu_data structure has callbacks, not idle. */ > sdp = raw_cpu_ptr(ssp->sda); > spin_lock_irqsave_rcu_node(sdp, flags); > -- > 2.17.1 >
> > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > > CPU's sdp->lock will be acquired to check whether there are pending > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > > probabilistically probe global state to decide whether to convert to > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > > kernels and after the rcu_set_runtime_mode() is called, invoke the > > rcu_gp_is_normal() is always return true, this mean that invoke the > > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > > probe global state in srcu_might_be_idle(). > > > > This commit therefore make srcu_might_be_idle() return immediately if the > > rcu_gp_is_normal() return true. > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > --- > > kernel/rcu/srcutree.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > index 20d7a238d675..aea49cb60a45 100644 > > --- a/kernel/rcu/srcutree.c > > +++ b/kernel/rcu/srcutree.c > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > unsigned long tlast; > > > > check_init_srcu_struct(ssp); > > + if (rcu_gp_is_normal()) > > + return false; > > Again, thank you for looking into SRCU! > > I am not at all enthusiastic about this one. With this change, the name > srcu_might_be_idle() is no longer accurate. Yes, the name could change, > but any name would be longer and more confusing. > > So unless there is a measureable benefit to this one on a production > workload, I cannot justify taking it. > > Is there a measureable benefit? > Hi, Paul I only find that for Preempt-RT kernel, the rcu_normal_after_boot is set by default: static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT); This affects only rcu but also srcu, this make the synchronize_srcu() and synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true), this means that call the srcu_might_be_idle() is meaningless. Thanks Zqiang > > Thanx, Paul > > > /* If the local srcu_data structure has callbacks, not idle. */ > > sdp = raw_cpu_ptr(ssp->sda); > > spin_lock_irqsave_rcu_node(sdp, flags); > > -- > > 2.17.1 > >
On Fri, Jul 07, 2023 at 06:28:29PM +0800, Z qiang wrote: > > > > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > > > CPU's sdp->lock will be acquired to check whether there are pending > > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > > > probabilistically probe global state to decide whether to convert to > > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > > > kernels and after the rcu_set_runtime_mode() is called, invoke the > > > rcu_gp_is_normal() is always return true, this mean that invoke the > > > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > > > probe global state in srcu_might_be_idle(). > > > > > > This commit therefore make srcu_might_be_idle() return immediately if the > > > rcu_gp_is_normal() return true. > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > --- > > > kernel/rcu/srcutree.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > index 20d7a238d675..aea49cb60a45 100644 > > > --- a/kernel/rcu/srcutree.c > > > +++ b/kernel/rcu/srcutree.c > > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > > unsigned long tlast; > > > > > > check_init_srcu_struct(ssp); > > > + if (rcu_gp_is_normal()) > > > + return false; > > > > Again, thank you for looking into SRCU! > > > > I am not at all enthusiastic about this one. With this change, the name > > srcu_might_be_idle() is no longer accurate. Yes, the name could change, > > but any name would be longer and more confusing. > > > > So unless there is a measureable benefit to this one on a production > > workload, I cannot justify taking it. > > > > Is there a measureable benefit? > > Hi, Paul > > I only find that for Preempt-RT kernel, the rcu_normal_after_boot is > set by default: > static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT); > This affects only rcu but also srcu, this make the synchronize_srcu() and > synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true), > this means that call the srcu_might_be_idle() is meaningless. I do understand that the current setup favors default kernel builds at runtime by a few low-cost instructions, and that your change favors, as you say, kernels built for real-time, kernels built for certain types of HPC workloads, and all kernels during a small time during boot. My question is instead whether any of this makes a measureable difference at the system level. My guess is "no, not even close", but the way to convince me otherwise would be to actually run the workload and kernels on real hardware and provide measurements showing a statistically significant difference that the workload(s) in question care(s) about. So what can you show me? And srcu_might_be_idle() is not meaningless in that situation, just ignored completely. And that is in fact the nature and purpose of the C-language || operator. ;-) Thanx, Paul > Thanks > Zqiang > > > > > Thanx, Paul > > > > > /* If the local srcu_data structure has callbacks, not idle. */ > > > sdp = raw_cpu_ptr(ssp->sda); > > > spin_lock_irqsave_rcu_node(sdp, flags); > > > -- > > > 2.17.1 > > >
On Fri, Jul 7, 2023 at 12:05 PM Paul E. McKenney <paulmck@kernel.org> wrote: > > On Fri, Jul 07, 2023 at 06:28:29PM +0800, Z qiang wrote: > > > > > > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > > > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > > > > CPU's sdp->lock will be acquired to check whether there are pending > > > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > > > > probabilistically probe global state to decide whether to convert to > > > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > > > > kernels and after the rcu_set_runtime_mode() is called, invoke the > > > > rcu_gp_is_normal() is always return true, this mean that invoke the > > > > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > > > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > > > > probe global state in srcu_might_be_idle(). > > > > > > > > This commit therefore make srcu_might_be_idle() return immediately if the > > > > rcu_gp_is_normal() return true. > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > --- > > > > kernel/rcu/srcutree.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > > index 20d7a238d675..aea49cb60a45 100644 > > > > --- a/kernel/rcu/srcutree.c > > > > +++ b/kernel/rcu/srcutree.c > > > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > > > unsigned long tlast; > > > > > > > > check_init_srcu_struct(ssp); > > > > + if (rcu_gp_is_normal()) > > > > + return false; > > > > > > Again, thank you for looking into SRCU! > > > > > > I am not at all enthusiastic about this one. With this change, the name > > > srcu_might_be_idle() is no longer accurate. Yes, the name could change, > > > but any name would be longer and more confusing. > > > > > > So unless there is a measureable benefit to this one on a production > > > workload, I cannot justify taking it. > > > > > > Is there a measureable benefit? > > > > Hi, Paul > > > > I only find that for Preempt-RT kernel, the rcu_normal_after_boot is > > set by default: > > static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT); > > This affects only rcu but also srcu, this make the synchronize_srcu() and > > synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true), > > this means that call the srcu_might_be_idle() is meaningless. > > I do understand that the current setup favors default kernel builds at > runtime by a few low-cost instructions, and that your change favors, > as you say, kernels built for real-time, kernels built for certain types > of HPC workloads, and all kernels during a small time during boot. > > My question is instead whether any of this makes a measureable difference > at the system level. > > My guess is "no, not even close", but the way to convince me otherwise > would be to actually run the workload and kernels on real hardware and > provide measurements showing a statistically significant difference that > the workload(s) in question care(s) about. > > So what can you show me? > > And srcu_might_be_idle() is not meaningless in that situation, just > ignored completely. And that is in fact the nature and purpose of the > C-language || operator. ;-) I agree with Paul, without any evidence of improvement, optimizing an obvious slow path is a NAK. thanks, - Joel
On Fri, Jul 7, 2023 at 1:16 PM Joel Fernandes <joel@joelfernandes.org> wrote: > > On Fri, Jul 7, 2023 at 12:05 PM Paul E. McKenney <paulmck@kernel.org> wrote: > > > > On Fri, Jul 07, 2023 at 06:28:29PM +0800, Z qiang wrote: > > > > > > > > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > > > > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > > > > > CPU's sdp->lock will be acquired to check whether there are pending > > > > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > > > > > probabilistically probe global state to decide whether to convert to > > > > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > > > > > kernels and after the rcu_set_runtime_mode() is called, invoke the > > > > > rcu_gp_is_normal() is always return true, this mean that invoke the > > > > > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > > > > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > > > > > probe global state in srcu_might_be_idle(). > > > > > > > > > > This commit therefore make srcu_might_be_idle() return immediately if the > > > > > rcu_gp_is_normal() return true. > > > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > > --- > > > > > kernel/rcu/srcutree.c | 2 ++ > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > > > index 20d7a238d675..aea49cb60a45 100644 > > > > > --- a/kernel/rcu/srcutree.c > > > > > +++ b/kernel/rcu/srcutree.c > > > > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > > > > unsigned long tlast; > > > > > > > > > > check_init_srcu_struct(ssp); > > > > > + if (rcu_gp_is_normal()) > > > > > + return false; > > > > > > > > Again, thank you for looking into SRCU! > > > > > > > > I am not at all enthusiastic about this one. With this change, the name > > > > srcu_might_be_idle() is no longer accurate. Yes, the name could change, > > > > but any name would be longer and more confusing. > > > > > > > > So unless there is a measureable benefit to this one on a production > > > > workload, I cannot justify taking it. > > > > > > > > Is there a measureable benefit? > > > > > > Hi, Paul > > > > > > I only find that for Preempt-RT kernel, the rcu_normal_after_boot is > > > set by default: > > > static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT); > > > This affects only rcu but also srcu, this make the synchronize_srcu() and > > > synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true), > > > this means that call the srcu_might_be_idle() is meaningless. > > > > I do understand that the current setup favors default kernel builds at > > runtime by a few low-cost instructions, and that your change favors, > > as you say, kernels built for real-time, kernels built for certain types > > of HPC workloads, and all kernels during a small time during boot. > > > > My question is instead whether any of this makes a measureable difference > > at the system level. > > > > My guess is "no, not even close", but the way to convince me otherwise > > would be to actually run the workload and kernels on real hardware and > > provide measurements showing a statistically significant difference that > > the workload(s) in question care(s) about. > > > > So what can you show me? > > > > And srcu_might_be_idle() is not meaningless in that situation, just > > ignored completely. And that is in fact the nature and purpose of the > > C-language || operator. ;-) > > I agree with Paul, without any evidence of improvement, optimizing an > obvious slow path is a NAK. Just to clarify, when I meant improvement I meant any kind (ex. better for maintenance, better performance numbers etc.). In this case, the extra 2 lines does not seem to buy much AFAICS. Thanks.
> > On Fri, Jul 07, 2023 at 06:28:29PM +0800, Z qiang wrote: > > > > > > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > > > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > > > > CPU's sdp->lock will be acquired to check whether there are pending > > > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > > > > probabilistically probe global state to decide whether to convert to > > > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > > > > kernels and after the rcu_set_runtime_mode() is called, invoke the > > > > rcu_gp_is_normal() is always return true, this mean that invoke the > > > > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > > > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > > > > probe global state in srcu_might_be_idle(). > > > > > > > > This commit therefore make srcu_might_be_idle() return immediately if the > > > > rcu_gp_is_normal() return true. > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > --- > > > > kernel/rcu/srcutree.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > > index 20d7a238d675..aea49cb60a45 100644 > > > > --- a/kernel/rcu/srcutree.c > > > > +++ b/kernel/rcu/srcutree.c > > > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > > > unsigned long tlast; > > > > > > > > check_init_srcu_struct(ssp); > > > > + if (rcu_gp_is_normal()) > > > > + return false; > > > > > > Again, thank you for looking into SRCU! > > > > > > I am not at all enthusiastic about this one. With this change, the name > > > srcu_might_be_idle() is no longer accurate. Yes, the name could change, > > > but any name would be longer and more confusing. > > > > > > So unless there is a measureable benefit to this one on a production > > > workload, I cannot justify taking it. > > > > > > Is there a measureable benefit? > > > > Hi, Paul > > > > I only find that for Preempt-RT kernel, the rcu_normal_after_boot is > > set by default: > > static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT); > > This affects only rcu but also srcu, this make the synchronize_srcu() and > > synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true), > > this means that call the srcu_might_be_idle() is meaningless. > > I do understand that the current setup favors default kernel builds at > runtime by a few low-cost instructions, and that your change favors, > as you say, kernels built for real-time, kernels built for certain types > of HPC workloads, and all kernels during a small time during boot. > > My question is instead whether any of this makes a measureable difference > at the system level. > > My guess is "no, not even close", but the way to convince me otherwise > would be to actually run the workload and kernels on real hardware and > provide measurements showing a statistically significant difference that > the workload(s) in question care(s) about. > > So what can you show me? > > And srcu_might_be_idle() is not meaningless in that situation, just > ignored completely. And that is in fact the nature and purpose of the > C-language || operator. ;-) > Agree with you :) This make me want to ask another question, why srcu also use rcupdate.rcu_normal and rcupdate.rcu_expedited to decide expedited srcu grace-period or only use normal grace-period instead of generating srcu_normal and srcu_expedited? Thanks Zqiang > > Thanx, Paul > > > Thanks > > Zqiang > > > > > > > > Thanx, Paul > > > > > > > /* If the local srcu_data structure has callbacks, not idle. */ > > > > sdp = raw_cpu_ptr(ssp->sda); > > > > spin_lock_irqsave_rcu_node(sdp, flags); > > > > -- > > > > 2.17.1 > > > >
On Sat, Jul 8, 2023 at 1:18 AM Joel Fernandes <joel@joelfernandes.org> wrote: > > On Fri, Jul 7, 2023 at 1:16 PM Joel Fernandes <joel@joelfernandes.org> wrote: > > > > On Fri, Jul 7, 2023 at 12:05 PM Paul E. McKenney <paulmck@kernel.org> wrote: > > > > > > On Fri, Jul 07, 2023 at 06:28:29PM +0800, Z qiang wrote: > > > > > > > > > > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > > > > > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > > > > > > CPU's sdp->lock will be acquired to check whether there are pending > > > > > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > > > > > > probabilistically probe global state to decide whether to convert to > > > > > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > > > > > > kernels and after the rcu_set_runtime_mode() is called, invoke the > > > > > > rcu_gp_is_normal() is always return true, this mean that invoke the > > > > > > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > > > > > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > > > > > > probe global state in srcu_might_be_idle(). > > > > > > > > > > > > This commit therefore make srcu_might_be_idle() return immediately if the > > > > > > rcu_gp_is_normal() return true. > > > > > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > > > --- > > > > > > kernel/rcu/srcutree.c | 2 ++ > > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > > > > index 20d7a238d675..aea49cb60a45 100644 > > > > > > --- a/kernel/rcu/srcutree.c > > > > > > +++ b/kernel/rcu/srcutree.c > > > > > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > > > > > unsigned long tlast; > > > > > > > > > > > > check_init_srcu_struct(ssp); > > > > > > + if (rcu_gp_is_normal()) > > > > > > + return false; > > > > > > > > > > Again, thank you for looking into SRCU! > > > > > > > > > > I am not at all enthusiastic about this one. With this change, the name > > > > > srcu_might_be_idle() is no longer accurate. Yes, the name could change, > > > > > but any name would be longer and more confusing. > > > > > > > > > > So unless there is a measureable benefit to this one on a production > > > > > workload, I cannot justify taking it. > > > > > > > > > > Is there a measureable benefit? > > > > > > > > Hi, Paul > > > > > > > > I only find that for Preempt-RT kernel, the rcu_normal_after_boot is > > > > set by default: > > > > static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT); > > > > This affects only rcu but also srcu, this make the synchronize_srcu() and > > > > synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true), > > > > this means that call the srcu_might_be_idle() is meaningless. > > > > > > I do understand that the current setup favors default kernel builds at > > > runtime by a few low-cost instructions, and that your change favors, > > > as you say, kernels built for real-time, kernels built for certain types > > > of HPC workloads, and all kernels during a small time during boot. > > > > > > My question is instead whether any of this makes a measureable difference > > > at the system level. > > > > > > My guess is "no, not even close", but the way to convince me otherwise > > > would be to actually run the workload and kernels on real hardware and > > > provide measurements showing a statistically significant difference that > > > the workload(s) in question care(s) about. > > > > > > So what can you show me? > > > > > > And srcu_might_be_idle() is not meaningless in that situation, just > > > ignored completely. And that is in fact the nature and purpose of the > > > C-language || operator. ;-) > > > > I agree with Paul, without any evidence of improvement, optimizing an > > obvious slow path is a NAK. > > Just to clarify, when I meant improvement I meant any kind (ex. better > for maintenance, better performance numbers etc.). In this case, the > extra 2 lines does not seem to buy much AFAICS. > Agree, optimization does require performance data. Thanks Zqiang > > Thanks.
On Sat, Jul 08, 2023 at 10:11:30AM +0800, Z qiang wrote: > > > > On Fri, Jul 07, 2023 at 06:28:29PM +0800, Z qiang wrote: > > > > > > > > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > > > > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > > > > > CPU's sdp->lock will be acquired to check whether there are pending > > > > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > > > > > probabilistically probe global state to decide whether to convert to > > > > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > > > > > kernels and after the rcu_set_runtime_mode() is called, invoke the > > > > > rcu_gp_is_normal() is always return true, this mean that invoke the > > > > > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > > > > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > > > > > probe global state in srcu_might_be_idle(). > > > > > > > > > > This commit therefore make srcu_might_be_idle() return immediately if the > > > > > rcu_gp_is_normal() return true. > > > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > > --- > > > > > kernel/rcu/srcutree.c | 2 ++ > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > > > index 20d7a238d675..aea49cb60a45 100644 > > > > > --- a/kernel/rcu/srcutree.c > > > > > +++ b/kernel/rcu/srcutree.c > > > > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > > > > unsigned long tlast; > > > > > > > > > > check_init_srcu_struct(ssp); > > > > > + if (rcu_gp_is_normal()) > > > > > + return false; > > > > > > > > Again, thank you for looking into SRCU! > > > > > > > > I am not at all enthusiastic about this one. With this change, the name > > > > srcu_might_be_idle() is no longer accurate. Yes, the name could change, > > > > but any name would be longer and more confusing. > > > > > > > > So unless there is a measureable benefit to this one on a production > > > > workload, I cannot justify taking it. > > > > > > > > Is there a measureable benefit? > > > > > > Hi, Paul > > > > > > I only find that for Preempt-RT kernel, the rcu_normal_after_boot is > > > set by default: > > > static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT); > > > This affects only rcu but also srcu, this make the synchronize_srcu() and > > > synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true), > > > this means that call the srcu_might_be_idle() is meaningless. > > > > I do understand that the current setup favors default kernel builds at > > runtime by a few low-cost instructions, and that your change favors, > > as you say, kernels built for real-time, kernels built for certain types > > of HPC workloads, and all kernels during a small time during boot. > > > > My question is instead whether any of this makes a measureable difference > > at the system level. > > > > My guess is "no, not even close", but the way to convince me otherwise > > would be to actually run the workload and kernels on real hardware and > > provide measurements showing a statistically significant difference that > > the workload(s) in question care(s) about. > > > > So what can you show me? > > > > And srcu_might_be_idle() is not meaningless in that situation, just > > ignored completely. And that is in fact the nature and purpose of the > > C-language || operator. ;-) > > Agree with you :) > This make me want to ask another question, why srcu also use > rcupdate.rcu_normal and rcupdate.rcu_expedited to decide expedited > srcu grace-period or only use normal grace-period instead of > generating srcu_normal and srcu_expedited? Because I have not yet come across a situation where it was useful for the one to be expedited and the other normal. But if you know of such a situation, let's talk about it. Thanx, Paul > Thanks > Zqiang > > > > > Thanx, Paul > > > > > Thanks > > > Zqiang > > > > > > > > > > > Thanx, Paul > > > > > > > > > /* If the local srcu_data structure has callbacks, not idle. */ > > > > > sdp = raw_cpu_ptr(ssp->sda); > > > > > spin_lock_irqsave_rcu_node(sdp, flags); > > > > > -- > > > > > 2.17.1 > > > > >
> > On Sat, Jul 08, 2023 at 10:11:30AM +0800, Z qiang wrote: > > > > > > On Fri, Jul 07, 2023 at 06:28:29PM +0800, Z qiang wrote: > > > > > > > > > > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote: > > > > > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current > > > > > > CPU's sdp->lock will be acquired to check whether there are pending > > > > > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks, > > > > > > probabilistically probe global state to decide whether to convert to > > > > > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1 > > > > > > kernels and after the rcu_set_runtime_mode() is called, invoke the > > > > > > rcu_gp_is_normal() is always return true, this mean that invoke the > > > > > > synchronize_srcu_expedited() always fall back to synchronize_srcu(), > > > > > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and > > > > > > probe global state in srcu_might_be_idle(). > > > > > > > > > > > > This commit therefore make srcu_might_be_idle() return immediately if the > > > > > > rcu_gp_is_normal() return true. > > > > > > > > > > > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> > > > > > > --- > > > > > > kernel/rcu/srcutree.c | 2 ++ > > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > > > > index 20d7a238d675..aea49cb60a45 100644 > > > > > > --- a/kernel/rcu/srcutree.c > > > > > > +++ b/kernel/rcu/srcutree.c > > > > > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) > > > > > > unsigned long tlast; > > > > > > > > > > > > check_init_srcu_struct(ssp); > > > > > > + if (rcu_gp_is_normal()) > > > > > > + return false; > > > > > > > > > > Again, thank you for looking into SRCU! > > > > > > > > > > I am not at all enthusiastic about this one. With this change, the name > > > > > srcu_might_be_idle() is no longer accurate. Yes, the name could change, > > > > > but any name would be longer and more confusing. > > > > > > > > > > So unless there is a measureable benefit to this one on a production > > > > > workload, I cannot justify taking it. > > > > > > > > > > Is there a measureable benefit? > > > > > > > > Hi, Paul > > > > > > > > I only find that for Preempt-RT kernel, the rcu_normal_after_boot is > > > > set by default: > > > > static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT); > > > > This affects only rcu but also srcu, this make the synchronize_srcu() and > > > > synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true), > > > > this means that call the srcu_might_be_idle() is meaningless. > > > > > > I do understand that the current setup favors default kernel builds at > > > runtime by a few low-cost instructions, and that your change favors, > > > as you say, kernels built for real-time, kernels built for certain types > > > of HPC workloads, and all kernels during a small time during boot. > > > > > > My question is instead whether any of this makes a measureable difference > > > at the system level. > > > > > > My guess is "no, not even close", but the way to convince me otherwise > > > would be to actually run the workload and kernels on real hardware and > > > provide measurements showing a statistically significant difference that > > > the workload(s) in question care(s) about. > > > > > > So what can you show me? > > > > > > And srcu_might_be_idle() is not meaningless in that situation, just > > > ignored completely. And that is in fact the nature and purpose of the > > > C-language || operator. ;-) > > > > Agree with you :) > > This make me want to ask another question, why srcu also use > > rcupdate.rcu_normal and rcupdate.rcu_expedited to decide expedited > > srcu grace-period or only use normal grace-period instead of > > generating srcu_normal and srcu_expedited? > > Because I have not yet come across a situation where it was useful for > the one to be expedited and the other normal. > > But if you know of such a situation, let's talk about it. > Thanks, I will let you know if I come across such a situation :) . Thanks Zqiang > > Thanx, Paul > > > Thanks > > Zqiang > > > > > > > > Thanx, Paul > > > > > > > Thanks > > > > Zqiang > > > > > > > > > > > > > > Thanx, Paul > > > > > > > > > > > /* If the local srcu_data structure has callbacks, not idle. */ > > > > > > sdp = raw_cpu_ptr(ssp->sda); > > > > > > spin_lock_irqsave_rcu_node(sdp, flags); > > > > > > -- > > > > > > 2.17.1 > > > > > >
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index 20d7a238d675..aea49cb60a45 100644 --- a/kernel/rcu/srcutree.c +++ b/kernel/rcu/srcutree.c @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp) unsigned long tlast; check_init_srcu_struct(ssp); + if (rcu_gp_is_normal()) + return false; /* If the local srcu_data structure has callbacks, not idle. */ sdp = raw_cpu_ptr(ssp->sda); spin_lock_irqsave_rcu_node(sdp, flags);