From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id F29CF17BA2 for ; Fri, 10 May 2024 17:13:02 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1715361185; cv=none; b=ppDuaTgtOx9VgwQoNRXuxkzGCtlVnHd2I2FNj2PUswlxlEZgTlBlDMaT26S6YZZjAhK/WaMVVQoUv2cE29Nz8y2S1v+Hv9vVE2UbYENyO1355ZD2BAlI9IWa2qLUHWasy2Ebclbn/cR244hSPpiDeqUsq9mNwFBNNNX4QVnVpPY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1715361185; c=relaxed/simple; bh=2h6DKRcRtBL7zVSlykpWFNs7GUlXVarb9mvkr0voyY0=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version:Content-Type:Content-Disposition; b=gPNvUc7LXjr048aytzbMhkXR23C3cbK8ybUfjAHyGXO238IBDko7JQDGLoy/AgPPvLSUBP7JBvuTuop4a/613ZQlIMW3rvXGbwLVrY2FA4Saz5zSKwCRlOtHYQLaKXi5ZqxVU3fOVJ+0+hYgTkNCM7NRMFJrGl2GUd/gi9iuaR4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=CWRwN+BP; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="CWRwN+BP" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1715361182; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=bioYp2CSl0AYue25xlAuG+WOXfEEJvx+bApG9LmRDtE=; b=CWRwN+BPO/tZ97qcGN2OSWg5CbLlpPbYPqZsGdvMJKbnE6q185/WwVoJAzk5EqcG/eQpii JzWsEIaLs6ele3jcRUwrgL/6hyVuaXWFR00gdPmG/O+KOz0S6MyI+5n+sAkh2UCYrv6SPO VFmqZpfNgn20W5DNEULUxZ/Cd/QewVs= Received: from mail-pl1-f198.google.com (mail-pl1-f198.google.com [209.85.214.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-552-Wez_xTElOKqp2X2JRHqY6w-1; Fri, 10 May 2024 13:12:59 -0400 X-MC-Unique: Wez_xTElOKqp2X2JRHqY6w-1 Received: by mail-pl1-f198.google.com with SMTP id d9443c01a7336-1ed8876c40eso21963565ad.1 for ; Fri, 10 May 2024 10:12:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715361178; x=1715965978; h=content-transfer-encoding:content-disposition:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=bioYp2CSl0AYue25xlAuG+WOXfEEJvx+bApG9LmRDtE=; b=c5sN7oZCiN9n0jcVc46N98hs8xcNNT/NKObaYGqLymcpYAMThnHxWr3KfvKI1o+kRa bYK50g7TA6KDiwd8kK17Xie1oFnaNzruwChXMhQfpitcl3BziYHGiUWo0wrxC5uN7Fky oH5L/m/Fp67p1zmHCVzuiTu60ONeqWDnBGiKhyikfrT5GvQzQSeITG1UAkBt4sSSqWNP bpshmtecuG81LmPq19JwxnS94PvnxskqXBtThw3djEZ6UA/KgBCb81qD33/Lo0b8erls jX3JnZ/rimXAgbfiIiAWN/0S+gHuX2dGZBAQVlhzT+tL6nVOdjm6HQELHKh34ek4Hgto lwCA== X-Forwarded-Encrypted: i=1; AJvYcCXkhd3wNuyJpX0+zNT5wFpz4TlTl0hkNlKEXlQSc0lx4ivAUHDrUfCxElHekqIgc6LLQ6Te5HtLVjUzxDypE5WlmndzIpn7MtAfRnRb X-Gm-Message-State: AOJu0YyKE85hdP1VPExQ+BkWFVlknqaxdvc8M828EXMRAaJbnMY6MhLr xvwamX0lf/2KVoBCapjUd0/tkSBGUxfprmoDMPZuwB285LN+EG2F/pwlM3Fv27NuRXAXEqGRNT1 SDM2L8GMUJQ6j5FVV2YB7XDU4KLT2tfiNb3T1FX4vC98hYRg8XeHOnunDVJGJjA== X-Received: by 2002:a17:902:d2c4:b0:1ec:304e:90c4 with SMTP id d9443c01a7336-1ef43d1407dmr39683345ad.17.1715361178088; Fri, 10 May 2024 10:12:58 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHn7opyRHUSzAXPKEz11pCvggeXdxBM3sqhSChQBiV+vg8ayLkITiqeP+ZPasl6cd3thKf0Eg== X-Received: by 2002:a17:902:d2c4:b0:1ec:304e:90c4 with SMTP id d9443c01a7336-1ef43d1407dmr39682865ad.17.1715361177512; Fri, 10 May 2024 10:12:57 -0700 (PDT) Received: from localhost.localdomain ([2804:1b3:a800:8d87:eac1:dae4:8dd4:fe50]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-1ef0bad62fesm34767835ad.83.2024.05.10.10.12.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 May 2024 10:12:56 -0700 (PDT) From: Leonardo Bras To: "Paul E. McKenney" Cc: Leonardo Bras , Sean Christopherson , Paolo Bonzini , Frederic Weisbecker , Neeraj Upadhyay , Joel Fernandes , Josh Triplett , Boqun Feng , Steven Rostedt , Mathieu Desnoyers , Lai Jiangshan , Zqiang , Marcelo Tosatti , kvm@vger.kernel.org, linux-kernel@vger.kernel.org, rcu@vger.kernel.org Subject: Re: [RFC PATCH v1 0/2] Avoid rcu_core() if CPU just left guest vcpu Date: Fri, 10 May 2024 14:12:32 -0300 Message-ID: X-Mailer: git-send-email 2.45.0 In-Reply-To: References: <5fd66909-1250-4a91-aa71-93cb36ed4ad5@paulmck-laptop> <09a8f4f6-a692-4586-bb68-b0a524b7a5d8@paulmck-laptop> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: 8bit On Fri, May 10, 2024 at 09:21:59AM -0700, Paul E. McKenney wrote: > On Fri, May 10, 2024 at 01:06:40PM -0300, Leonardo Bras wrote: > > On Thu, May 09, 2024 at 04:45:53PM -0700, Paul E. McKenney wrote: > > > On Thu, May 09, 2024 at 07:14:18AM -0300, Leonardo Bras wrote: > > > > On Thu, May 09, 2024 at 05:16:57AM -0300, Leonardo Bras wrote: > > > > > > [ . . . ] > > > > > > > > Here I suppose something like this can take care of not needing to convert > > > > > ms -> jiffies every rcu_pending(): > > > > > > > > > > + nocb_patience_delay = msecs_to_jiffies(nocb_patience_delay); > > > > > > > > > > > > > Uh, there is more to it, actually. We need to make sure the user > > > > understands that we are rounding-down the value to multiple of a jiffy > > > > period, so it's not a surprise if the delay value is not exactly the same > > > > as the passed on kernel cmdline. > > > > > > > > So something like bellow diff should be ok, as this behavior is explained > > > > in the docs, and pr_info() will print the effective value. > > > > > > > > What do you think? > > > > > > Good point, and I have taken your advice on making the documentation > > > say what it does. > > > > Thanks :) > > > > > > > > > Thanks! > > > > Leo > > > > > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > > > index 0a3b0fd1910e..9a50be9fd9eb 100644 > > > > --- a/Documentation/admin-guide/kernel-parameters.txt > > > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > > > @@ -4974,20 +4974,28 @@ > > > > otherwise be caused by callback floods through > > > > use of the ->nocb_bypass list. However, in the > > > > common non-flooded case, RCU queues directly to > > > > the main ->cblist in order to avoid the extra > > > > overhead of the ->nocb_bypass list and its lock. > > > > But if there are too many callbacks queued during > > > > a single jiffy, RCU pre-queues the callbacks into > > > > the ->nocb_bypass queue. The definition of "too > > > > many" is supplied by this kernel boot parameter. > > > > > > > > + rcutree.nocb_patience_delay= [KNL] > > > > + On callback-offloaded (rcu_nocbs) CPUs, avoid > > > > + disturbing RCU unless the grace period has > > > > + reached the specified age in milliseconds. > > > > + Defaults to zero. Large values will be capped > > > > + at five seconds. Values rounded-down to a multiple > > > > + of a jiffy period. > > > > + > > > > rcutree.qhimark= [KNL] > > > > Set threshold of queued RCU callbacks beyond which > > > > batch limiting is disabled. > > > > > > > > rcutree.qlowmark= [KNL] > > > > Set threshold of queued RCU callbacks below which > > > > batch limiting is re-enabled. > > > > > > > > rcutree.qovld= [KNL] > > > > Set threshold of queued RCU callbacks beyond which > > > > diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h > > > > index fcf2b4aa3441..62ede401420f 100644 > > > > --- a/kernel/rcu/tree.h > > > > +++ b/kernel/rcu/tree.h > > > > @@ -512,20 +512,21 @@ do { \ > > > > local_irq_save(flags); \ > > > > if (rcu_segcblist_is_offloaded(&(rdp)->cblist)) \ > > > > raw_spin_lock(&(rdp)->nocb_lock); \ > > > > } while (0) > > > > #else /* #ifdef CONFIG_RCU_NOCB_CPU */ > > > > #define rcu_nocb_lock_irqsave(rdp, flags) local_irq_save(flags) > > > > #endif /* #else #ifdef CONFIG_RCU_NOCB_CPU */ > > > > > > > > static void rcu_bind_gp_kthread(void); > > > > static bool rcu_nohz_full_cpu(void); > > > > +static bool rcu_on_patience_delay(void); > > > > > > I don't think we need an access function, but will check below. > > > > > > > /* Forward declarations for tree_stall.h */ > > > > static void record_gp_stall_check_time(void); > > > > static void rcu_iw_handler(struct irq_work *iwp); > > > > static void check_cpu_stall(struct rcu_data *rdp); > > > > static void rcu_check_gp_start_stall(struct rcu_node *rnp, struct rcu_data *rdp, > > > > const unsigned long gpssdelay); > > > > > > > > /* Forward declarations for tree_exp.h. */ > > > > static void sync_rcu_do_polled_gp(struct work_struct *wp); > > > > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h > > > > index 340bbefe5f65..639243b0410f 100644 > > > > --- a/kernel/rcu/tree_plugin.h > > > > +++ b/kernel/rcu/tree_plugin.h > > > > @@ -5,20 +5,21 @@ > > > > * or preemptible semantics. > > > > * > > > > * Copyright Red Hat, 2009 > > > > * Copyright IBM Corporation, 2009 > > > > * > > > > * Author: Ingo Molnar > > > > * Paul E. McKenney > > > > */ > > > > > > > > #include "../locking/rtmutex_common.h" > > > > +#include > > > > > > This is already pulled in by the enclosing tree.c file, so it should not > > > be necessary to include it again. > > > > Even better :) > > > > > (Or did you get a build failure when > > > leaving this out?) > > > > I didn't, it's just that my editor complained the symbols were not getting > > properly resolved, so I included it and it was fixed. But since clangd is > > know to make some mistakes, I should have compile-test'd before adding it. > > Ah, got it! ;-) > > > > > static bool rcu_rdp_is_offloaded(struct rcu_data *rdp) > > > > { > > > > /* > > > > * In order to read the offloaded state of an rdp in a safe > > > > * and stable way and prevent from its value to be changed > > > > * under us, we must either hold the barrier mutex, the cpu > > > > * hotplug lock (read or write) or the nocb lock. Local > > > > * non-preemptible reads are also safe. NOCB kthreads and > > > > * timers have their own means of synchronization against the > > > > @@ -86,20 +87,33 @@ static void __init rcu_bootup_announce_oddness(void) > > > > if (rcu_kick_kthreads) > > > > pr_info("\tKick kthreads if too-long grace period.\n"); > > > > if (IS_ENABLED(CONFIG_DEBUG_OBJECTS_RCU_HEAD)) > > > > pr_info("\tRCU callback double-/use-after-free debug is enabled.\n"); > > > > if (gp_preinit_delay) > > > > pr_info("\tRCU debug GP pre-init slowdown %d jiffies.\n", gp_preinit_delay); > > > > if (gp_init_delay) > > > > pr_info("\tRCU debug GP init slowdown %d jiffies.\n", gp_init_delay); > > > > if (gp_cleanup_delay) > > > > pr_info("\tRCU debug GP cleanup slowdown %d jiffies.\n", gp_cleanup_delay); > > > > + if (nocb_patience_delay < 0) { > > > > + pr_info("\tRCU NOCB CPU patience negative (%d), resetting to zero.\n", > > > > + nocb_patience_delay); > > > > + nocb_patience_delay = 0; > > > > + } else if (nocb_patience_delay > 5 * MSEC_PER_SEC) { > > > > + pr_info("\tRCU NOCB CPU patience too large (%d), resetting to %ld.\n", > > > > + nocb_patience_delay, 5 * MSEC_PER_SEC); > > > > + nocb_patience_delay = msecs_to_jiffies(5 * MSEC_PER_SEC); > > > > + } else if (nocb_patience_delay) { > > > > + nocb_patience_delay = msecs_to_jiffies(nocb_patience_delay); > > > > + pr_info("\tRCU NOCB CPU patience set to %d milliseconds.\n", > > > > + jiffies_to_msecs(nocb_patience_delay); > > > > + } > > > > > > I just did this here at the end: > > > > > > nocb_patience_delay_jiffies = msecs_to_jiffies(nocb_patience_delay); > > > > > > Ah, you are wanting to print out the milliseconds after the rounding > > > to jiffies. > > > > That's right, just to make sure the user gets the effective patience time, > > instead of the before-rounding one, which was on input. > > > > > I am going to hold off on that for the moment, but I hear your request > > > and I have not yet said "no". ;-) > > > > Sure :) > > It's just something I think it's nice to have (as a user). > > If you would like to do a separate patch adding this, here are the > requirements: > > o If the current code prints nothing, nothing additional should > be printed. > > o If the rounding ended up with the same value (as it should in > systems with HZ=1000), nothing additional should be printed. > > o Your choice as to whether or not you want to print out the > jiffies value. > > o If the additional message is on a new line, it needs to be > indented so that it is clear that it is subordinate to the > previous message. > > Otherwise, you can use pr_cont() to continue the previous > line, of course being careful about "\n". > > Probably also something that I am forgetting, but that is most of it. > Thanks! I will work on a patch doing that :) > > > > if (!use_softirq) > > > > pr_info("\tRCU_SOFTIRQ processing moved to rcuc kthreads.\n"); > > > > if (IS_ENABLED(CONFIG_RCU_EQS_DEBUG)) > > > > pr_info("\tRCU debug extended QS entry/exit.\n"); > > > > rcupdate_announce_bootup_oddness(); > > > > } > > > > > > > > #ifdef CONFIG_PREEMPT_RCU > > > > > > > > static void rcu_report_exp_rnp(struct rcu_node *rnp, bool wake); > > > > @@ -1260,10 +1274,29 @@ static bool rcu_nohz_full_cpu(void) > > > > > > > > /* > > > > * Bind the RCU grace-period kthreads to the housekeeping CPU. > > > > */ > > > > static void rcu_bind_gp_kthread(void) > > > > { > > > > if (!tick_nohz_full_enabled()) > > > > return; > > > > housekeeping_affine(current, HK_TYPE_RCU); > > > > } > > > > + > > > > +/* > > > > + * Is this CPU a NO_HZ_FULL CPU that should ignore RCU if the time since the > > > > + * start of current grace period is smaller than nocb_patience_delay ? > > > > + * > > > > + * This code relies on the fact that all NO_HZ_FULL CPUs are also > > > > + * RCU_NOCB_CPU CPUs. > > > > + */ > > > > +static bool rcu_on_patience_delay(void) > > > > +{ > > > > +#ifdef CONFIG_NO_HZ_FULL > > > > > > You lost me on this one. Why do we need the #ifdef instead of > > > IS_ENABLED()? Also, please note that rcu_nohz_full_cpu() is already a > > > compile-time @false in CONFIG_NO_HZ_FULL=n kernels. > > > > You are right. rcu_nohz_full_cpu() has a high chance of being inlined on > > if ((...) && rcu_nohz_full_cpu()) > > And since it returns false, this whole statement will be compiled out, and > > the new function will not exist in CONFIG_NO_HZ_FULL=n, so there is no > > need to test it. > > Very good! You had me going there for a bit. ;-) > > > > > + if (!nocb_patience_delay) > > > > + return false; > > > > > > We get this automatically with the comparison below, right? > > > > Right > > > > > If so, we > > > are not gaining much by creating the helper function. Or am I missing > > > some trick here? > > > > Well, it's a fastpath. Up to here, we just need to read > > nocb_patience_delay{,_jiffies} from memory. > > Just nocb_patience_delay_jiffies, correct? Unless I am missing something, > nocb_patience_delay is unused after boot. Right, I used both because I was referring to the older version and the current version with _jiffies. > > > If we don't include the fastpath we have to read jiffies and > > rcu_state.gp_start, which can take extra time: up to 2 cache misses. > > > > I thought it could be relevant, as we reduce the overhead of the new > > parameter when it's disabled (patience=0). > > > > Do you think that could be relevant? > > Well, the hardware's opinion is what matters. ;-) > > But the caller's code path reads jiffies a few times, so it should > be hot in the cache, correct? Right, but I wonder how are the chances of it getting updated between caller's use and this function's. Same for gp_start. > > But that does lead to another topic, namely the possibility of tagging > nocb_patience_delay_jiffies with __read_mostly. Oh, right. This was supposed to be in the diff I sent earlier, but I completelly forgot to change before sending. So, yeah, I agree on nocb_patience_delay being __read_mostly; > And there might be > a number of other of RCU's variables that could be similarly tagged > in order to avoid false sharing. (But is there any false sharing? > This might be worth testing.) Maybe there isn't, but I wonder if it would hurt performance if they were tagged as __read_only anyway. Thanks! Leo > > Thanx, Paul > > > Thanks! > > Leo > > > > > > > > Thanx, Paul > > > > > > > + if (time_before(jiffies, READ_ONCE(rcu_state.gp_start) + nocb_patience_delay)) > > > > + return true; > > > > +#endif /* #ifdef CONFIG_NO_HZ_FULL */ > > > > + return false; > > > > +} > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > > index 7560e204198b..7a2d94370ab4 100644 > > > > --- a/kernel/rcu/tree.c > > > > +++ b/kernel/rcu/tree.c > > > > @@ -169,20 +169,22 @@ static int kthread_prio = IS_ENABLED(CONFIG_RCU_BOOST) ? 1 : 0; > > > > module_param(kthread_prio, int, 0444); > > > > > > > > /* Delay in jiffies for grace-period initialization delays, debug only. */ > > > > > > > > static int gp_preinit_delay; > > > > module_param(gp_preinit_delay, int, 0444); > > > > static int gp_init_delay; > > > > module_param(gp_init_delay, int, 0444); > > > > static int gp_cleanup_delay; > > > > module_param(gp_cleanup_delay, int, 0444); > > > > +static int nocb_patience_delay; > > > > +module_param(nocb_patience_delay, int, 0444); > > > > > > > > // Add delay to rcu_read_unlock() for strict grace periods. > > > > static int rcu_unlock_delay; > > > > #ifdef CONFIG_RCU_STRICT_GRACE_PERIOD > > > > module_param(rcu_unlock_delay, int, 0444); > > > > #endif > > > > > > > > /* > > > > * This rcu parameter is runtime-read-only. It reflects > > > > * a minimum allowed number of objects which can be cached > > > > @@ -4340,25 +4342,27 @@ static int rcu_pending(int user) > > > > lockdep_assert_irqs_disabled(); > > > > > > > > /* Check for CPU stalls, if enabled. */ > > > > check_cpu_stall(rdp); > > > > > > > > /* Does this CPU need a deferred NOCB wakeup? */ > > > > if (rcu_nocb_need_deferred_wakeup(rdp, RCU_NOCB_WAKE)) > > > > return 1; > > > > > > > > /* Is this a nohz_full CPU in userspace or idle? (Ignore RCU if so.) */ > > > > - if ((user || rcu_is_cpu_rrupt_from_idle()) && rcu_nohz_full_cpu()) > > > > + gp_in_progress = rcu_gp_in_progress(); > > > > + if ((user || rcu_is_cpu_rrupt_from_idle() || > > > > + (gp_in_progress && rcu_on_patience_delay())) && > > > > + rcu_nohz_full_cpu()) > > > > return 0; > > > > > > > > /* Is the RCU core waiting for a quiescent state from this CPU? */ > > > > - gp_in_progress = rcu_gp_in_progress(); > > > > if (rdp->core_needs_qs && !rdp->cpu_no_qs.b.norm && gp_in_progress) > > > > return 1; > > > > > > > > /* Does this CPU have callbacks ready to invoke? */ > > > > if (!rcu_rdp_is_offloaded(rdp) && > > > > rcu_segcblist_ready_cbs(&rdp->cblist)) > > > > return 1; > > > > > > > > /* Has RCU gone idle with this CPU needing another grace period? */ > > > > if (!gp_in_progress && rcu_segcblist_is_enabled(&rdp->cblist) && > > > > > > > > > > > > > > > > > > > >