* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Sergey Senozhatsky @ 2019-08-29 0:27 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sergey Senozhatsky, Rasmus Villemoes, Jani Nikula, Petr Mladek,
Steven Rostedt, Andrew Morton, Enrico Weigelt, metux IT consult,
Jonathan Corbet, linux-doc, linux-kernel
In-Reply-To: <61cd079f-d41b-75ec-9a1e-ef80f9d1f8fd@kleine-koenig.org>
On (08/28/19 18:22), Uwe Kleine-König wrote:
> That is wrong. When you do
>
> pr_err("There are no round tuits to give out: %dE\n", -ENOENT);
>
> in a kernel that doesn't support %dE you get:
>
> There are no round tuits to give out: -2E
OK. Good point.
-ss
^ permalink raw reply
* Re: [PATCH 01/11] ftrace: move recordmcount tools to scripts/ftrace
From: Changbin Du @ 2019-08-28 23:41 UTC (permalink / raw)
To: Steven Rostedt
Cc: Changbin Du, Ingo Molnar, Jonathan Corbet, Jessica Yu,
Thomas Gleixner, x86, linux-doc, linux-kernel, linux-arm-kernel,
linux-mips, linux-parisc, linuxppc-dev, linux-riscv, linux-s390,
linux-sh, sparclinux, linux-arch, linux-kbuild, John F . Reiser,
Matt Helsley
In-Reply-To: <20190826184444.09334ae9@gandalf.local.home>
On Mon, Aug 26, 2019 at 06:44:44PM -0400, Steven Rostedt wrote:
> On Sun, 25 Aug 2019 21:23:20 +0800
> Changbin Du <changbin.du@gmail.com> wrote:
>
> > Move ftrace tools to its own directory. We will add another tool later.
> >
> > Cc: John F. Reiser <jreiser@BitWagon.com>
> > Signed-off-by: Changbin Du <changbin.du@gmail.com>
> > ---
> > scripts/.gitignore | 1 -
> > scripts/Makefile | 2 +-
> > scripts/Makefile.build | 10 +++++-----
> > scripts/ftrace/.gitignore | 4 ++++
> > scripts/ftrace/Makefile | 4 ++++
> > scripts/{ => ftrace}/recordmcount.c | 0
> > scripts/{ => ftrace}/recordmcount.h | 0
> > scripts/{ => ftrace}/recordmcount.pl | 0
> > 8 files changed, 14 insertions(+), 7 deletions(-)
> > create mode 100644 scripts/ftrace/.gitignore
> > create mode 100644 scripts/ftrace/Makefile
> > rename scripts/{ => ftrace}/recordmcount.c (100%)
> > rename scripts/{ => ftrace}/recordmcount.h (100%)
> > rename scripts/{ => ftrace}/recordmcount.pl (100%)
> > mode change 100755 => 100644
>
> Note, we are in the process of merging recordmcount with objtool. It
> would be better to continue from that work.
>
> http://lkml.kernel.org/r/2767f55f4a5fbf30ba0635aed7a9c5ee92ac07dd.1563992889.git.mhelsley@vmware.com
>
> -- Steve
Thanks for reminding. Let me check if prototype tool can merge into
objtool easily after above work.
--
Cheers,
Changbin Du
^ permalink raw reply
* Re: [RFC v1 2/2] rcu/tree: Remove dynticks_nmi_nesting counter
From: Paul E. McKenney @ 2019-08-28 23:12 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Frederic Weisbecker, Jonathan Corbet, Josh Triplett,
kernel-team, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
Mauro Carvalho Chehab, rcu, Steven Rostedt
In-Reply-To: <20190828221444.GA100789@google.com>
On Wed, Aug 28, 2019 at 06:14:44PM -0400, Joel Fernandes wrote:
> On Wed, Aug 28, 2019 at 03:01:08PM -0700, Paul E. McKenney wrote:
> > On Wed, Aug 28, 2019 at 05:42:41PM -0400, Joel Fernandes wrote:
> > > On Wed, Aug 28, 2019 at 02:19:04PM -0700, Paul E. McKenney wrote:
> > > > On Wed, Aug 28, 2019 at 05:05:25PM -0400, Joel Fernandes wrote:
> > > > > On Wed, Aug 28, 2019 at 01:23:30PM -0700, Paul E. McKenney wrote:
> > > > > > On Mon, Aug 26, 2019 at 09:33:54PM -0400, Joel Fernandes (Google) wrote:
> > > > > > > The dynticks_nmi_nesting counter serves 4 purposes:
> > > > > > >
> > > > > > > (a) rcu_is_cpu_rrupt_from_idle() needs to be able to detect first
> > > > > > > interrupt nesting level.
> > > > > > >
> > > > > > > (b) We need to detect half-interrupts till we are sure they're not an
> > > > > > > issue. However, change the comparison to DYNTICK_IRQ_NONIDLE with 0.
> > > > > > >
> > > > > > > (c) When a quiescent state report is needed from a nohz_full CPU.
> > > > > > > The nesting counter detects we are a first level interrupt.
> > > > > > >
> > > > > > > For (a) we can just use dyntick_nesting == 1 to determine this. Only the
> > > > > > > outermost interrupt that interrupted an RCU-idle state can set it to 1.
> > > > > > >
> > > > > > > For (b), this warning condition has not occurred for several kernel
> > > > > > > releases. But we still keep the warning but change it to use
> > > > > > > in_interrupt() instead of the nesting counter. In a later year, we can
> > > > > > > remove the warning.
> > > > > > >
> > > > > > > For (c), the nest check is not really necessary since forced_tick would
> > > > > > > have been set to true in the outermost interrupt, so the nested/NMI
> > > > > > > interrupts will check forced_tick anyway, and bail.
> > > > > >
> > > > > > Skipping the commit log and documentation for this pass.
> > > > > [snip]
> > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > > > > index 255cd6835526..1465a3e406f8 100644
> > > > > > > --- a/kernel/rcu/tree.c
> > > > > > > +++ b/kernel/rcu/tree.c
> > > > > > > @@ -81,7 +81,6 @@
> > > > > > >
> > > > > > > static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_data) = {
> > > > > > > .dynticks_nesting = 1,
> > > > > > > - .dynticks_nmi_nesting = 0,
> > > > > >
> > > > > > This should be in the previous patch, give or take naming.
> > > > >
> > > > > Done.
> > > > >
> > > > > > > .dynticks = ATOMIC_INIT(RCU_DYNTICK_CTRL_CTR),
> > > > > > > };
> > > > > > > struct rcu_state rcu_state = {
> > > > > > > @@ -392,15 +391,9 @@ static int rcu_is_cpu_rrupt_from_idle(void)
> > > > > > > /* Check for counter underflows */
> > > > > > > RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nesting) < 0,
> > > > > > > "RCU dynticks_nesting counter underflow!");
> > > > > > > - RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0,
> > > > > > > - "RCU dynticks_nmi_nesting counter underflow/zero!");
> > > > > > >
> > > > > > > - /* Are we at first interrupt nesting level? */
> > > > > > > - if (__this_cpu_read(rcu_data.dynticks_nmi_nesting) != 1)
> > > > > > > - return false;
> > > > > > > -
> > > > > > > - /* Does CPU appear to be idle from an RCU standpoint? */
> > > > > > > - return __this_cpu_read(rcu_data.dynticks_nesting) == 0;
> > > > > > > + /* Are we the outermost interrupt that arrived when RCU was idle? */
> > > > > > > + return __this_cpu_read(rcu_data.dynticks_nesting) == 1;
> > > > > > > }
> > > > > > >
> > > > > > > #define DEFAULT_RCU_BLIMIT 10 /* Maximum callbacks per rcu_do_batch ... */
> > > > > > > @@ -564,11 +557,10 @@ static void rcu_eqs_enter(bool user)
> > > > > > > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > > > > >
> > > > > > > /* Entering usermode/idle from interrupt is not handled. These would
> > > > > > > - * mean usermode upcalls or idle entry happened from interrupts. But,
> > > > > > > - * reset the counter if we warn.
> > > > > > > + * mean usermode upcalls or idle exit happened from interrupts. Remove
> > > > > > > + * the warning by 2020.
> > > > > > > */
> > > > > > > - if (WARN_ON_ONCE(rdp->dynticks_nmi_nesting != 0))
> > > > > > > - WRITE_ONCE(rdp->dynticks_nmi_nesting, 0);
> > > > > > > + WARN_ON_ONCE(in_interrupt());
> > > > > >
> > > > > > And this is a red flag. Bad things happen should some common code
> > > > > > that disables BH be invoked from the idle loop. This might not be
> > > > > > happening now, but we need to avoid this sort of constraint.
> > > > > > How about instead merging ->dyntick_nesting into the low-order bits
> > > > > > of ->dyntick_nmi_nesting?
> > > > > >
> > > > > > Yes, this assumes that we don't enter process level twice, but it should
> > > > > > be easy to add a WARN_ON() to test for that. Except that we don't have
> > > > > > to because there is already this near the end of rcu_eqs_exit():
> > > > > >
> > > > > > WARN_ON_ONCE(rdp->dynticks_nmi_nesting);
> > > > > >
> > > > > > So the low-order bit of the combined counter could indicate process-level
> > > > > > non-idle, the next three bits could be unused to make interpretation
> > > > > > of hex printouts easier, and then the rest of the bits could be used in
> > > > > > the same way as currently.
> > > > > >
> > > > > > This would allow a single read to see the full state, so that 0x1 means
> > > > > > at process level in the kernel, 0x11 is interrupt (or NMI) from process
> > > > > > level, 0x10 is interrupt/NMI from idle/user, and so on.
> > > > > >
> > > > > > What am I missing here? Why wouldn't this work, and without adding yet
> > > > > > another RCU-imposed constraint on some other subsystem?
> > > > >
> > > > > What about replacing the warning with a WARN_ON_ONCE(in_irq()), would that
> > > > > address your concern?
> > > > >
> > > > > Also, considering this warning condition is most likely never occurring as we
> > > > > know it, and we are considering deleting it soon enough, is it really worth
> > > > > reimplementing the whole mechanism with a complex bit-sharing scheme just
> > > > > because of the BH-disable condition you mentioned, which likely doesn't
> > > > > happen today? In my implementation, this is just a simple counter. I feel
> > > > > combining bits in the same counter will just introduce more complexity that
> > > > > this patch tries to address/avoid.
> > > > >
> > > > > OTOH, I also don't mind with just deleting the warning altogether if you are
> > > > > Ok with that.
> > > >
> > > > The big advantage of combining the counters is that all of the state is
> > > > explicit and visible in one place. Plus it can be accessed atomically.
> > > > And it avoids setting a time bomb for some poor guys just trying to get
> > > > their idle-loop jobs done some time in the dim distant future.
> > >
> > > I could try the approach you're suggesting but I didn't actually see an issue
> > > with the patch in its current state other than the WARN_ON_ONCE which I could
> > > change to WARN_ON_ONCE(in_irq()) to remove the concern. AFAICS, we don't
> > > detect "half soft-interrupts" in this code in anyway.
> > >
> > > I do feel the approach you're suggesting can be a follow up, these 2 patches
> > > just focus on deleting dynticks_nmi_nesting counter and we can test this
> > > approach thoroughly for a release or so.
> > >
> > > > Besides, this pair of patches already makes a large change from a
> > > > conceptual viewpoint. If we are going to make a large change, let's
> > > > get our money's worth out of that change!
> > >
> > > IMHO, most of the changes are to code comments, the actual code change is
> > > very little and is just removal of dynticks_nmi_nesting and simplification;
> > > its not really an introduction of a new mechanism.
> >
> > This change is not fixing a bug, so there is no need for an emergency fix,
> > and thus no point in additional churn. I understand that it is a bit
> > annoying to code and test something and have your friendly maintainer say
> > "sorry, wrong rocks", and the reason that I understand this is that I do
> > that to myself rather often.
>
> The motivation for me for this change is to avoid future bugs such as with
> the following patch where "== 2" did not take the force write of
> DYNTICK_IRQ_NONIDLE into account:
> https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=13c4b07593977d9288e5d0c21c89d9ba27e2ea1f
Yes, the current code does need some simplification.
> I still don't see it as pointless churn, it is also a maintenance cost in its
> current form and the simplification is worth it IMHO both from a readability,
> and maintenance stand point.
>
> I still don't see what's technically wrong with the patch. I could perhaps
> add the above "== 2" point in the patch?
I don't know of a crash or splat your patch would cause, if that is
your question. But that is also true of the current code, so the point
is simplification, not bug fixing. And from what I can see, there is an
opportunity to simplify quite a bit further. And with something like
RCU, further simplification is worth -serious- consideration.
> We could also discuss f2f at LPC to see if we can agree about it?
That might make a lot of sense.
In the meantime, could you please create an independent series (or
more than one series, as the case may be) of the other patches?
Thanx, Paul
^ permalink raw reply
* Re: [PATCH] rcu/dyntick-idle: Add better tracing
From: Paul E. McKenney @ 2019-08-28 23:05 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Frederic Weisbecker, Jonathan Corbet, Josh Triplett,
kernel-team, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
Mauro Carvalho Chehab, rcu, Steven Rostedt
In-Reply-To: <20190828182613.37715-1-joel@joelfernandes.org>
On Wed, Aug 28, 2019 at 02:26:13PM -0400, Joel Fernandes (Google) wrote:
> The dyntick-idle traces are a bit confusing. This patch makes it simpler
> and adds some missing cases such as EQS-enter because user vs idle mode.
>
> Following are the changes:
> (1) Add a new context field to trace_rcu_dyntick tracepoint. This
> context field can be "USER", "IDLE" or "IRQ".
>
> (2) Remove the "++=" and "--=" strings and replace them with
> "StillNonIdle". This is much easier on the eyes, and the -- and ++
> are easily apparent in the dynticks_nesting counters we are printing
> anyway.
>
> This patch is based on the previous patches to simplify rcu_dyntick
> counters [1] and with these traces, I have verified the counters are
> working properly.
>
> [1]
> Link: https://lore.kernel.org/patchwork/patch/1120021/
> Link: https://lore.kernel.org/patchwork/patch/1120022/
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Looks plausible to me.
Thanx, Paul
> ---
> include/trace/events/rcu.h | 13 ++++++++-----
> kernel/rcu/tree.c | 17 +++++++++++------
> 2 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/include/trace/events/rcu.h b/include/trace/events/rcu.h
> index 66122602bd08..474c1f7e7104 100644
> --- a/include/trace/events/rcu.h
> +++ b/include/trace/events/rcu.h
> @@ -449,12 +449,14 @@ TRACE_EVENT_RCU(rcu_fqs,
> */
> TRACE_EVENT_RCU(rcu_dyntick,
>
> - TP_PROTO(const char *polarity, long oldnesting, long newnesting, atomic_t dynticks),
> + TP_PROTO(const char *polarity, const char *context, long oldnesting,
> + long newnesting, atomic_t dynticks),
>
> - TP_ARGS(polarity, oldnesting, newnesting, dynticks),
> + TP_ARGS(polarity, context, oldnesting, newnesting, dynticks),
>
> TP_STRUCT__entry(
> __field(const char *, polarity)
> + __field(const char *, context)
> __field(long, oldnesting)
> __field(long, newnesting)
> __field(int, dynticks)
> @@ -462,14 +464,15 @@ TRACE_EVENT_RCU(rcu_dyntick,
>
> TP_fast_assign(
> __entry->polarity = polarity;
> + __entry->context = context;
> __entry->oldnesting = oldnesting;
> __entry->newnesting = newnesting;
> __entry->dynticks = atomic_read(&dynticks);
> ),
>
> - TP_printk("%s %lx %lx %#3x", __entry->polarity,
> - __entry->oldnesting, __entry->newnesting,
> - __entry->dynticks & 0xfff)
> + TP_printk("%s %s %lx %lx %#3x", __entry->polarity,
> + __entry->context, __entry->oldnesting, __entry->newnesting,
> + __entry->dynticks & 0xfff)
> );
>
> /*
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 1465a3e406f8..1a65919ec800 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -570,7 +570,8 @@ static void rcu_eqs_enter(bool user)
> }
>
> lockdep_assert_irqs_disabled();
> - trace_rcu_dyntick(TPS("Start"), rdp->dynticks_nesting, 0, rdp->dynticks);
> + trace_rcu_dyntick(TPS("Start"), (user ? TPS("USER") : TPS("IDLE")),
> + rdp->dynticks_nesting, 0, rdp->dynticks);
> WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !user && !is_idle_task(current));
> rdp = this_cpu_ptr(&rcu_data);
> do_nocb_deferred_wakeup(rdp);
> @@ -642,15 +643,17 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
> * leave it in non-RCU-idle state.
> */
> if (rdp->dynticks_nesting != 1) {
> - trace_rcu_dyntick(TPS("--="), rdp->dynticks_nesting,
> - rdp->dynticks_nesting - 2, rdp->dynticks);
> + trace_rcu_dyntick(TPS("StillNonIdle"), TPS("IRQ"),
> + rdp->dynticks_nesting, rdp->dynticks_nesting - 2,
> + rdp->dynticks);
> WRITE_ONCE(rdp->dynticks_nesting, /* No store tearing. */
> rdp->dynticks_nesting - 2);
> return;
> }
>
> /* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
> - trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nesting, 0, rdp->dynticks);
> + trace_rcu_dyntick(TPS("Start"), TPS("IRQ"), rdp->dynticks_nesting, 0,
> + rdp->dynticks);
> WRITE_ONCE(rdp->dynticks_nesting, 0); /* Avoid store tearing. */
>
> if (irq)
> @@ -733,7 +736,8 @@ static void rcu_eqs_exit(bool user)
> rcu_dynticks_task_exit();
> rcu_dynticks_eqs_exit();
> rcu_cleanup_after_idle();
> - trace_rcu_dyntick(TPS("End"), rdp->dynticks_nesting, 1, rdp->dynticks);
> + trace_rcu_dyntick(TPS("End"), (user ? TPS("USER") : TPS("IDLE")),
> + rdp->dynticks_nesting, 1, rdp->dynticks);
> WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !user && !is_idle_task(current));
> WRITE_ONCE(rdp->dynticks_nesting, 1);
>
> @@ -825,7 +829,8 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
> tick_dep_set_cpu(rdp->cpu, TICK_DEP_BIT_RCU);
> }
>
> - trace_rcu_dyntick(incby == 1 ? TPS("Endirq") : TPS("++="),
> + trace_rcu_dyntick(incby == 1 ? TPS("End") : TPS("StillNonIdle"),
> + TPS("IRQ"),
> rdp->dynticks_nesting,
> rdp->dynticks_nesting + incby, rdp->dynticks);
>
> --
> 2.23.0.187.g17f5b7556c-goog
>
^ permalink raw reply
* Re: [patch] Fix up l1ft documentation was Re: Taking a break - time to look back
From: Pavel Machek @ 2019-08-28 22:18 UTC (permalink / raw)
To: Thomas Gleixner
Cc: corbet, LKML, Linus Torvalds, x86, Peter Zijlstra, Jiri Kosina,
Josh Poimboeuf, Dave Hansen, Andy Lutomirski, Greg KH,
Konrad Rzeszutek Wilk, David Woodhouse, Tom Lendacky,
Paolo Bonzini, Joerg Roedel, Tony Luck, Salvatore Bonaccorso,
linux-doc
In-Reply-To: <alpine.DEB.2.21.1903120633000.1985@nanos.tec.linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1696 bytes --]
Hi!
> On Tue, 12 Mar 2019, Pavel Machek wrote:
> > On Mon 2019-03-11 23:31:08, Thomas Gleixner wrote:
> > > Calling this a lie is a completly unjustified personal attack on those who
> >
> > So how should it be called? I initally used less strong words, only to
> > get "Care to tell what's a lie instead of making bold statements?"
> > back. Also look at the timing of the thread.
>
> You called it a lie from the very beginning or what do you think made me
> tell you that? Here is what you said:
Actually, I still call it a lie. Document clearly says that bug is
fixed in non-virtualized cases, when in fact it depends on PAE and
limited memory.
> If you want to provide more accurate documentation then you better come up
> with something which is helpful instead of completely useless blurb like
> the below:
At this point I want you to fix it yourself. Lying about security bugs
being fixed when they are not is not cool. I tried to be helpful and
submit a patch, but I don't feel like you are cooperating on getting
the patch applied.
> > + Mitigation is present in kernels v4.19 and newer, and in
> > + recent -stable kernels. PAE needs to be enabled for mitigation to
> > + work.
>
> No. The mitigation is available when the kernel provides it. Numbers are
> irrelevant because that documentation has to be applicable for stable
> kernels as well. And what is a recent -stable kernel?
>
> Also the PAE part needs to go to a completely different section.
Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* [PATCH v2 3/5] padata: get rid of padata_remove_cpu() for real
From: Daniel Jordan @ 2019-08-28 22:14 UTC (permalink / raw)
To: Herbert Xu, Steffen Klassert
Cc: Sebastian Andrzej Siewior, Thomas Gleixner, linux-crypto,
linux-kernel, Daniel Jordan, Jonathan Corbet, linux-doc
In-Reply-To: <20190828221425.22701-1-daniel.m.jordan@oracle.com>
A later patch is going to address a lock ordering issue involving
pinst->mutex and the CPU hotplug lock. padata_remove_cpu() needs fixing
but it has no callers, so just delete it rather than maintaining unused
code. The Fixes commit forgot to do it anyway.
While at it remove Documentation references to other unused functions.
Fixes: 815613da6a67 ("kernel/padata.c: removed unused code")
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-crypto@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
Documentation/padata.txt | 18 ++++--------------
kernel/padata.c | 35 -----------------------------------
2 files changed, 4 insertions(+), 49 deletions(-)
diff --git a/Documentation/padata.txt b/Documentation/padata.txt
index b103d0c82000..43ca928da713 100644
--- a/Documentation/padata.txt
+++ b/Documentation/padata.txt
@@ -51,27 +51,17 @@ padata cpumask contains no active CPU (flag not set).
padata_stop clears the flag and blocks until the padata instance
is unused.
-The list of CPUs to be used can be adjusted with these functions::
+The list of CPUs to be used can be adjusted with this function::
- int padata_set_cpumasks(struct padata_instance *pinst,
- cpumask_var_t pcpumask,
- cpumask_var_t cbcpumask);
int padata_set_cpumask(struct padata_instance *pinst, int cpumask_type,
cpumask_var_t cpumask);
- int padata_add_cpu(struct padata_instance *pinst, int cpu, int mask);
- int padata_remove_cpu(struct padata_instance *pinst, int cpu, int mask);
Changing the CPU masks are expensive operations, though, so it should not be
done with great frequency.
-It's possible to change both cpumasks of a padata instance with
-padata_set_cpumasks by specifying the cpumasks for parallel execution (pcpumask)
-and for the serial callback function (cbcpumask). padata_set_cpumask is used to
-change just one of the cpumasks. Here cpumask_type is one of PADATA_CPU_SERIAL,
-PADATA_CPU_PARALLEL and cpumask specifies the new cpumask to use.
-To simply add or remove one CPU from a certain cpumask the functions
-padata_add_cpu/padata_remove_cpu are used. cpu specifies the CPU to add or
-remove and mask is one of PADATA_CPU_SERIAL, PADATA_CPU_PARALLEL.
+padata_set_cpumask is used to change just one of the cpumasks. Here cpumask_type
+is one of PADATA_CPU_SERIAL or PADATA_CPU_PARALLEL, and cpumask specifies the
+new cpumask to use.
If a user is interested in padata cpumask changes, he can register to
the padata cpumask change notifier::
diff --git a/kernel/padata.c b/kernel/padata.c
index 2bfce01c5b85..6adce3b203fe 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -710,41 +710,6 @@ static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
return 0;
}
- /**
- * padata_remove_cpu - remove a cpu from the one or both(serial and parallel)
- * padata cpumasks.
- *
- * @pinst: padata instance
- * @cpu: cpu to remove
- * @mask: bitmask specifying from which cpumask @cpu should be removed
- * The @mask may be any combination of the following flags:
- * PADATA_CPU_SERIAL - serial cpumask
- * PADATA_CPU_PARALLEL - parallel cpumask
- */
-int padata_remove_cpu(struct padata_instance *pinst, int cpu, int mask)
-{
- int err;
-
- if (!(mask & (PADATA_CPU_SERIAL | PADATA_CPU_PARALLEL)))
- return -EINVAL;
-
- mutex_lock(&pinst->lock);
-
- get_online_cpus();
- if (mask & PADATA_CPU_SERIAL)
- cpumask_clear_cpu(cpu, pinst->cpumask.cbcpu);
- if (mask & PADATA_CPU_PARALLEL)
- cpumask_clear_cpu(cpu, pinst->cpumask.pcpu);
-
- err = __padata_remove_cpu(pinst, cpu);
- put_online_cpus();
-
- mutex_unlock(&pinst->lock);
-
- return err;
-}
-EXPORT_SYMBOL(padata_remove_cpu);
-
static inline int pinst_has_cpu(struct padata_instance *pinst, int cpu)
{
return cpumask_test_cpu(cpu, pinst->cpumask.pcpu) ||
--
2.23.0
^ permalink raw reply related
* Re: [RFC v1 2/2] rcu/tree: Remove dynticks_nmi_nesting counter
From: Joel Fernandes @ 2019-08-28 22:14 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Frederic Weisbecker, Jonathan Corbet, Josh Triplett,
kernel-team, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
Mauro Carvalho Chehab, rcu, Steven Rostedt
In-Reply-To: <20190828220108.GC26530@linux.ibm.com>
On Wed, Aug 28, 2019 at 03:01:08PM -0700, Paul E. McKenney wrote:
> On Wed, Aug 28, 2019 at 05:42:41PM -0400, Joel Fernandes wrote:
> > On Wed, Aug 28, 2019 at 02:19:04PM -0700, Paul E. McKenney wrote:
> > > On Wed, Aug 28, 2019 at 05:05:25PM -0400, Joel Fernandes wrote:
> > > > On Wed, Aug 28, 2019 at 01:23:30PM -0700, Paul E. McKenney wrote:
> > > > > On Mon, Aug 26, 2019 at 09:33:54PM -0400, Joel Fernandes (Google) wrote:
> > > > > > The dynticks_nmi_nesting counter serves 4 purposes:
> > > > > >
> > > > > > (a) rcu_is_cpu_rrupt_from_idle() needs to be able to detect first
> > > > > > interrupt nesting level.
> > > > > >
> > > > > > (b) We need to detect half-interrupts till we are sure they're not an
> > > > > > issue. However, change the comparison to DYNTICK_IRQ_NONIDLE with 0.
> > > > > >
> > > > > > (c) When a quiescent state report is needed from a nohz_full CPU.
> > > > > > The nesting counter detects we are a first level interrupt.
> > > > > >
> > > > > > For (a) we can just use dyntick_nesting == 1 to determine this. Only the
> > > > > > outermost interrupt that interrupted an RCU-idle state can set it to 1.
> > > > > >
> > > > > > For (b), this warning condition has not occurred for several kernel
> > > > > > releases. But we still keep the warning but change it to use
> > > > > > in_interrupt() instead of the nesting counter. In a later year, we can
> > > > > > remove the warning.
> > > > > >
> > > > > > For (c), the nest check is not really necessary since forced_tick would
> > > > > > have been set to true in the outermost interrupt, so the nested/NMI
> > > > > > interrupts will check forced_tick anyway, and bail.
> > > > >
> > > > > Skipping the commit log and documentation for this pass.
> > > > [snip]
> > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > > > index 255cd6835526..1465a3e406f8 100644
> > > > > > --- a/kernel/rcu/tree.c
> > > > > > +++ b/kernel/rcu/tree.c
> > > > > > @@ -81,7 +81,6 @@
> > > > > >
> > > > > > static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_data) = {
> > > > > > .dynticks_nesting = 1,
> > > > > > - .dynticks_nmi_nesting = 0,
> > > > >
> > > > > This should be in the previous patch, give or take naming.
> > > >
> > > > Done.
> > > >
> > > > > > .dynticks = ATOMIC_INIT(RCU_DYNTICK_CTRL_CTR),
> > > > > > };
> > > > > > struct rcu_state rcu_state = {
> > > > > > @@ -392,15 +391,9 @@ static int rcu_is_cpu_rrupt_from_idle(void)
> > > > > > /* Check for counter underflows */
> > > > > > RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nesting) < 0,
> > > > > > "RCU dynticks_nesting counter underflow!");
> > > > > > - RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0,
> > > > > > - "RCU dynticks_nmi_nesting counter underflow/zero!");
> > > > > >
> > > > > > - /* Are we at first interrupt nesting level? */
> > > > > > - if (__this_cpu_read(rcu_data.dynticks_nmi_nesting) != 1)
> > > > > > - return false;
> > > > > > -
> > > > > > - /* Does CPU appear to be idle from an RCU standpoint? */
> > > > > > - return __this_cpu_read(rcu_data.dynticks_nesting) == 0;
> > > > > > + /* Are we the outermost interrupt that arrived when RCU was idle? */
> > > > > > + return __this_cpu_read(rcu_data.dynticks_nesting) == 1;
> > > > > > }
> > > > > >
> > > > > > #define DEFAULT_RCU_BLIMIT 10 /* Maximum callbacks per rcu_do_batch ... */
> > > > > > @@ -564,11 +557,10 @@ static void rcu_eqs_enter(bool user)
> > > > > > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > > > >
> > > > > > /* Entering usermode/idle from interrupt is not handled. These would
> > > > > > - * mean usermode upcalls or idle entry happened from interrupts. But,
> > > > > > - * reset the counter if we warn.
> > > > > > + * mean usermode upcalls or idle exit happened from interrupts. Remove
> > > > > > + * the warning by 2020.
> > > > > > */
> > > > > > - if (WARN_ON_ONCE(rdp->dynticks_nmi_nesting != 0))
> > > > > > - WRITE_ONCE(rdp->dynticks_nmi_nesting, 0);
> > > > > > + WARN_ON_ONCE(in_interrupt());
> > > > >
> > > > > And this is a red flag. Bad things happen should some common code
> > > > > that disables BH be invoked from the idle loop. This might not be
> > > > > happening now, but we need to avoid this sort of constraint.
> > > > > How about instead merging ->dyntick_nesting into the low-order bits
> > > > > of ->dyntick_nmi_nesting?
> > > > >
> > > > > Yes, this assumes that we don't enter process level twice, but it should
> > > > > be easy to add a WARN_ON() to test for that. Except that we don't have
> > > > > to because there is already this near the end of rcu_eqs_exit():
> > > > >
> > > > > WARN_ON_ONCE(rdp->dynticks_nmi_nesting);
> > > > >
> > > > > So the low-order bit of the combined counter could indicate process-level
> > > > > non-idle, the next three bits could be unused to make interpretation
> > > > > of hex printouts easier, and then the rest of the bits could be used in
> > > > > the same way as currently.
> > > > >
> > > > > This would allow a single read to see the full state, so that 0x1 means
> > > > > at process level in the kernel, 0x11 is interrupt (or NMI) from process
> > > > > level, 0x10 is interrupt/NMI from idle/user, and so on.
> > > > >
> > > > > What am I missing here? Why wouldn't this work, and without adding yet
> > > > > another RCU-imposed constraint on some other subsystem?
> > > >
> > > > What about replacing the warning with a WARN_ON_ONCE(in_irq()), would that
> > > > address your concern?
> > > >
> > > > Also, considering this warning condition is most likely never occurring as we
> > > > know it, and we are considering deleting it soon enough, is it really worth
> > > > reimplementing the whole mechanism with a complex bit-sharing scheme just
> > > > because of the BH-disable condition you mentioned, which likely doesn't
> > > > happen today? In my implementation, this is just a simple counter. I feel
> > > > combining bits in the same counter will just introduce more complexity that
> > > > this patch tries to address/avoid.
> > > >
> > > > OTOH, I also don't mind with just deleting the warning altogether if you are
> > > > Ok with that.
> > >
> > > The big advantage of combining the counters is that all of the state is
> > > explicit and visible in one place. Plus it can be accessed atomically.
> > > And it avoids setting a time bomb for some poor guys just trying to get
> > > their idle-loop jobs done some time in the dim distant future.
> >
> > I could try the approach you're suggesting but I didn't actually see an issue
> > with the patch in its current state other than the WARN_ON_ONCE which I could
> > change to WARN_ON_ONCE(in_irq()) to remove the concern. AFAICS, we don't
> > detect "half soft-interrupts" in this code in anyway.
> >
> > I do feel the approach you're suggesting can be a follow up, these 2 patches
> > just focus on deleting dynticks_nmi_nesting counter and we can test this
> > approach thoroughly for a release or so.
> >
> > > Besides, this pair of patches already makes a large change from a
> > > conceptual viewpoint. If we are going to make a large change, let's
> > > get our money's worth out of that change!
> >
> > IMHO, most of the changes are to code comments, the actual code change is
> > very little and is just removal of dynticks_nmi_nesting and simplification;
> > its not really an introduction of a new mechanism.
>
> This change is not fixing a bug, so there is no need for an emergency fix,
> and thus no point in additional churn. I understand that it is a bit
> annoying to code and test something and have your friendly maintainer say
> "sorry, wrong rocks", and the reason that I understand this is that I do
> that to myself rather often.
The motivation for me for this change is to avoid future bugs such as with
the following patch where "== 2" did not take the force write of
DYNTICK_IRQ_NONIDLE into account:
https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=13c4b07593977d9288e5d0c21c89d9ba27e2ea1f
I still don't see it as pointless churn, it is also a maintenance cost in its
current form and the simplification is worth it IMHO both from a readability,
and maintenance stand point.
I still don't see what's technically wrong with the patch. I could perhaps
add the above "== 2" point in the patch?
We could also discuss f2f at LPC to see if we can agree about it?
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH 4.19 72/98] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Pavel Machek @ 2019-08-28 22:05 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Pavel Machek, Borislav Petkov, Greg Kroah-Hartman, linux-kernel,
stable, Tom Lendacky, Andrew Cooper, Andrew Morton, Chen Yu,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Poimboeuf,
Juergen Gross, Kees Cook, linux-doc@vger.kernel.org,
linux-pm@vger.kernel.org, Nathan Chancellor, Paolo Bonzini,
Rafael J. Wysocki, x86@kernel.org
In-Reply-To: <alpine.DEB.2.21.1908281610310.23149@nanos.tec.linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 2210 bytes --]
On Wed 2019-08-28 16:15:06, Thomas Gleixner wrote:
> On Wed, 28 Aug 2019, Pavel Machek wrote:
> > On Wed 2019-08-28 14:46:21, Borislav Petkov wrote:
> > > On Wed, Aug 28, 2019 at 02:29:13PM +0200, Pavel Machek wrote:
> > > > This is not a way to have an inteligent conversation.
> > >
> > > No, this *is* the way to keep the conversation sane, without veering
> > > off into some absurd claims.
> > >
> > > So, to cut to the chase: you can simply add "rdrand=force" to your
> > > cmdline parameters and get back to using RDRAND.
> > >
> > > And yet if you still feel this fix does not meet your expectations,
> > > you were told already to either produce patches or who to contact. I'm
> > > afraid complaining on this thread won't get you anywhere but that's your
> > > call.
> >
> > No, this does not meet my expectations, it violates stable kernel
> > rules, and will cause regression to some users, while better solution
> > is known to be available.
>
> Your unqualified ranting does not meet my expectation either and it
> violates any rule of common sense.
>
> For the record:
>
> Neither AMD nor we have any idea which particular machines have a fixed
> BIOS and which have not. There is no technical indicator either at boot
> time as the wreckage manifests itself only after resume.
>
> So in the interest of users the only sensible decision is to disable
> RDRAND for this class of CPUs.
No.
Obviously best solution would be microcode update to fix the problem,
or to enable kernel to fix the problem.
> If you have a list of machines which have a fixed BIOS, then provide it
> in form of patches. If not then stop claiming that there is a better
> solution available.
And yes, whitelist would be already better than present solution. It is
not my duty to submit fixes to your proposed patch.
> Anyway, I'm done with that and further rants of yours go directly to
> /dev/null.
>
> Thanks for wasting everyones time
Thanks for your profesional attitude.
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [RFC v1 2/2] rcu/tree: Remove dynticks_nmi_nesting counter
From: Paul E. McKenney @ 2019-08-28 22:01 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Frederic Weisbecker, Jonathan Corbet, Josh Triplett,
kernel-team, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
Mauro Carvalho Chehab, rcu, Steven Rostedt
In-Reply-To: <20190828214241.GD75931@google.com>
On Wed, Aug 28, 2019 at 05:42:41PM -0400, Joel Fernandes wrote:
> On Wed, Aug 28, 2019 at 02:19:04PM -0700, Paul E. McKenney wrote:
> > On Wed, Aug 28, 2019 at 05:05:25PM -0400, Joel Fernandes wrote:
> > > On Wed, Aug 28, 2019 at 01:23:30PM -0700, Paul E. McKenney wrote:
> > > > On Mon, Aug 26, 2019 at 09:33:54PM -0400, Joel Fernandes (Google) wrote:
> > > > > The dynticks_nmi_nesting counter serves 4 purposes:
> > > > >
> > > > > (a) rcu_is_cpu_rrupt_from_idle() needs to be able to detect first
> > > > > interrupt nesting level.
> > > > >
> > > > > (b) We need to detect half-interrupts till we are sure they're not an
> > > > > issue. However, change the comparison to DYNTICK_IRQ_NONIDLE with 0.
> > > > >
> > > > > (c) When a quiescent state report is needed from a nohz_full CPU.
> > > > > The nesting counter detects we are a first level interrupt.
> > > > >
> > > > > For (a) we can just use dyntick_nesting == 1 to determine this. Only the
> > > > > outermost interrupt that interrupted an RCU-idle state can set it to 1.
> > > > >
> > > > > For (b), this warning condition has not occurred for several kernel
> > > > > releases. But we still keep the warning but change it to use
> > > > > in_interrupt() instead of the nesting counter. In a later year, we can
> > > > > remove the warning.
> > > > >
> > > > > For (c), the nest check is not really necessary since forced_tick would
> > > > > have been set to true in the outermost interrupt, so the nested/NMI
> > > > > interrupts will check forced_tick anyway, and bail.
> > > >
> > > > Skipping the commit log and documentation for this pass.
> > > [snip]
> > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > > index 255cd6835526..1465a3e406f8 100644
> > > > > --- a/kernel/rcu/tree.c
> > > > > +++ b/kernel/rcu/tree.c
> > > > > @@ -81,7 +81,6 @@
> > > > >
> > > > > static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_data) = {
> > > > > .dynticks_nesting = 1,
> > > > > - .dynticks_nmi_nesting = 0,
> > > >
> > > > This should be in the previous patch, give or take naming.
> > >
> > > Done.
> > >
> > > > > .dynticks = ATOMIC_INIT(RCU_DYNTICK_CTRL_CTR),
> > > > > };
> > > > > struct rcu_state rcu_state = {
> > > > > @@ -392,15 +391,9 @@ static int rcu_is_cpu_rrupt_from_idle(void)
> > > > > /* Check for counter underflows */
> > > > > RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nesting) < 0,
> > > > > "RCU dynticks_nesting counter underflow!");
> > > > > - RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0,
> > > > > - "RCU dynticks_nmi_nesting counter underflow/zero!");
> > > > >
> > > > > - /* Are we at first interrupt nesting level? */
> > > > > - if (__this_cpu_read(rcu_data.dynticks_nmi_nesting) != 1)
> > > > > - return false;
> > > > > -
> > > > > - /* Does CPU appear to be idle from an RCU standpoint? */
> > > > > - return __this_cpu_read(rcu_data.dynticks_nesting) == 0;
> > > > > + /* Are we the outermost interrupt that arrived when RCU was idle? */
> > > > > + return __this_cpu_read(rcu_data.dynticks_nesting) == 1;
> > > > > }
> > > > >
> > > > > #define DEFAULT_RCU_BLIMIT 10 /* Maximum callbacks per rcu_do_batch ... */
> > > > > @@ -564,11 +557,10 @@ static void rcu_eqs_enter(bool user)
> > > > > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > > >
> > > > > /* Entering usermode/idle from interrupt is not handled. These would
> > > > > - * mean usermode upcalls or idle entry happened from interrupts. But,
> > > > > - * reset the counter if we warn.
> > > > > + * mean usermode upcalls or idle exit happened from interrupts. Remove
> > > > > + * the warning by 2020.
> > > > > */
> > > > > - if (WARN_ON_ONCE(rdp->dynticks_nmi_nesting != 0))
> > > > > - WRITE_ONCE(rdp->dynticks_nmi_nesting, 0);
> > > > > + WARN_ON_ONCE(in_interrupt());
> > > >
> > > > And this is a red flag. Bad things happen should some common code
> > > > that disables BH be invoked from the idle loop. This might not be
> > > > happening now, but we need to avoid this sort of constraint.
> > > > How about instead merging ->dyntick_nesting into the low-order bits
> > > > of ->dyntick_nmi_nesting?
> > > >
> > > > Yes, this assumes that we don't enter process level twice, but it should
> > > > be easy to add a WARN_ON() to test for that. Except that we don't have
> > > > to because there is already this near the end of rcu_eqs_exit():
> > > >
> > > > WARN_ON_ONCE(rdp->dynticks_nmi_nesting);
> > > >
> > > > So the low-order bit of the combined counter could indicate process-level
> > > > non-idle, the next three bits could be unused to make interpretation
> > > > of hex printouts easier, and then the rest of the bits could be used in
> > > > the same way as currently.
> > > >
> > > > This would allow a single read to see the full state, so that 0x1 means
> > > > at process level in the kernel, 0x11 is interrupt (or NMI) from process
> > > > level, 0x10 is interrupt/NMI from idle/user, and so on.
> > > >
> > > > What am I missing here? Why wouldn't this work, and without adding yet
> > > > another RCU-imposed constraint on some other subsystem?
> > >
> > > What about replacing the warning with a WARN_ON_ONCE(in_irq()), would that
> > > address your concern?
> > >
> > > Also, considering this warning condition is most likely never occurring as we
> > > know it, and we are considering deleting it soon enough, is it really worth
> > > reimplementing the whole mechanism with a complex bit-sharing scheme just
> > > because of the BH-disable condition you mentioned, which likely doesn't
> > > happen today? In my implementation, this is just a simple counter. I feel
> > > combining bits in the same counter will just introduce more complexity that
> > > this patch tries to address/avoid.
> > >
> > > OTOH, I also don't mind with just deleting the warning altogether if you are
> > > Ok with that.
> >
> > The big advantage of combining the counters is that all of the state is
> > explicit and visible in one place. Plus it can be accessed atomically.
> > And it avoids setting a time bomb for some poor guys just trying to get
> > their idle-loop jobs done some time in the dim distant future.
>
> I could try the approach you're suggesting but I didn't actually see an issue
> with the patch in its current state other than the WARN_ON_ONCE which I could
> change to WARN_ON_ONCE(in_irq()) to remove the concern. AFAICS, we don't
> detect "half soft-interrupts" in this code in anyway.
>
> I do feel the approach you're suggesting can be a follow up, these 2 patches
> just focus on deleting dynticks_nmi_nesting counter and we can test this
> approach thoroughly for a release or so.
>
> > Besides, this pair of patches already makes a large change from a
> > conceptual viewpoint. If we are going to make a large change, let's
> > get our money's worth out of that change!
>
> IMHO, most of the changes are to code comments, the actual code change is
> very little and is just removal of dynticks_nmi_nesting and simplification;
> its not really an introduction of a new mechanism.
This change is not fixing a bug, so there is no need for an emergency fix,
and thus no point in additional churn. I understand that it is a bit
annoying to code and test something and have your friendly maintainer say
"sorry, wrong rocks", and the reason that I understand this is that I do
that to myself rather often.
Welcome to the wonderful world of RCU! ;-)
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 3/5] rcu/tree: Add support for debug_objects debugging for kfree_rcu()
From: Paul E. McKenney @ 2019-08-28 21:57 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, byungchul.park, Josh Triplett, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, rcu, Steven Rostedt, kernel-team
In-Reply-To: <20190828214320.GE75931@google.com>
On Wed, Aug 28, 2019 at 05:43:20PM -0400, Joel Fernandes wrote:
> On Wed, Aug 28, 2019 at 02:31:19PM -0700, Paul E. McKenney wrote:
> > On Tue, Aug 27, 2019 at 03:01:57PM -0400, Joel Fernandes (Google) wrote:
> > > Make use of RCU's debug_objects debugging support
> > > (CONFIG_DEBUG_OBJECTS_RCU_HEAD) similar to call_rcu() and other flavors.
> >
> > Other flavors? Ah, call_srcu(), rcu_barrier(), and srcu_barrier(),
> > right?
>
> Yes.
>
> > > We queue the object during the kfree_rcu() call and dequeue it during
> > > reclaim.
> > >
> > > Tested that enabling CONFIG_DEBUG_OBJECTS_RCU_HEAD successfully detects
> > > double kfree_rcu() calls.
> > >
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >
> > The code looks good!
>
> thanks, does that mean you'll ack/apply it? :-P
Is it independent of 1/5 and 2/5?
Thanx, Paul
> - Joel
>
> >
> > Thanx, Paul
> >
> > > ---
> > > kernel/rcu/tree.c | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 9b9ae4db1c2d..64568f12641d 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -2757,6 +2757,7 @@ static void kfree_rcu_work(struct work_struct *work)
> > > for (; head; head = next) {
> > > next = head->next;
> > > /* Could be possible to optimize with kfree_bulk in future */
> > > + debug_rcu_head_unqueue(head);
> > > __rcu_reclaim(rcu_state.name, head);
> > > cond_resched_tasks_rcu_qs();
> > > }
> > > @@ -2868,6 +2869,13 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> > > if (rcu_scheduler_active != RCU_SCHEDULER_RUNNING)
> > > return kfree_call_rcu_nobatch(head, func);
> > >
> > > + if (debug_rcu_head_queue(head)) {
> > > + /* Probable double kfree_rcu() */
> > > + WARN_ONCE(1, "kfree_call_rcu(): Double-freed call. rcu_head %p\n",
> > > + head);
> > > + return;
> > > + }
> > > +
> > > head->func = func;
> > >
> > > local_irq_save(flags); /* For safely calling this_cpu_ptr(). */
> > > --
> > > 2.23.0.187.g17f5b7556c-goog
> > >
^ permalink raw reply
* Re: [PATCH 5/5] rcu: Remove kfree_call_rcu_nobatch()
From: Paul E. McKenney @ 2019-08-28 21:56 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, byungchul.park, Josh Triplett, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, rcu, Steven Rostedt, kernel-team
In-Reply-To: <5d657e3b.1c69fb81.54250.01e2@mx.google.com>
On Tue, Aug 27, 2019 at 03:01:59PM -0400, Joel Fernandes (Google) wrote:
> Now that kfree_rcu() special casing have been removed from tree RCU,
> remove kfree_call_rcu_nobatch() since it is not needed.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Now -this- one qualifies as a nice negative delta! ;-)
A few things below, please fix in next version.
Thanx, Paul
> ---
> .../admin-guide/kernel-parameters.txt | 4 ---
> include/linux/rcutiny.h | 5 ---
> include/linux/rcutree.h | 1 -
> kernel/rcu/rcuperf.c | 10 +-----
> kernel/rcu/tree.c | 33 ++++++++-----------
> 5 files changed, 14 insertions(+), 39 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 24fe8aefb12c..56be0e30100b 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3909,10 +3909,6 @@
> Number of loops doing rcuperf.kfree_alloc_num number
> of allocations and frees.
>
> - rcuperf.kfree_no_batch= [KNL]
> - Use the non-batching (less efficient) version of kfree_rcu().
> - This is useful for comparing with the batched version.
> -
> rcuperf.nreaders= [KNL]
> Set number of RCU readers. The value -1 selects
> N, where N is the number of CPUs. A value
> diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h
> index 949841f52ec5..7aa93afa5d8d 100644
> --- a/include/linux/rcutiny.h
> +++ b/include/linux/rcutiny.h
> @@ -39,11 +39,6 @@ static inline void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> call_rcu(head, func);
> }
>
> -static inline void kfree_call_rcu_nobatch(struct rcu_head *head, rcu_callback_t func)
> -{
> - call_rcu(head, func);
> -}
> -
> void rcu_qs(void);
>
> static inline void rcu_softirq_qs(void)
> diff --git a/include/linux/rcutree.h b/include/linux/rcutree.h
> index 961b7e05d141..0b68aa952f8b 100644
> --- a/include/linux/rcutree.h
> +++ b/include/linux/rcutree.h
> @@ -34,7 +34,6 @@ static inline void rcu_virt_note_context_switch(int cpu)
>
> void synchronize_rcu_expedited(void);
> void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func);
> -void kfree_call_rcu_nobatch(struct rcu_head *head, rcu_callback_t func);
>
> void rcu_barrier(void);
> bool rcu_eqs_special_set(int cpu);
> diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c
> index c1e25fd10f2a..da94b89cd531 100644
> --- a/kernel/rcu/rcuperf.c
> +++ b/kernel/rcu/rcuperf.c
> @@ -593,7 +593,6 @@ rcu_perf_shutdown(void *arg)
> torture_param(int, kfree_nthreads, -1, "Number of threads running loops of kfree_rcu().");
> torture_param(int, kfree_alloc_num, 8000, "Number of allocations and frees done in an iteration.");
> torture_param(int, kfree_loops, 10, "Number of loops doing kfree_alloc_num allocations and frees.");
> -torture_param(int, kfree_no_batch, 0, "Use the non-batching (slower) version of kfree_rcu().");
>
> static struct task_struct **kfree_reader_tasks;
> static int kfree_nrealthreads;
> @@ -632,14 +631,7 @@ kfree_perf_thread(void *arg)
> if (!alloc_ptr)
> return -ENOMEM;
>
> - if (!kfree_no_batch) {
> - kfree_rcu(alloc_ptr, rh);
> - } else {
> - rcu_callback_t cb;
> -
> - cb = (rcu_callback_t)(unsigned long)offsetof(struct kfree_obj, rh);
> - kfree_call_rcu_nobatch(&(alloc_ptr->rh), cb);
> - }
> + kfree_rcu(alloc_ptr, rh);
> }
>
> cond_resched();
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 12c17e10f2b4..c767973d62ac 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2777,8 +2777,10 @@ static void kfree_rcu_work(struct work_struct *work)
> rcu_lock_acquire(&rcu_callback_map);
> trace_rcu_invoke_kfree_callback(rcu_state.name, head, offset);
>
> - /* Could be possible to optimize with kfree_bulk in future */
> - kfree((void *)head - offset);
> + if (!WARN_ON_ONCE(!__is_kfree_rcu_offset(offset))) {
> + /* Could be optimized with kfree_bulk() in future. */
> + kfree((void *)head - offset);
> + }
This really needs to be in the previous patch until such time as Tiny RCU
no longer needs the restriction.
> rcu_lock_release(&rcu_callback_map);
> cond_resched_tasks_rcu_qs();
> @@ -2856,16 +2858,6 @@ static void kfree_rcu_monitor(struct work_struct *work)
> spin_unlock_irqrestore(&krcp->lock, flags);
> }
>
> -/*
> - * This version of kfree_call_rcu does not do batching of kfree_rcu() requests.
> - * Used only by rcuperf torture test for comparison with kfree_rcu_batch().
> - */
> -void kfree_call_rcu_nobatch(struct rcu_head *head, rcu_callback_t func)
> -{
> - __call_rcu(head, func);
> -}
> -EXPORT_SYMBOL_GPL(kfree_call_rcu_nobatch);
> -
> /*
> * Queue a request for lazy invocation of kfree() after a grace period.
> *
> @@ -2885,12 +2877,6 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> unsigned long flags;
> struct kfree_rcu_cpu *krcp;
>
> - /* kfree_call_rcu() batching requires timers to be up. If the scheduler
> - * is not yet up, just skip batching and do the non-batched version.
> - */
> - if (rcu_scheduler_active != RCU_SCHEDULER_RUNNING)
> - return kfree_call_rcu_nobatch(head, func);
> -
> if (debug_rcu_head_queue(head)) {
> /* Probable double kfree_rcu() */
> WARN_ONCE(1, "kfree_call_rcu(): Double-freed call. rcu_head %p\n",
> @@ -2909,8 +2895,15 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> krcp->head = head;
>
> /* Schedule monitor for timely drain after KFREE_DRAIN_JIFFIES. */
> - if (!xchg(&krcp->monitor_todo, true))
> - schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES);
> + if (!xchg(&krcp->monitor_todo, true)) {
> + /* Scheduling the monitor requires scheduler/timers to be up,
> + * if it is not, just skip it. An eventual kfree_rcu() will
> + * kick it again.
> + */
> + if ((rcu_scheduler_active == RCU_SCHEDULER_RUNNING)) {
> + schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES);
> + }
> + }
And this also needs to be in an earlier patch. Bisectability and all that!
Are we really guaranteed that there will be an eventual kfree_rcu()?
More of a worry for Tiny RCU than for Tree RCU, but still could be
annoying for someone trying to debug a memory leak.
Thanx, Paul
> spin_unlock(&krcp->lock);
> local_irq_restore(flags);
> --
> 2.23.0.187.g17f5b7556c-goog
>
^ permalink raw reply
* Re: [RFC v1 1/2] rcu/tree: Clean up dynticks counter usage
From: Joel Fernandes @ 2019-08-28 21:56 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Frederic Weisbecker, Jonathan Corbet, Josh Triplett,
kernel-team, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
Mauro Carvalho Chehab, rcu, Steven Rostedt
In-Reply-To: <20190828201344.GR26530@linux.ibm.com>
On Wed, Aug 28, 2019 at 01:13:44PM -0700, Paul E. McKenney wrote:
> On Mon, Aug 26, 2019 at 09:33:53PM -0400, Joel Fernandes (Google) wrote:
> > The dynticks counter are confusing due to crowbar writes of
> > DYNTICK_IRQ_NONIDLE whose purpose is to detect half-interrupts (i.e. we
> > see rcu_irq_enter() but not rcu_irq_exit() due to a usermode upcall) and
> > if so then do a reset of the dyntick_nmi_nesting counters. This patch
> > tries to get rid of DYNTICK_IRQ_NONIDLE while still keeping the code
> > working, fully functional, and less confusing. The confusion recently
> > has even led to patches forgetting that DYNTICK_IRQ_NONIDLE was written
> > to which wasted lots of time.
> >
> > The patch has the following changes:
[snip]
> > /*
> > * Grace-period counter management.
> > */
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 68ebf0eb64c8..255cd6835526 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -81,7 +81,7 @@
> >
> > static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_data) = {
> > .dynticks_nesting = 1,
> > - .dynticks_nmi_nesting = DYNTICK_IRQ_NONIDLE,
> > + .dynticks_nmi_nesting = 0,
>
> C initializes to zero by default, so this can simply be deleted.
Fixed.
> > .dynticks = ATOMIC_INIT(RCU_DYNTICK_CTRL_CTR),
> > };
> > struct rcu_state rcu_state = {
> > @@ -558,17 +558,18 @@ EXPORT_SYMBOL_GPL(rcutorture_get_gp_data);
> > /*
> > * Enter an RCU extended quiescent state, which can be either the
> > * idle loop or adaptive-tickless usermode execution.
> > - *
> > - * We crowbar the ->dynticks_nmi_nesting field to zero to allow for
> > - * the possibility of usermode upcalls having messed up our count
> > - * of interrupt nesting level during the prior busy period.
> > */
> > static void rcu_eqs_enter(bool user)
> > {
> > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> >
> > - WARN_ON_ONCE(rdp->dynticks_nmi_nesting != DYNTICK_IRQ_NONIDLE);
> > - WRITE_ONCE(rdp->dynticks_nmi_nesting, 0);
> > + /* Entering usermode/idle from interrupt is not handled. These would
> > + * mean usermode upcalls or idle entry happened from interrupts. But,
> > + * reset the counter if we warn.
> > + */
>
> Please either put the "/*" on its own line or use "//"-style comments.
I'll put "/*" on its own line.
> > WARN_ON_ONCE(rdp->dynticks_nmi_nesting <= 0);
> > WARN_ON_ONCE(rcu_dynticks_curr_cpu_in_eqs());
> >
> > + WRITE_ONCE(rdp->dynticks_nmi_nesting, /* No store tearing. */
> > + rdp->dynticks_nmi_nesting - 1);
>
> This is problematic. The +/-1 and +/-2 dance is specifically for NMIs, so...
This counter is deleted in the following patch so I hope its Ok to leave it
here for this one. I just kept it split into different patch to make
testing/review/development easier.
> > if (irq)
> > rcu_prepare_for_idle();
> > @@ -723,10 +728,6 @@ void rcu_irq_exit_irqson(void)
> > /*
> > * Exit an RCU extended quiescent state, which can be either the
> > * idle loop or adaptive-tickless usermode execution.
> > - *
> > - * We crowbar the ->dynticks_nmi_nesting field to DYNTICK_IRQ_NONIDLE to
> > - * allow for the possibility of usermode upcalls messing up our count of
> > - * interrupt nesting level during the busy period that is just now starting.
> > */
> > static void rcu_eqs_exit(bool user)
> > {
> > @@ -747,8 +748,13 @@ static void rcu_eqs_exit(bool user)
> > trace_rcu_dyntick(TPS("End"), rdp->dynticks_nesting, 1, rdp->dynticks);
> > WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !user && !is_idle_task(current));
> > WRITE_ONCE(rdp->dynticks_nesting, 1);
> > - WARN_ON_ONCE(rdp->dynticks_nmi_nesting);
> > - WRITE_ONCE(rdp->dynticks_nmi_nesting, DYNTICK_IRQ_NONIDLE);
> > +
> > + /* Exiting usermode/idle from interrupt is not handled. These would
> > + * mean usermode upcalls or idle exit happened from interrupts. But,
> > + * reset the counter if we warn.
> > + */
> > + if (WARN_ON_ONCE(rdp->dynticks_nmi_nesting != 0))
> > + WRITE_ONCE(rdp->dynticks_nmi_nesting, 0);
>
> And here. Plus this is adding a test and branch in the common case.
> Given that the location being written to should be hot in the cache,
> it is not clear that this is a win.
The next patch removes the branch itself and just has the warning.
> > WARN_ON_ONCE(rdp->dynticks_nmi_nesting < 0);
> >
> > /*
> > @@ -826,16 +833,21 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
> >
> > incby = 1;
> > } else if (tick_nohz_full_cpu(rdp->cpu) &&
> > - rdp->dynticks_nmi_nesting == DYNTICK_IRQ_NONIDLE &&
> > - rdp->rcu_urgent_qs && !rdp->rcu_forced_tick) {
> > + !rdp->dynticks_nmi_nesting && rdp->rcu_urgent_qs &&
> > + !rdp->rcu_forced_tick) {
>
> OK. Though you should be able to save a line by pulling the
> "rdp->rcu_urgent_qs &&" onto the first line.
Fixed.
> > rdp->rcu_forced_tick = true;
> > tick_dep_set_cpu(rdp->cpu, TICK_DEP_BIT_RCU);
> > }
> > +
>
> Not clear that the added blank line is a win, here or below.
Fixed,
thanks!
- Joel
[snip]
^ permalink raw reply
* Re: [PATCH 4/5] rcu: Remove kfree_rcu() special casing and lazy handling
From: Paul E. McKenney @ 2019-08-28 21:46 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, byungchul.park, Josh Triplett, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, rcu, Steven Rostedt, kernel-team
In-Reply-To: <5d657e39.1c69fb81.54250.01e0@mx.google.com>
On Tue, Aug 27, 2019 at 03:01:58PM -0400, Joel Fernandes (Google) wrote:
> Remove kfree_rcu() special casing and lazy handling from RCU.
> For Tiny RCU we fold the special handling into just Tiny RCU code.
>
> Suggested-by: Paul E. McKenney <paulmck@linux.ibm.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Nice! Several comments inline below.
Thanx, Paul
> ---
> Documentation/RCU/stallwarn.txt | 13 +++------
> include/linux/rcu_segcblist.h | 2 --
> include/trace/events/rcu.h | 32 +++++++++-------------
> kernel/rcu/rcu.h | 27 -------------------
> kernel/rcu/rcu_segcblist.c | 25 +++--------------
> kernel/rcu/rcu_segcblist.h | 25 ++---------------
> kernel/rcu/srcutree.c | 4 +--
> kernel/rcu/tiny.c | 29 +++++++++++++++++++-
> kernel/rcu/tree.c | 48 ++++++++++++++++++++++++---------
> kernel/rcu/tree.h | 1 -
> kernel/rcu/tree_plugin.h | 42 +++++++----------------------
> kernel/rcu/tree_stall.h | 6 ++---
> 12 files changed, 97 insertions(+), 157 deletions(-)
>
> diff --git a/Documentation/RCU/stallwarn.txt b/Documentation/RCU/stallwarn.txt
> index f48f4621ccbc..2c02b8de01e7 100644
> --- a/Documentation/RCU/stallwarn.txt
> +++ b/Documentation/RCU/stallwarn.txt
> @@ -225,18 +225,12 @@ an estimate of the total number of RCU callbacks queued across all CPUs
> In kernels with CONFIG_RCU_FAST_NO_HZ, more information is printed
> for each CPU:
>
> - 0: (64628 ticks this GP) idle=dd5/3fffffffffffffff/0 softirq=82/543 last_accelerate: a345/d342 Nonlazy posted: ..D
> + 0: (64628 ticks this GP) idle=dd5/3fffffffffffffff/0 softirq=82/543 last_accelerate: a345/d342 dyntick_enabled: 1
>
> The "last_accelerate:" prints the low-order 16 bits (in hex) of the
> jiffies counter when this CPU last invoked rcu_try_advance_all_cbs()
> from rcu_needs_cpu() or last invoked rcu_accelerate_cbs() from
> -rcu_prepare_for_idle(). The "Nonlazy posted:" indicates lazy-callback
> -status, so that an "l" indicates that all callbacks were lazy at the start
> -of the last idle period and an "L" indicates that there are currently
> -no non-lazy callbacks (in both cases, "." is printed otherwise, as
> -shown above) and "D" indicates that dyntick-idle processing is enabled
> -("." is printed otherwise, for example, if disabled via the "nohz="
> -kernel boot parameter).
> +rcu_prepare_for_idle().
>
> If the grace period ends just as the stall warning starts printing,
> there will be a spurious stall-warning message, which will include
> @@ -249,7 +243,8 @@ possible for a zero-jiffy stall to be flagged in this case, depending
> on how the stall warning and the grace-period initialization happen to
> interact. Please note that it is not possible to entirely eliminate this
> sort of false positive without resorting to things like stop_machine(),
> -which is overkill for this sort of problem.
> +which is overkill for this sort of problem. "dyntick_enabled: 1" indicates that
> +dyntick-idle processing is enabled.
OK, I'll bite... Why not add the sentence to the earlier paragraph that
used to discuss its "D" counterpart?
> If all CPUs and tasks have passed through quiescent states, but the
> grace period has nevertheless failed to end, the stall-warning splat
> diff --git a/include/linux/rcu_segcblist.h b/include/linux/rcu_segcblist.h
> index 646759042333..b36afe7b22c9 100644
> --- a/include/linux/rcu_segcblist.h
> +++ b/include/linux/rcu_segcblist.h
> @@ -22,7 +22,6 @@ struct rcu_cblist {
> struct rcu_head *head;
> struct rcu_head **tail;
> long len;
> - long len_lazy;
> };
>
> #define RCU_CBLIST_INITIALIZER(n) { .head = NULL, .tail = &n.head }
> @@ -73,7 +72,6 @@ struct rcu_segcblist {
> #else
> long len;
> #endif
> - long len_lazy;
> u8 enabled;
> u8 offloaded;
> };
> diff --git a/include/trace/events/rcu.h b/include/trace/events/rcu.h
> index 694bd040cf51..0dd3478597ee 100644
> --- a/include/trace/events/rcu.h
> +++ b/include/trace/events/rcu.h
> @@ -474,16 +474,14 @@ TRACE_EVENT_RCU(rcu_dyntick,
> */
> TRACE_EVENT_RCU(rcu_callback,
>
> - TP_PROTO(const char *rcuname, struct rcu_head *rhp, long qlen_lazy,
> - long qlen),
> + TP_PROTO(const char *rcuname, struct rcu_head *rhp, long qlen),
>
> - TP_ARGS(rcuname, rhp, qlen_lazy, qlen),
> + TP_ARGS(rcuname, rhp, qlen),
>
> TP_STRUCT__entry(
> __field(const char *, rcuname)
> __field(void *, rhp)
> __field(void *, func)
> - __field(long, qlen_lazy)
> __field(long, qlen)
> ),
>
> @@ -491,13 +489,12 @@ TRACE_EVENT_RCU(rcu_callback,
> __entry->rcuname = rcuname;
> __entry->rhp = rhp;
> __entry->func = rhp->func;
> - __entry->qlen_lazy = qlen_lazy;
> __entry->qlen = qlen;
> ),
>
> - TP_printk("%s rhp=%p func=%ps %ld/%ld",
> + TP_printk("%s rhp=%p func=%ps %ld",
> __entry->rcuname, __entry->rhp, __entry->func,
> - __entry->qlen_lazy, __entry->qlen)
> + __entry->qlen)
> );
>
> /*
> @@ -511,15 +508,14 @@ TRACE_EVENT_RCU(rcu_callback,
> TRACE_EVENT_RCU(rcu_kfree_callback,
>
> TP_PROTO(const char *rcuname, struct rcu_head *rhp, unsigned long offset,
> - long qlen_lazy, long qlen),
> + long qlen),
>
> - TP_ARGS(rcuname, rhp, offset, qlen_lazy, qlen),
> + TP_ARGS(rcuname, rhp, offset, qlen),
>
> TP_STRUCT__entry(
> __field(const char *, rcuname)
> __field(void *, rhp)
> __field(unsigned long, offset)
> - __field(long, qlen_lazy)
> __field(long, qlen)
> ),
>
> @@ -527,13 +523,12 @@ TRACE_EVENT_RCU(rcu_kfree_callback,
> __entry->rcuname = rcuname;
> __entry->rhp = rhp;
> __entry->offset = offset;
> - __entry->qlen_lazy = qlen_lazy;
> __entry->qlen = qlen;
> ),
>
> - TP_printk("%s rhp=%p func=%ld %ld/%ld",
> + TP_printk("%s rhp=%p func=%ld %ld",
> __entry->rcuname, __entry->rhp, __entry->offset,
> - __entry->qlen_lazy, __entry->qlen)
> + __entry->qlen)
> );
>
> /*
> @@ -545,27 +540,24 @@ TRACE_EVENT_RCU(rcu_kfree_callback,
> */
> TRACE_EVENT_RCU(rcu_batch_start,
>
> - TP_PROTO(const char *rcuname, long qlen_lazy, long qlen, long blimit),
> + TP_PROTO(const char *rcuname, long qlen, long blimit),
>
> - TP_ARGS(rcuname, qlen_lazy, qlen, blimit),
> + TP_ARGS(rcuname, qlen, blimit),
>
> TP_STRUCT__entry(
> __field(const char *, rcuname)
> - __field(long, qlen_lazy)
> __field(long, qlen)
> __field(long, blimit)
> ),
>
> TP_fast_assign(
> __entry->rcuname = rcuname;
> - __entry->qlen_lazy = qlen_lazy;
> __entry->qlen = qlen;
> __entry->blimit = blimit;
> ),
>
> - TP_printk("%s CBs=%ld/%ld bl=%ld",
> - __entry->rcuname, __entry->qlen_lazy, __entry->qlen,
> - __entry->blimit)
> + TP_printk("%s CBs=%ld bl=%ld",
> + __entry->rcuname, __entry->qlen, __entry->blimit)
> );
>
> /*
> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> index aeec70fda82c..3f5a30ca7ed7 100644
> --- a/kernel/rcu/rcu.h
> +++ b/kernel/rcu/rcu.h
> @@ -198,33 +198,6 @@ static inline void debug_rcu_head_unqueue(struct rcu_head *head)
> }
> #endif /* #else !CONFIG_DEBUG_OBJECTS_RCU_HEAD */
>
> -void kfree(const void *);
> -
> -/*
> - * Reclaim the specified callback, either by invoking it (non-lazy case)
> - * or freeing it directly (lazy case). Return true if lazy, false otherwise.
> - */
> -static inline bool __rcu_reclaim(const char *rn, struct rcu_head *head)
> -{
> - rcu_callback_t f;
> - unsigned long offset = (unsigned long)head->func;
> -
> - rcu_lock_acquire(&rcu_callback_map);
> - if (__is_kfree_rcu_offset(offset)) {
> - trace_rcu_invoke_kfree_callback(rn, head, offset);
> - kfree((void *)head - offset);
> - rcu_lock_release(&rcu_callback_map);
> - return true;
> - } else {
> - trace_rcu_invoke_callback(rn, head);
> - f = head->func;
> - WRITE_ONCE(head->func, (rcu_callback_t)0L);
> - f(head);
> - rcu_lock_release(&rcu_callback_map);
> - return false;
> - }
> -}
Nice to see this one go! ;-)
> #ifdef CONFIG_RCU_STALL_COMMON
>
> extern int rcu_cpu_stall_ftrace_dump;
> diff --git a/kernel/rcu/rcu_segcblist.c b/kernel/rcu/rcu_segcblist.c
> index cbc87b804db9..5f4fd3b8777c 100644
> --- a/kernel/rcu/rcu_segcblist.c
> +++ b/kernel/rcu/rcu_segcblist.c
> @@ -20,14 +20,10 @@ void rcu_cblist_init(struct rcu_cblist *rclp)
> rclp->head = NULL;
> rclp->tail = &rclp->head;
> rclp->len = 0;
> - rclp->len_lazy = 0;
> }
>
> /*
> * Enqueue an rcu_head structure onto the specified callback list.
> - * This function assumes that the callback is non-lazy because it
> - * is intended for use by no-CBs CPUs, which do not distinguish
> - * between lazy and non-lazy RCU callbacks.
> */
> void rcu_cblist_enqueue(struct rcu_cblist *rclp, struct rcu_head *rhp)
> {
> @@ -54,7 +50,6 @@ void rcu_cblist_flush_enqueue(struct rcu_cblist *drclp,
> else
> drclp->tail = &drclp->head;
> drclp->len = srclp->len;
> - drclp->len_lazy = srclp->len_lazy;
> if (!rhp) {
> rcu_cblist_init(srclp);
> } else {
> @@ -62,16 +57,12 @@ void rcu_cblist_flush_enqueue(struct rcu_cblist *drclp,
> srclp->head = rhp;
> srclp->tail = &rhp->next;
> WRITE_ONCE(srclp->len, 1);
> - srclp->len_lazy = 0;
> }
> }
>
> /*
> * Dequeue the oldest rcu_head structure from the specified callback
> - * list. This function assumes that the callback is non-lazy, but
> - * the caller can later invoke rcu_cblist_dequeued_lazy() if it
> - * finds otherwise (and if it cares about laziness). This allows
> - * different users to have different ways of determining laziness.
> + * list.
> */
> struct rcu_head *rcu_cblist_dequeue(struct rcu_cblist *rclp)
> {
> @@ -161,7 +152,6 @@ void rcu_segcblist_init(struct rcu_segcblist *rsclp)
> for (i = 0; i < RCU_CBLIST_NSEGS; i++)
> rsclp->tails[i] = &rsclp->head;
> rcu_segcblist_set_len(rsclp, 0);
> - rsclp->len_lazy = 0;
> rsclp->enabled = 1;
> }
>
> @@ -173,7 +163,6 @@ void rcu_segcblist_disable(struct rcu_segcblist *rsclp)
> {
> WARN_ON_ONCE(!rcu_segcblist_empty(rsclp));
> WARN_ON_ONCE(rcu_segcblist_n_cbs(rsclp));
> - WARN_ON_ONCE(rcu_segcblist_n_lazy_cbs(rsclp));
> rsclp->enabled = 0;
> }
>
> @@ -253,11 +242,9 @@ bool rcu_segcblist_nextgp(struct rcu_segcblist *rsclp, unsigned long *lp)
> * absolutely not OK for it to ever miss posting a callback.
> */
> void rcu_segcblist_enqueue(struct rcu_segcblist *rsclp,
> - struct rcu_head *rhp, bool lazy)
> + struct rcu_head *rhp)
> {
> rcu_segcblist_inc_len(rsclp);
> - if (lazy)
> - rsclp->len_lazy++;
> smp_mb(); /* Ensure counts are updated before callback is enqueued. */
> rhp->next = NULL;
> WRITE_ONCE(*rsclp->tails[RCU_NEXT_TAIL], rhp);
> @@ -275,15 +262,13 @@ void rcu_segcblist_enqueue(struct rcu_segcblist *rsclp,
> * period. You have been warned.
> */
> bool rcu_segcblist_entrain(struct rcu_segcblist *rsclp,
> - struct rcu_head *rhp, bool lazy)
> + struct rcu_head *rhp)
> {
> int i;
>
> if (rcu_segcblist_n_cbs(rsclp) == 0)
> return false;
> rcu_segcblist_inc_len(rsclp);
> - if (lazy)
> - rsclp->len_lazy++;
> smp_mb(); /* Ensure counts are updated before callback is entrained. */
> rhp->next = NULL;
> for (i = RCU_NEXT_TAIL; i > RCU_DONE_TAIL; i--)
> @@ -307,8 +292,6 @@ bool rcu_segcblist_entrain(struct rcu_segcblist *rsclp,
> void rcu_segcblist_extract_count(struct rcu_segcblist *rsclp,
> struct rcu_cblist *rclp)
> {
> - rclp->len_lazy += rsclp->len_lazy;
> - rsclp->len_lazy = 0;
> rclp->len = rcu_segcblist_xchg_len(rsclp, 0);
> }
>
> @@ -361,9 +344,7 @@ void rcu_segcblist_extract_pend_cbs(struct rcu_segcblist *rsclp,
> void rcu_segcblist_insert_count(struct rcu_segcblist *rsclp,
> struct rcu_cblist *rclp)
> {
> - rsclp->len_lazy += rclp->len_lazy;
> rcu_segcblist_add_len(rsclp, rclp->len);
> - rclp->len_lazy = 0;
> rclp->len = 0;
> }
>
> diff --git a/kernel/rcu/rcu_segcblist.h b/kernel/rcu/rcu_segcblist.h
> index 815c2fdd3fcc..5c293afc07b8 100644
> --- a/kernel/rcu/rcu_segcblist.h
> +++ b/kernel/rcu/rcu_segcblist.h
> @@ -15,15 +15,6 @@ static inline long rcu_cblist_n_cbs(struct rcu_cblist *rclp)
> return READ_ONCE(rclp->len);
> }
>
> -/*
> - * Account for the fact that a previously dequeued callback turned out
> - * to be marked as lazy.
> - */
> -static inline void rcu_cblist_dequeued_lazy(struct rcu_cblist *rclp)
> -{
> - rclp->len_lazy--;
> -}
> -
> void rcu_cblist_init(struct rcu_cblist *rclp);
> void rcu_cblist_enqueue(struct rcu_cblist *rclp, struct rcu_head *rhp);
> void rcu_cblist_flush_enqueue(struct rcu_cblist *drclp,
> @@ -59,18 +50,6 @@ static inline long rcu_segcblist_n_cbs(struct rcu_segcblist *rsclp)
> #endif
> }
>
> -/* Return number of lazy callbacks in segmented callback list. */
> -static inline long rcu_segcblist_n_lazy_cbs(struct rcu_segcblist *rsclp)
> -{
> - return rsclp->len_lazy;
> -}
> -
> -/* Return number of lazy callbacks in segmented callback list. */
> -static inline long rcu_segcblist_n_nonlazy_cbs(struct rcu_segcblist *rsclp)
> -{
> - return rcu_segcblist_n_cbs(rsclp) - rsclp->len_lazy;
> -}
> -
> /*
> * Is the specified rcu_segcblist enabled, for example, not corresponding
> * to an offline CPU?
> @@ -106,9 +85,9 @@ struct rcu_head *rcu_segcblist_first_cb(struct rcu_segcblist *rsclp);
> struct rcu_head *rcu_segcblist_first_pend_cb(struct rcu_segcblist *rsclp);
> bool rcu_segcblist_nextgp(struct rcu_segcblist *rsclp, unsigned long *lp);
> void rcu_segcblist_enqueue(struct rcu_segcblist *rsclp,
> - struct rcu_head *rhp, bool lazy);
> + struct rcu_head *rhp);
> bool rcu_segcblist_entrain(struct rcu_segcblist *rsclp,
> - struct rcu_head *rhp, bool lazy);
> + struct rcu_head *rhp);
> void rcu_segcblist_extract_count(struct rcu_segcblist *rsclp,
> struct rcu_cblist *rclp);
> void rcu_segcblist_extract_done_cbs(struct rcu_segcblist *rsclp,
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 5dffade2d7cd..d0a9d5b69087 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -853,7 +853,7 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
> local_irq_save(flags);
> sdp = this_cpu_ptr(ssp->sda);
> spin_lock_rcu_node(sdp);
> - rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp, false);
> + rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
> rcu_segcblist_advance(&sdp->srcu_cblist,
> rcu_seq_current(&ssp->srcu_gp_seq));
> s = rcu_seq_snap(&ssp->srcu_gp_seq);
> @@ -1052,7 +1052,7 @@ void srcu_barrier(struct srcu_struct *ssp)
> sdp->srcu_barrier_head.func = srcu_barrier_cb;
> debug_rcu_head_queue(&sdp->srcu_barrier_head);
> if (!rcu_segcblist_entrain(&sdp->srcu_cblist,
> - &sdp->srcu_barrier_head, 0)) {
> + &sdp->srcu_barrier_head)) {
> debug_rcu_head_unqueue(&sdp->srcu_barrier_head);
> atomic_dec(&ssp->srcu_barrier_cpu_cnt);
> }
> diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c
> index 477b4eb44af5..cc64dc8b3893 100644
> --- a/kernel/rcu/tiny.c
> +++ b/kernel/rcu/tiny.c
> @@ -73,6 +73,33 @@ void rcu_sched_clock_irq(int user)
> }
> }
>
> +void kfree(const void *);
> +
> +/*
> + * Reclaim the specified callback, either by invoking it for non-kfree cases or
> + * freeing it directly (for kfree). Return true if kfreeing, false otherwise.
> + */
> +static inline bool rcu_reclaim_tiny(struct rcu_head *head)
> +{
> + rcu_callback_t f;
> + unsigned long offset = (unsigned long)head->func;
> +
> + rcu_lock_acquire(&rcu_callback_map);
> + if (__is_kfree_rcu_offset(offset)) {
> + trace_rcu_invoke_kfree_callback("", head, offset);
> + kfree((void *)head - offset);
> + rcu_lock_release(&rcu_callback_map);
> + return true;
> + }
> +
> + trace_rcu_invoke_callback("", head);
> + f = head->func;
> + WRITE_ONCE(head->func, (rcu_callback_t)0L);
> + f(head);
> + rcu_lock_release(&rcu_callback_map);
> + return false;
> +}
Even if it did show up here in the meantime. ;-)
> /* Invoke the RCU callbacks whose grace period has elapsed. */
> static __latent_entropy void rcu_process_callbacks(struct softirq_action *unused)
> {
> @@ -100,7 +127,7 @@ static __latent_entropy void rcu_process_callbacks(struct softirq_action *unused
> prefetch(next);
> debug_rcu_head_unqueue(list);
> local_bh_disable();
> - __rcu_reclaim("", list);
> + rcu_reclaim_tiny(list);
> local_bh_enable();
> list = next;
> }
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 64568f12641d..12c17e10f2b4 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2129,6 +2129,23 @@ int rcutree_dead_cpu(unsigned int cpu)
> return 0;
> }
>
> +/*
> + * Reclaim the specified callback.
> + */
> +static inline void rcu_reclaim_tree(const char *rn, struct rcu_head *head)
> +{
> + rcu_callback_t f;
> +
> + rcu_lock_acquire(&rcu_callback_map);
> + trace_rcu_invoke_callback(rn, head);
> +
> + f = head->func;
> + WRITE_ONCE(head->func, (rcu_callback_t)0L);
> + f(head);
> +
> + rcu_lock_release(&rcu_callback_map);
> +}
Might be worth inlining this one, as you did for kfree_rcu().
> /*
> * Invoke any RCU callbacks that have made it to the end of their grace
> * period. Thottle as specified by rdp->blimit.
> @@ -2146,7 +2163,6 @@ static void rcu_do_batch(struct rcu_data *rdp)
> /* If no callbacks are ready, just return. */
> if (!rcu_segcblist_ready_cbs(&rdp->cblist)) {
> trace_rcu_batch_start(rcu_state.name,
> - rcu_segcblist_n_lazy_cbs(&rdp->cblist),
> rcu_segcblist_n_cbs(&rdp->cblist), 0);
> trace_rcu_batch_end(rcu_state.name, 0,
> !rcu_segcblist_empty(&rdp->cblist),
> @@ -2168,7 +2184,6 @@ static void rcu_do_batch(struct rcu_data *rdp)
> if (unlikely(bl > 100))
> tlimit = local_clock() + rcu_resched_ns;
> trace_rcu_batch_start(rcu_state.name,
> - rcu_segcblist_n_lazy_cbs(&rdp->cblist),
> rcu_segcblist_n_cbs(&rdp->cblist), bl);
> rcu_segcblist_extract_done_cbs(&rdp->cblist, &rcl);
> if (offloaded)
> @@ -2181,8 +2196,8 @@ static void rcu_do_batch(struct rcu_data *rdp)
> rhp = rcu_cblist_dequeue(&rcl);
> for (; rhp; rhp = rcu_cblist_dequeue(&rcl)) {
> debug_rcu_head_unqueue(rhp);
> - if (__rcu_reclaim(rcu_state.name, rhp))
> - rcu_cblist_dequeued_lazy(&rcl);
> + rcu_reclaim_tree(rcu_state.name, rhp);
> +
> /*
> * Stop only if limit reached and CPU has something to do.
> * Note: The rcl structure counts down from zero.
> @@ -2585,7 +2600,7 @@ static void rcu_leak_callback(struct rcu_head *rhp)
> * is expected to specify a CPU.
> */
> static void
> -__call_rcu(struct rcu_head *head, rcu_callback_t func, bool lazy)
> +__call_rcu(struct rcu_head *head, rcu_callback_t func)
> {
> unsigned long flags;
> struct rcu_data *rdp;
> @@ -2620,18 +2635,17 @@ __call_rcu(struct rcu_head *head, rcu_callback_t func, bool lazy)
> if (rcu_segcblist_empty(&rdp->cblist))
> rcu_segcblist_init(&rdp->cblist);
> }
> +
> if (rcu_nocb_try_bypass(rdp, head, &was_alldone, flags))
> return; // Enqueued onto ->nocb_bypass, so just leave.
> /* If we get here, rcu_nocb_try_bypass() acquired ->nocb_lock. */
> - rcu_segcblist_enqueue(&rdp->cblist, head, lazy);
> + rcu_segcblist_enqueue(&rdp->cblist, head);
> if (__is_kfree_rcu_offset((unsigned long)func))
> trace_rcu_kfree_callback(rcu_state.name, head,
> (unsigned long)func,
> - rcu_segcblist_n_lazy_cbs(&rdp->cblist),
> rcu_segcblist_n_cbs(&rdp->cblist));
> else
> trace_rcu_callback(rcu_state.name, head,
> - rcu_segcblist_n_lazy_cbs(&rdp->cblist),
> rcu_segcblist_n_cbs(&rdp->cblist));
>
> /* Go handle any RCU core processing required. */
> @@ -2681,7 +2695,7 @@ __call_rcu(struct rcu_head *head, rcu_callback_t func, bool lazy)
> */
> void call_rcu(struct rcu_head *head, rcu_callback_t func)
> {
> - __call_rcu(head, func, 0);
> + __call_rcu(head, func);
> }
> EXPORT_SYMBOL_GPL(call_rcu);
>
> @@ -2755,10 +2769,18 @@ static void kfree_rcu_work(struct work_struct *work)
> * access is Ok.
> */
> for (; head; head = next) {
> + unsigned long offset = (unsigned long)head->func;
> +
> next = head->next;
> - /* Could be possible to optimize with kfree_bulk in future */
> +
> debug_rcu_head_unqueue(head);
> - __rcu_reclaim(rcu_state.name, head);
> + rcu_lock_acquire(&rcu_callback_map);
> + trace_rcu_invoke_kfree_callback(rcu_state.name, head, offset);
> +
> + /* Could be possible to optimize with kfree_bulk in future */
> + kfree((void *)head - offset);
> +
> + rcu_lock_release(&rcu_callback_map);
> cond_resched_tasks_rcu_qs();
> }
> }
> @@ -2840,7 +2862,7 @@ static void kfree_rcu_monitor(struct work_struct *work)
> */
> void kfree_call_rcu_nobatch(struct rcu_head *head, rcu_callback_t func)
> {
> - __call_rcu(head, func, 1);
> + __call_rcu(head, func);
> }
> EXPORT_SYMBOL_GPL(kfree_call_rcu_nobatch);
>
> @@ -3094,7 +3116,7 @@ static void rcu_barrier_func(void *unused)
> debug_rcu_head_queue(&rdp->barrier_head);
> rcu_nocb_lock(rdp);
> WARN_ON_ONCE(!rcu_nocb_flush_bypass(rdp, NULL, jiffies));
> - if (rcu_segcblist_entrain(&rdp->cblist, &rdp->barrier_head, 0)) {
> + if (rcu_segcblist_entrain(&rdp->cblist, &rdp->barrier_head)) {
> atomic_inc(&rcu_state.barrier_cpu_count);
> } else {
> debug_rcu_head_unqueue(&rdp->barrier_head);
> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> index 055c31781d3a..15405420b40c 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -183,7 +183,6 @@ struct rcu_data {
> bool rcu_urgent_qs; /* GP old need light quiescent state. */
> bool rcu_forced_tick; /* Forced tick to provide QS. */
> #ifdef CONFIG_RCU_FAST_NO_HZ
> - bool all_lazy; /* All CPU's CBs lazy at idle start? */
> unsigned long last_accelerate; /* Last jiffy CBs were accelerated. */
> unsigned long last_advance_all; /* Last jiffy CBs were all advanced. */
> int tick_nohz_enabled_snap; /* Previously seen value from sysfs. */
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 2defc7fe74c3..0d0fa615c5fa 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -1274,21 +1274,15 @@ static void rcu_prepare_for_idle(void)
We now have only one preprocessor symbol (as opposed to the erroneous
"three proprocessor symbols" in my original).
> * number, be warned: Setting RCU_IDLE_GP_DELAY too high can hang your
> * system. And if you are -that- concerned about energy efficiency,
> * just power the system down and be done with it!
> - * RCU_IDLE_LAZY_GP_DELAY gives the number of jiffies that a CPU is
> - * permitted to sleep in dyntick-idle mode with only lazy RCU
> - * callbacks pending. Setting this too high can OOM your system.
> *
> * The values below work well in practice. If future workloads require
"value below works"
> * adjustment, they can be converted into kernel config parameters, though
> * making the state machine smarter might be a better option.
> */
> #define RCU_IDLE_GP_DELAY 4 /* Roughly one grace period. */
> -#define RCU_IDLE_LAZY_GP_DELAY (6 * HZ) /* Roughly six seconds. */
>
> static int rcu_idle_gp_delay = RCU_IDLE_GP_DELAY;
> module_param(rcu_idle_gp_delay, int, 0644);
> -static int rcu_idle_lazy_gp_delay = RCU_IDLE_LAZY_GP_DELAY;
> -module_param(rcu_idle_lazy_gp_delay, int, 0644);
>
> /*
> * Try to advance callbacks on the current CPU, but only if it has been
> @@ -1327,8 +1321,7 @@ static bool __maybe_unused rcu_try_advance_all_cbs(void)
> /*
> * Allow the CPU to enter dyntick-idle mode unless it has callbacks ready
> * to invoke. If the CPU has callbacks, try to advance them. Tell the
> - * caller to set the timeout based on whether or not there are non-lazy
> - * callbacks.
> + * caller about what to set the timeout.
> *
> * The caller must have disabled interrupts.
> */
> @@ -1354,25 +1347,19 @@ int rcu_needs_cpu(u64 basemono, u64 *nextevt)
> }
> rdp->last_accelerate = jiffies;
>
> - /* Request timer delay depending on laziness, and round. */
> - rdp->all_lazy = !rcu_segcblist_n_nonlazy_cbs(&rdp->cblist);
> - if (rdp->all_lazy) {
> - dj = round_jiffies(rcu_idle_lazy_gp_delay + jiffies) - jiffies;
> - } else {
> - dj = round_up(rcu_idle_gp_delay + jiffies,
> - rcu_idle_gp_delay) - jiffies;
> - }
> + /* Request timer and round. */
> + dj = round_up(rcu_idle_gp_delay + jiffies,
> + rcu_idle_gp_delay) - jiffies;
I bet that this now fits onto one line.
> +
> *nextevt = basemono + dj * TICK_NSEC;
> return 0;
> }
>
> /*
> - * Prepare a CPU for idle from an RCU perspective. The first major task
> - * is to sense whether nohz mode has been enabled or disabled via sysfs.
> - * The second major task is to check to see if a non-lazy callback has
> - * arrived at a CPU that previously had only lazy callbacks. The third
> - * major task is to accelerate (that is, assign grace-period numbers to)
> - * any recently arrived callbacks.
> + * Prepare a CPU for idle from an RCU perspective. The first major task is to
> + * sense whether nohz mode has been enabled or disabled via sysfs. The second
> + * major task is to accelerate (that is, assign grace-period numbers to) any
> + * recently arrived callbacks.
> *
> * The caller must have disabled interrupts.
> */
> @@ -1398,17 +1385,6 @@ static void rcu_prepare_for_idle(void)
> if (!tne)
> return;
>
> - /*
> - * If a non-lazy callback arrived at a CPU having only lazy
> - * callbacks, invoke RCU core for the side-effect of recalculating
> - * idle duration on re-entry to idle.
> - */
> - if (rdp->all_lazy && rcu_segcblist_n_nonlazy_cbs(&rdp->cblist)) {
> - rdp->all_lazy = false;
> - invoke_rcu_core();
> - return;
> - }
> -
> /*
> * If we have not yet accelerated this jiffy, accelerate all
> * callbacks on this CPU.
> diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h
> index 841ab43f3e60..67530c23c708 100644
> --- a/kernel/rcu/tree_stall.h
> +++ b/kernel/rcu/tree_stall.h
> @@ -263,11 +263,9 @@ static void print_cpu_stall_fast_no_hz(char *cp, int cpu)
> {
> struct rcu_data *rdp = &per_cpu(rcu_data, cpu);
>
> - sprintf(cp, "last_accelerate: %04lx/%04lx, Nonlazy posted: %c%c%c",
> + sprintf(cp, "last_accelerate: %04lx/%04lx dyntick_enabled: %d",
> rdp->last_accelerate & 0xffff, jiffies & 0xffff,
> - ".l"[rdp->all_lazy],
> - ".L"[!rcu_segcblist_n_nonlazy_cbs(&rdp->cblist)],
> - ".D"[!!rdp->tick_nohz_enabled_snap]);
> + !!rdp->tick_nohz_enabled_snap);
> }
>
> #else /* #ifdef CONFIG_RCU_FAST_NO_HZ */
> --
> 2.23.0.187.g17f5b7556c-goog
>
^ permalink raw reply
* Re: [PATCH 3/5] rcu/tree: Add support for debug_objects debugging for kfree_rcu()
From: Joel Fernandes @ 2019-08-28 21:43 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, byungchul.park, Josh Triplett, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, rcu, Steven Rostedt, kernel-team
In-Reply-To: <20190828213119.GY26530@linux.ibm.com>
On Wed, Aug 28, 2019 at 02:31:19PM -0700, Paul E. McKenney wrote:
> On Tue, Aug 27, 2019 at 03:01:57PM -0400, Joel Fernandes (Google) wrote:
> > Make use of RCU's debug_objects debugging support
> > (CONFIG_DEBUG_OBJECTS_RCU_HEAD) similar to call_rcu() and other flavors.
>
> Other flavors? Ah, call_srcu(), rcu_barrier(), and srcu_barrier(),
> right?
Yes.
> > We queue the object during the kfree_rcu() call and dequeue it during
> > reclaim.
> >
> > Tested that enabling CONFIG_DEBUG_OBJECTS_RCU_HEAD successfully detects
> > double kfree_rcu() calls.
> >
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> The code looks good!
thanks, does that mean you'll ack/apply it? :-P
- Joel
>
> Thanx, Paul
>
> > ---
> > kernel/rcu/tree.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 9b9ae4db1c2d..64568f12641d 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -2757,6 +2757,7 @@ static void kfree_rcu_work(struct work_struct *work)
> > for (; head; head = next) {
> > next = head->next;
> > /* Could be possible to optimize with kfree_bulk in future */
> > + debug_rcu_head_unqueue(head);
> > __rcu_reclaim(rcu_state.name, head);
> > cond_resched_tasks_rcu_qs();
> > }
> > @@ -2868,6 +2869,13 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> > if (rcu_scheduler_active != RCU_SCHEDULER_RUNNING)
> > return kfree_call_rcu_nobatch(head, func);
> >
> > + if (debug_rcu_head_queue(head)) {
> > + /* Probable double kfree_rcu() */
> > + WARN_ONCE(1, "kfree_call_rcu(): Double-freed call. rcu_head %p\n",
> > + head);
> > + return;
> > + }
> > +
> > head->func = func;
> >
> > local_irq_save(flags); /* For safely calling this_cpu_ptr(). */
> > --
> > 2.23.0.187.g17f5b7556c-goog
> >
^ permalink raw reply
* Re: [RFC v1 2/2] rcu/tree: Remove dynticks_nmi_nesting counter
From: Joel Fernandes @ 2019-08-28 21:42 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Frederic Weisbecker, Jonathan Corbet, Josh Triplett,
kernel-team, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
Mauro Carvalho Chehab, rcu, Steven Rostedt
In-Reply-To: <20190828211904.GX26530@linux.ibm.com>
On Wed, Aug 28, 2019 at 02:19:04PM -0700, Paul E. McKenney wrote:
> On Wed, Aug 28, 2019 at 05:05:25PM -0400, Joel Fernandes wrote:
> > On Wed, Aug 28, 2019 at 01:23:30PM -0700, Paul E. McKenney wrote:
> > > On Mon, Aug 26, 2019 at 09:33:54PM -0400, Joel Fernandes (Google) wrote:
> > > > The dynticks_nmi_nesting counter serves 4 purposes:
> > > >
> > > > (a) rcu_is_cpu_rrupt_from_idle() needs to be able to detect first
> > > > interrupt nesting level.
> > > >
> > > > (b) We need to detect half-interrupts till we are sure they're not an
> > > > issue. However, change the comparison to DYNTICK_IRQ_NONIDLE with 0.
> > > >
> > > > (c) When a quiescent state report is needed from a nohz_full CPU.
> > > > The nesting counter detects we are a first level interrupt.
> > > >
> > > > For (a) we can just use dyntick_nesting == 1 to determine this. Only the
> > > > outermost interrupt that interrupted an RCU-idle state can set it to 1.
> > > >
> > > > For (b), this warning condition has not occurred for several kernel
> > > > releases. But we still keep the warning but change it to use
> > > > in_interrupt() instead of the nesting counter. In a later year, we can
> > > > remove the warning.
> > > >
> > > > For (c), the nest check is not really necessary since forced_tick would
> > > > have been set to true in the outermost interrupt, so the nested/NMI
> > > > interrupts will check forced_tick anyway, and bail.
> > >
> > > Skipping the commit log and documentation for this pass.
> > [snip]
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index 255cd6835526..1465a3e406f8 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -81,7 +81,6 @@
> > > >
> > > > static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_data) = {
> > > > .dynticks_nesting = 1,
> > > > - .dynticks_nmi_nesting = 0,
> > >
> > > This should be in the previous patch, give or take naming.
> >
> > Done.
> >
> > > > .dynticks = ATOMIC_INIT(RCU_DYNTICK_CTRL_CTR),
> > > > };
> > > > struct rcu_state rcu_state = {
> > > > @@ -392,15 +391,9 @@ static int rcu_is_cpu_rrupt_from_idle(void)
> > > > /* Check for counter underflows */
> > > > RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nesting) < 0,
> > > > "RCU dynticks_nesting counter underflow!");
> > > > - RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0,
> > > > - "RCU dynticks_nmi_nesting counter underflow/zero!");
> > > >
> > > > - /* Are we at first interrupt nesting level? */
> > > > - if (__this_cpu_read(rcu_data.dynticks_nmi_nesting) != 1)
> > > > - return false;
> > > > -
> > > > - /* Does CPU appear to be idle from an RCU standpoint? */
> > > > - return __this_cpu_read(rcu_data.dynticks_nesting) == 0;
> > > > + /* Are we the outermost interrupt that arrived when RCU was idle? */
> > > > + return __this_cpu_read(rcu_data.dynticks_nesting) == 1;
> > > > }
> > > >
> > > > #define DEFAULT_RCU_BLIMIT 10 /* Maximum callbacks per rcu_do_batch ... */
> > > > @@ -564,11 +557,10 @@ static void rcu_eqs_enter(bool user)
> > > > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > >
> > > > /* Entering usermode/idle from interrupt is not handled. These would
> > > > - * mean usermode upcalls or idle entry happened from interrupts. But,
> > > > - * reset the counter if we warn.
> > > > + * mean usermode upcalls or idle exit happened from interrupts. Remove
> > > > + * the warning by 2020.
> > > > */
> > > > - if (WARN_ON_ONCE(rdp->dynticks_nmi_nesting != 0))
> > > > - WRITE_ONCE(rdp->dynticks_nmi_nesting, 0);
> > > > + WARN_ON_ONCE(in_interrupt());
> > >
> > > And this is a red flag. Bad things happen should some common code
> > > that disables BH be invoked from the idle loop. This might not be
> > > happening now, but we need to avoid this sort of constraint.
> > > How about instead merging ->dyntick_nesting into the low-order bits
> > > of ->dyntick_nmi_nesting?
> > >
> > > Yes, this assumes that we don't enter process level twice, but it should
> > > be easy to add a WARN_ON() to test for that. Except that we don't have
> > > to because there is already this near the end of rcu_eqs_exit():
> > >
> > > WARN_ON_ONCE(rdp->dynticks_nmi_nesting);
> > >
> > > So the low-order bit of the combined counter could indicate process-level
> > > non-idle, the next three bits could be unused to make interpretation
> > > of hex printouts easier, and then the rest of the bits could be used in
> > > the same way as currently.
> > >
> > > This would allow a single read to see the full state, so that 0x1 means
> > > at process level in the kernel, 0x11 is interrupt (or NMI) from process
> > > level, 0x10 is interrupt/NMI from idle/user, and so on.
> > >
> > > What am I missing here? Why wouldn't this work, and without adding yet
> > > another RCU-imposed constraint on some other subsystem?
> >
> > What about replacing the warning with a WARN_ON_ONCE(in_irq()), would that
> > address your concern?
> >
> > Also, considering this warning condition is most likely never occurring as we
> > know it, and we are considering deleting it soon enough, is it really worth
> > reimplementing the whole mechanism with a complex bit-sharing scheme just
> > because of the BH-disable condition you mentioned, which likely doesn't
> > happen today? In my implementation, this is just a simple counter. I feel
> > combining bits in the same counter will just introduce more complexity that
> > this patch tries to address/avoid.
> >
> > OTOH, I also don't mind with just deleting the warning altogether if you are
> > Ok with that.
>
> The big advantage of combining the counters is that all of the state is
> explicit and visible in one place. Plus it can be accessed atomically.
> And it avoids setting a time bomb for some poor guys just trying to get
> their idle-loop jobs done some time in the dim distant future.
I could try the approach you're suggesting but I didn't actually see an issue
with the patch in its current state other than the WARN_ON_ONCE which I could
change to WARN_ON_ONCE(in_irq()) to remove the concern. AFAICS, we don't
detect "half soft-interrupts" in this code in anyway.
I do feel the approach you're suggesting can be a follow up, these 2 patches
just focus on deleting dynticks_nmi_nesting counter and we can test this
approach thoroughly for a release or so.
> Besides, this pair of patches already makes a large change from a
> conceptual viewpoint. If we are going to make a large change, let's
> get our money's worth out of that change!
IMHO, most of the changes are to code comments, the actual code change is
very little and is just removal of dynticks_nmi_nesting and simplification;
its not really an introduction of a new mechanism.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH 3/5] rcu/tree: Add support for debug_objects debugging for kfree_rcu()
From: Paul E. McKenney @ 2019-08-28 21:31 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, byungchul.park, Josh Triplett, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, rcu, Steven Rostedt, kernel-team
In-Reply-To: <5d657e37.1c69fb81.54250.01df@mx.google.com>
On Tue, Aug 27, 2019 at 03:01:57PM -0400, Joel Fernandes (Google) wrote:
> Make use of RCU's debug_objects debugging support
> (CONFIG_DEBUG_OBJECTS_RCU_HEAD) similar to call_rcu() and other flavors.
Other flavors? Ah, call_srcu(), rcu_barrier(), and srcu_barrier(),
right?
> We queue the object during the kfree_rcu() call and dequeue it during
> reclaim.
>
> Tested that enabling CONFIG_DEBUG_OBJECTS_RCU_HEAD successfully detects
> double kfree_rcu() calls.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
The code looks good!
Thanx, Paul
> ---
> kernel/rcu/tree.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 9b9ae4db1c2d..64568f12641d 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2757,6 +2757,7 @@ static void kfree_rcu_work(struct work_struct *work)
> for (; head; head = next) {
> next = head->next;
> /* Could be possible to optimize with kfree_bulk in future */
> + debug_rcu_head_unqueue(head);
> __rcu_reclaim(rcu_state.name, head);
> cond_resched_tasks_rcu_qs();
> }
> @@ -2868,6 +2869,13 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> if (rcu_scheduler_active != RCU_SCHEDULER_RUNNING)
> return kfree_call_rcu_nobatch(head, func);
>
> + if (debug_rcu_head_queue(head)) {
> + /* Probable double kfree_rcu() */
> + WARN_ONCE(1, "kfree_call_rcu(): Double-freed call. rcu_head %p\n",
> + head);
> + return;
> + }
> +
> head->func = func;
>
> local_irq_save(flags); /* For safely calling this_cpu_ptr(). */
> --
> 2.23.0.187.g17f5b7556c-goog
>
^ permalink raw reply
* Re: [PATCH 0/5] kfree_rcu() additions for -rcu
From: Joel Fernandes @ 2019-08-28 21:26 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, byungchul.park, Josh Triplett, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, rcu, Steven Rostedt, kernel-team
In-Reply-To: <20190828204624.GV26530@linux.ibm.com>
On Wed, Aug 28, 2019 at 01:46:24PM -0700, Paul E. McKenney wrote:
> On Wed, Aug 28, 2019 at 04:34:58PM -0400, Joel Fernandes wrote:
> > On Wed, Aug 28, 2019 at 01:28:08PM -0700, Paul E. McKenney wrote:
> > > On Tue, Aug 27, 2019 at 03:01:54PM -0400, Joel Fernandes (Google) wrote:
> > > > Hi,
> > > >
> > > > This is a series on top of the patch "rcu/tree: Add basic support for kfree_rcu() batching".
> > > >
> > > > Link: http://lore.kernel.org/r/20190814160411.58591-1-joel@joelfernandes.org
> > > >
> > > > It adds performance tests, some clean ups and removal of "lazy" RCU callbacks.
> > > >
> > > > Now that kfree_rcu() is handled separately from call_rcu(), we also get rid of
> > > > kfree "lazy" handling from tree RCU as suggested by Paul which will be unused.
> > > > This also results in a nice negative delta as well.
> > > >
> > > > Joel Fernandes (Google) (5):
> > > > rcu/rcuperf: Add kfree_rcu() performance Tests
> > > > rcu/tree: Add multiple in-flight batches of kfree_rcu work
> > > > rcu/tree: Add support for debug_objects debugging for kfree_rcu()
> > > > rcu: Remove kfree_rcu() special casing and lazy handling
> > > > rcu: Remove kfree_call_rcu_nobatch()
> > > >
> > > > Documentation/RCU/stallwarn.txt | 13 +-
> > > > .../admin-guide/kernel-parameters.txt | 13 ++
> > > > include/linux/rcu_segcblist.h | 2 -
> > > > include/linux/rcutiny.h | 5 -
> > > > include/linux/rcutree.h | 1 -
> > > > include/trace/events/rcu.h | 32 ++--
> > > > kernel/rcu/rcu.h | 27 ---
> > > > kernel/rcu/rcu_segcblist.c | 25 +--
> > > > kernel/rcu/rcu_segcblist.h | 25 +--
> > > > kernel/rcu/rcuperf.c | 173 +++++++++++++++++-
> > > > kernel/rcu/srcutree.c | 4 +-
> > > > kernel/rcu/tiny.c | 29 ++-
> > > > kernel/rcu/tree.c | 145 ++++++++++-----
> > > > kernel/rcu/tree.h | 1 -
> > > > kernel/rcu/tree_plugin.h | 42 +----
> > > > kernel/rcu/tree_stall.h | 6 +-
> > > > 16 files changed, 337 insertions(+), 206 deletions(-)
> > >
> > > Looks like a 131-line positive delta to me. ;-)
> >
> > Not if you overlook the rcuperf changes which is just test code. :-D ;-)
>
> Which suggests that you should move the "nice negative delta" comment
> to the commits that actually have nice negative deltas. ;-)
Will do!
thanks,
- Joel
^ permalink raw reply
* Re: [RFC v1 2/2] rcu/tree: Remove dynticks_nmi_nesting counter
From: Paul E. McKenney @ 2019-08-28 21:19 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Frederic Weisbecker, Jonathan Corbet, Josh Triplett,
kernel-team, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
Mauro Carvalho Chehab, rcu, Steven Rostedt
In-Reply-To: <20190828210525.GB75931@google.com>
On Wed, Aug 28, 2019 at 05:05:25PM -0400, Joel Fernandes wrote:
> On Wed, Aug 28, 2019 at 01:23:30PM -0700, Paul E. McKenney wrote:
> > On Mon, Aug 26, 2019 at 09:33:54PM -0400, Joel Fernandes (Google) wrote:
> > > The dynticks_nmi_nesting counter serves 4 purposes:
> > >
> > > (a) rcu_is_cpu_rrupt_from_idle() needs to be able to detect first
> > > interrupt nesting level.
> > >
> > > (b) We need to detect half-interrupts till we are sure they're not an
> > > issue. However, change the comparison to DYNTICK_IRQ_NONIDLE with 0.
> > >
> > > (c) When a quiescent state report is needed from a nohz_full CPU.
> > > The nesting counter detects we are a first level interrupt.
> > >
> > > For (a) we can just use dyntick_nesting == 1 to determine this. Only the
> > > outermost interrupt that interrupted an RCU-idle state can set it to 1.
> > >
> > > For (b), this warning condition has not occurred for several kernel
> > > releases. But we still keep the warning but change it to use
> > > in_interrupt() instead of the nesting counter. In a later year, we can
> > > remove the warning.
> > >
> > > For (c), the nest check is not really necessary since forced_tick would
> > > have been set to true in the outermost interrupt, so the nested/NMI
> > > interrupts will check forced_tick anyway, and bail.
> >
> > Skipping the commit log and documentation for this pass.
> [snip]
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 255cd6835526..1465a3e406f8 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -81,7 +81,6 @@
> > >
> > > static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_data) = {
> > > .dynticks_nesting = 1,
> > > - .dynticks_nmi_nesting = 0,
> >
> > This should be in the previous patch, give or take naming.
>
> Done.
>
> > > .dynticks = ATOMIC_INIT(RCU_DYNTICK_CTRL_CTR),
> > > };
> > > struct rcu_state rcu_state = {
> > > @@ -392,15 +391,9 @@ static int rcu_is_cpu_rrupt_from_idle(void)
> > > /* Check for counter underflows */
> > > RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nesting) < 0,
> > > "RCU dynticks_nesting counter underflow!");
> > > - RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0,
> > > - "RCU dynticks_nmi_nesting counter underflow/zero!");
> > >
> > > - /* Are we at first interrupt nesting level? */
> > > - if (__this_cpu_read(rcu_data.dynticks_nmi_nesting) != 1)
> > > - return false;
> > > -
> > > - /* Does CPU appear to be idle from an RCU standpoint? */
> > > - return __this_cpu_read(rcu_data.dynticks_nesting) == 0;
> > > + /* Are we the outermost interrupt that arrived when RCU was idle? */
> > > + return __this_cpu_read(rcu_data.dynticks_nesting) == 1;
> > > }
> > >
> > > #define DEFAULT_RCU_BLIMIT 10 /* Maximum callbacks per rcu_do_batch ... */
> > > @@ -564,11 +557,10 @@ static void rcu_eqs_enter(bool user)
> > > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > >
> > > /* Entering usermode/idle from interrupt is not handled. These would
> > > - * mean usermode upcalls or idle entry happened from interrupts. But,
> > > - * reset the counter if we warn.
> > > + * mean usermode upcalls or idle exit happened from interrupts. Remove
> > > + * the warning by 2020.
> > > */
> > > - if (WARN_ON_ONCE(rdp->dynticks_nmi_nesting != 0))
> > > - WRITE_ONCE(rdp->dynticks_nmi_nesting, 0);
> > > + WARN_ON_ONCE(in_interrupt());
> >
> > And this is a red flag. Bad things happen should some common code
> > that disables BH be invoked from the idle loop. This might not be
> > happening now, but we need to avoid this sort of constraint.
> > How about instead merging ->dyntick_nesting into the low-order bits
> > of ->dyntick_nmi_nesting?
> >
> > Yes, this assumes that we don't enter process level twice, but it should
> > be easy to add a WARN_ON() to test for that. Except that we don't have
> > to because there is already this near the end of rcu_eqs_exit():
> >
> > WARN_ON_ONCE(rdp->dynticks_nmi_nesting);
> >
> > So the low-order bit of the combined counter could indicate process-level
> > non-idle, the next three bits could be unused to make interpretation
> > of hex printouts easier, and then the rest of the bits could be used in
> > the same way as currently.
> >
> > This would allow a single read to see the full state, so that 0x1 means
> > at process level in the kernel, 0x11 is interrupt (or NMI) from process
> > level, 0x10 is interrupt/NMI from idle/user, and so on.
> >
> > What am I missing here? Why wouldn't this work, and without adding yet
> > another RCU-imposed constraint on some other subsystem?
>
> What about replacing the warning with a WARN_ON_ONCE(in_irq()), would that
> address your concern?
>
> Also, considering this warning condition is most likely never occurring as we
> know it, and we are considering deleting it soon enough, is it really worth
> reimplementing the whole mechanism with a complex bit-sharing scheme just
> because of the BH-disable condition you mentioned, which likely doesn't
> happen today? In my implementation, this is just a simple counter. I feel
> combining bits in the same counter will just introduce more complexity that
> this patch tries to address/avoid.
>
> OTOH, I also don't mind with just deleting the warning altogether if you are
> Ok with that.
The big advantage of combining the counters is that all of the state is
explicit and visible in one place. Plus it can be accessed atomically.
And it avoids setting a time bomb for some poor guys just trying to get
their idle-loop jobs done some time in the dim distant future.
Besides, this pair of patches already makes a large change from a
conceptual viewpoint. If we are going to make a large change, let's
get our money's worth out of that change!
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 1/5] rcu/rcuperf: Add kfree_rcu() performance Tests
From: Paul E. McKenney @ 2019-08-28 21:12 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, byungchul.park, Josh Triplett, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, rcu, Steven Rostedt, kernel-team
In-Reply-To: <5d657e33.1c69fb81.54250.01dd@mx.google.com>
On Tue, Aug 27, 2019 at 03:01:55PM -0400, Joel Fernandes (Google) wrote:
> This test runs kfree_rcu() in a loop to measure performance of the new
> kfree_rcu() batching functionality.
>
> The following table shows results when booting with arguments:
> rcuperf.kfree_loops=20000 rcuperf.kfree_alloc_num=8000 rcuperf.kfree_rcu_test=1
>
> In addition, rcuperf.kfree_no_batch is used to toggle the batching of
> kfree_rcu()s for a test run.
>
> patch applied GPs time (seconds)
> yes 1732 14.5
> no 9133 11.5
This is really "rcuperf.kfree_no_batch" rather than "patch applied", right?
(Yes, we did discuss this last time around, but this table combined with
the prior paragraph is still ambiguous.) Please make it unambiguous.
One way to do that is as follows:
------------------------------------------------------------------------
The following table shows results when booting with arguments:
rcuperf.kfree_loops=20000 rcuperf.kfree_alloc_num=8000 rcuperf.kfree_rcu_test=1 rcuperf.kfree_no_batch=X
rcuperf.kfree_no_batch=X # Grace Periods Test Duration (s)
X=1 (old behavior) 9133 11.5
X=0 (new behavior) 1732 14.5
------------------------------------------------------------------------
> On a 16 CPU system with the above boot parameters, we see that the total
> number of grace periods that elapse during the test drops from 9133 when
> not batching to 1732 when batching (a 5X improvement). The kfree_rcu()
> flood itself slows down a bit when batching, though, as shown.
This last sentence would be more clear as something like: "However,
use of batching increases the duration of the kfree_rcu()-flood test."
> Note that the active memory consumption during the kfree_rcu() flood
> does increase to around 200-250MB due to the batching (from around 50MB
> without batching). However, this memory consumption is relatively
> constant. In other words, the system is able to keep up with the
> kfree_rcu() load. The memory consumption comes down considerably if
> KFREE_DRAIN_JIFFIES is increased from HZ/50 to HZ/80.
That would be a decrease rather than an increase in KFREE_DRAIN_JIFFIES,
correct?
This would also be a good place to mention that a later patch will
decrease consumption, but that is strictly optional. However, you did
introduce the topic of changing KFREE_DRAIN_JIFFIES, so if a later patch
changes this value, this would be an excellent place to mention this.
> Also, when running the test, please disable CONFIG_DEBUG_PREEMPT and
> CONFIG_PROVE_RCU for realistic comparisons with/without batching.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
And the code is much better! Just one duplication-avoidance nit below.
Plus a thought for a future patch.
Thanx, Paul
> ---
> .../admin-guide/kernel-parameters.txt | 17 ++
> kernel/rcu/rcuperf.c | 181 +++++++++++++++++-
> 2 files changed, 190 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 79b983bedcaa..24fe8aefb12c 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3896,6 +3896,23 @@
> test until boot completes in order to avoid
> interference.
>
> + rcuperf.kfree_rcu_test= [KNL]
> + Set to measure performance of kfree_rcu() flooding.
> +
> + rcuperf.kfree_nthreads= [KNL]
> + The number of threads running loops of kfree_rcu().
> +
> + rcuperf.kfree_alloc_num= [KNL]
> + Number of allocations and frees done in an iteration.
> +
> + rcuperf.kfree_loops= [KNL]
> + Number of loops doing rcuperf.kfree_alloc_num number
> + of allocations and frees.
> +
> + rcuperf.kfree_no_batch= [KNL]
> + Use the non-batching (less efficient) version of kfree_rcu().
> + This is useful for comparing with the batched version.
> +
> rcuperf.nreaders= [KNL]
> Set number of RCU readers. The value -1 selects
> N, where N is the number of CPUs. A value
> diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c
> index 5f884d560384..c1e25fd10f2a 100644
> --- a/kernel/rcu/rcuperf.c
> +++ b/kernel/rcu/rcuperf.c
> @@ -86,6 +86,7 @@ torture_param(bool, shutdown, RCUPERF_SHUTDOWN,
> "Shutdown at end of performance tests.");
> torture_param(int, verbose, 1, "Enable verbose debugging printk()s");
> torture_param(int, writer_holdoff, 0, "Holdoff (us) between GPs, zero to disable");
> +torture_param(int, kfree_rcu_test, 0, "Do we run a kfree_rcu() perf test?");
>
> static char *perf_type = "rcu";
> module_param(perf_type, charp, 0444);
> @@ -105,8 +106,8 @@ static atomic_t n_rcu_perf_writer_finished;
> static wait_queue_head_t shutdown_wq;
> static u64 t_rcu_perf_writer_started;
> static u64 t_rcu_perf_writer_finished;
> -static unsigned long b_rcu_perf_writer_started;
> -static unsigned long b_rcu_perf_writer_finished;
> +static unsigned long b_rcu_gp_test_started;
> +static unsigned long b_rcu_gp_test_finished;
> static DEFINE_PER_CPU(atomic_t, n_async_inflight);
>
> #define MAX_MEAS 10000
> @@ -378,10 +379,10 @@ rcu_perf_writer(void *arg)
> if (atomic_inc_return(&n_rcu_perf_writer_started) >= nrealwriters) {
> t_rcu_perf_writer_started = t;
> if (gp_exp) {
> - b_rcu_perf_writer_started =
> + b_rcu_gp_test_started =
> cur_ops->exp_completed() / 2;
> } else {
> - b_rcu_perf_writer_started = cur_ops->get_gp_seq();
> + b_rcu_gp_test_started = cur_ops->get_gp_seq();
> }
> }
>
> @@ -429,10 +430,10 @@ rcu_perf_writer(void *arg)
> PERFOUT_STRING("Test complete");
> t_rcu_perf_writer_finished = t;
> if (gp_exp) {
> - b_rcu_perf_writer_finished =
> + b_rcu_gp_test_finished =
> cur_ops->exp_completed() / 2;
> } else {
> - b_rcu_perf_writer_finished =
> + b_rcu_gp_test_finished =
> cur_ops->get_gp_seq();
> }
> if (shutdown) {
> @@ -515,8 +516,8 @@ rcu_perf_cleanup(void)
> t_rcu_perf_writer_finished -
> t_rcu_perf_writer_started,
> ngps,
> - rcuperf_seq_diff(b_rcu_perf_writer_finished,
> - b_rcu_perf_writer_started));
> + rcuperf_seq_diff(b_rcu_gp_test_finished,
> + b_rcu_gp_test_started));
> for (i = 0; i < nrealwriters; i++) {
> if (!writer_durations)
> break;
> @@ -584,6 +585,167 @@ rcu_perf_shutdown(void *arg)
> return -EINVAL;
> }
>
> +/*
> + * kfree_rcu() performance tests: Start a kfree_rcu() loop on all CPUs for number
> + * of iterations and measure total time and number of GP for all iterations to complete.
> + */
> +
> +torture_param(int, kfree_nthreads, -1, "Number of threads running loops of kfree_rcu().");
> +torture_param(int, kfree_alloc_num, 8000, "Number of allocations and frees done in an iteration.");
> +torture_param(int, kfree_loops, 10, "Number of loops doing kfree_alloc_num allocations and frees.");
> +torture_param(int, kfree_no_batch, 0, "Use the non-batching (slower) version of kfree_rcu().");
> +
> +static struct task_struct **kfree_reader_tasks;
> +static int kfree_nrealthreads;
> +static atomic_t n_kfree_perf_thread_started;
> +static atomic_t n_kfree_perf_thread_ended;
> +
> +struct kfree_obj {
> + char kfree_obj[8];
> + struct rcu_head rh;
> +};
> +
> +static int
> +kfree_perf_thread(void *arg)
> +{
> + int i, loop = 0;
> + long me = (long)arg;
> + struct kfree_obj *alloc_ptr;
> + u64 start_time, end_time;
> +
> + VERBOSE_PERFOUT_STRING("kfree_perf_thread task started");
> + set_cpus_allowed_ptr(current, cpumask_of(me % nr_cpu_ids));
> + set_user_nice(current, MAX_NICE);
> +
> + start_time = ktime_get_mono_fast_ns();
> +
> + if (atomic_inc_return(&n_kfree_perf_thread_started) >= kfree_nrealthreads) {
> + if (gp_exp)
> + b_rcu_gp_test_started = cur_ops->exp_completed() / 2;
At some point, it would be good to use the new grace-period
sequence-counter functions (rcuperf_seq_diff(), for example) instead of
the open-coded division by 2. I freely admit that you are just copying
my obsolete hack in this case, so not needed in this patch.
> + else
> + b_rcu_gp_test_started = cur_ops->get_gp_seq();
> + }
> +
> + do {
> + for (i = 0; i < kfree_alloc_num; i++) {
> + alloc_ptr = kmalloc(sizeof(struct kfree_obj), GFP_KERNEL);
> + if (!alloc_ptr)
> + return -ENOMEM;
> +
> + if (!kfree_no_batch) {
> + kfree_rcu(alloc_ptr, rh);
> + } else {
> + rcu_callback_t cb;
> +
> + cb = (rcu_callback_t)(unsigned long)offsetof(struct kfree_obj, rh);
> + kfree_call_rcu_nobatch(&(alloc_ptr->rh), cb);
> + }
> + }
Nice, much simpler than the earlier version!
> + cond_resched();
> + } while (!torture_must_stop() && ++loop < kfree_loops);
> +
> + if (atomic_inc_return(&n_kfree_perf_thread_ended) >= kfree_nrealthreads) {
> + end_time = ktime_get_mono_fast_ns();
> +
> + if (gp_exp)
> + b_rcu_gp_test_finished = cur_ops->exp_completed() / 2;
Same here on open-coded division by 2.
> + else
> + b_rcu_gp_test_finished = cur_ops->get_gp_seq();
> +
> + pr_alert("Total time taken by all kfree'ers: %llu ns, loops: %d, batches: %ld\n",
> + (unsigned long long)(end_time - start_time), kfree_loops,
> + rcuperf_seq_diff(b_rcu_gp_test_finished, b_rcu_gp_test_started));
> + if (shutdown) {
> + smp_mb(); /* Assign before wake. */
> + wake_up(&shutdown_wq);
> + }
> + }
> +
> + torture_kthread_stopping("kfree_perf_thread");
> + return 0;
> +}
> +
> +static void
> +kfree_perf_cleanup(void)
> +{
> + int i;
> +
> + if (torture_cleanup_begin())
> + return;
> +
> + if (kfree_reader_tasks) {
> + for (i = 0; i < kfree_nrealthreads; i++)
> + torture_stop_kthread(kfree_perf_thread,
> + kfree_reader_tasks[i]);
> + kfree(kfree_reader_tasks);
> + }
> +
> + torture_cleanup_end();
> +}
> +
> +/*
> + * shutdown kthread. Just waits to be awakened, then shuts down system.
> + */
> +static int
> +kfree_perf_shutdown(void *arg)
> +{
> + do {
> + wait_event(shutdown_wq,
> + atomic_read(&n_kfree_perf_thread_ended) >=
> + kfree_nrealthreads);
> + } while (atomic_read(&n_kfree_perf_thread_ended) < kfree_nrealthreads);
> +
> + smp_mb(); /* Wake before output. */
> +
> + kfree_perf_cleanup();
> + kernel_power_off();
> + return -EINVAL;
These last four lines should be combined with those of
rcu_perf_shutdown(). Actually, you could fold the two functions together
with only a pair of arguments and two one-line wrapper functions, which
would be even better.
> +}
> +
> +static int __init
> +kfree_perf_init(void)
> +{
> + long i;
> + int firsterr = 0;
> +
> + kfree_nrealthreads = compute_real(kfree_nthreads);
> + /* Start up the kthreads. */
> + if (shutdown) {
> + init_waitqueue_head(&shutdown_wq);
> + firsterr = torture_create_kthread(kfree_perf_shutdown, NULL,
> + shutdown_task);
> + if (firsterr)
> + goto unwind;
> + schedule_timeout_uninterruptible(1);
> + }
> +
> + kfree_reader_tasks = kcalloc(kfree_nrealthreads, sizeof(kfree_reader_tasks[0]),
> + GFP_KERNEL);
> + if (kfree_reader_tasks == NULL) {
> + firsterr = -ENOMEM;
> + goto unwind;
> + }
> +
> + for (i = 0; i < kfree_nrealthreads; i++) {
> + firsterr = torture_create_kthread(kfree_perf_thread, (void *)i,
> + kfree_reader_tasks[i]);
> + if (firsterr)
> + goto unwind;
> + }
> +
> + while (atomic_read(&n_kfree_perf_thread_started) < kfree_nrealthreads)
> + schedule_timeout_uninterruptible(1);
> +
> + torture_init_end();
> + return 0;
> +
> +unwind:
> + torture_init_end();
> + kfree_perf_cleanup();
> + return firsterr;
> +}
> +
> static int __init
> rcu_perf_init(void)
> {
> @@ -616,6 +778,9 @@ rcu_perf_init(void)
> if (cur_ops->init)
> cur_ops->init();
>
> + if (kfree_rcu_test)
> + return kfree_perf_init();
> +
> nrealwriters = compute_real(nwriters);
> nrealreaders = compute_real(nreaders);
> atomic_set(&n_rcu_perf_reader_started, 0);
> --
> 2.23.0.187.g17f5b7556c-goog
>
^ permalink raw reply
* Re: [RFC v1 2/2] rcu/tree: Remove dynticks_nmi_nesting counter
From: Joel Fernandes @ 2019-08-28 21:05 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Frederic Weisbecker, Jonathan Corbet, Josh Triplett,
kernel-team, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
Mauro Carvalho Chehab, rcu, Steven Rostedt
In-Reply-To: <20190828202330.GS26530@linux.ibm.com>
On Wed, Aug 28, 2019 at 01:23:30PM -0700, Paul E. McKenney wrote:
> On Mon, Aug 26, 2019 at 09:33:54PM -0400, Joel Fernandes (Google) wrote:
> > The dynticks_nmi_nesting counter serves 4 purposes:
> >
> > (a) rcu_is_cpu_rrupt_from_idle() needs to be able to detect first
> > interrupt nesting level.
> >
> > (b) We need to detect half-interrupts till we are sure they're not an
> > issue. However, change the comparison to DYNTICK_IRQ_NONIDLE with 0.
> >
> > (c) When a quiescent state report is needed from a nohz_full CPU.
> > The nesting counter detects we are a first level interrupt.
> >
> > For (a) we can just use dyntick_nesting == 1 to determine this. Only the
> > outermost interrupt that interrupted an RCU-idle state can set it to 1.
> >
> > For (b), this warning condition has not occurred for several kernel
> > releases. But we still keep the warning but change it to use
> > in_interrupt() instead of the nesting counter. In a later year, we can
> > remove the warning.
> >
> > For (c), the nest check is not really necessary since forced_tick would
> > have been set to true in the outermost interrupt, so the nested/NMI
> > interrupts will check forced_tick anyway, and bail.
>
> Skipping the commit log and documentation for this pass.
[snip]
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 255cd6835526..1465a3e406f8 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -81,7 +81,6 @@
> >
> > static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_data) = {
> > .dynticks_nesting = 1,
> > - .dynticks_nmi_nesting = 0,
>
> This should be in the previous patch, give or take naming.
Done.
> > .dynticks = ATOMIC_INIT(RCU_DYNTICK_CTRL_CTR),
> > };
> > struct rcu_state rcu_state = {
> > @@ -392,15 +391,9 @@ static int rcu_is_cpu_rrupt_from_idle(void)
> > /* Check for counter underflows */
> > RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nesting) < 0,
> > "RCU dynticks_nesting counter underflow!");
> > - RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0,
> > - "RCU dynticks_nmi_nesting counter underflow/zero!");
> >
> > - /* Are we at first interrupt nesting level? */
> > - if (__this_cpu_read(rcu_data.dynticks_nmi_nesting) != 1)
> > - return false;
> > -
> > - /* Does CPU appear to be idle from an RCU standpoint? */
> > - return __this_cpu_read(rcu_data.dynticks_nesting) == 0;
> > + /* Are we the outermost interrupt that arrived when RCU was idle? */
> > + return __this_cpu_read(rcu_data.dynticks_nesting) == 1;
> > }
> >
> > #define DEFAULT_RCU_BLIMIT 10 /* Maximum callbacks per rcu_do_batch ... */
> > @@ -564,11 +557,10 @@ static void rcu_eqs_enter(bool user)
> > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> >
> > /* Entering usermode/idle from interrupt is not handled. These would
> > - * mean usermode upcalls or idle entry happened from interrupts. But,
> > - * reset the counter if we warn.
> > + * mean usermode upcalls or idle exit happened from interrupts. Remove
> > + * the warning by 2020.
> > */
> > - if (WARN_ON_ONCE(rdp->dynticks_nmi_nesting != 0))
> > - WRITE_ONCE(rdp->dynticks_nmi_nesting, 0);
> > + WARN_ON_ONCE(in_interrupt());
>
> And this is a red flag. Bad things happen should some common code
> that disables BH be invoked from the idle loop. This might not be
> happening now, but we need to avoid this sort of constraint.
> How about instead merging ->dyntick_nesting into the low-order bits
> of ->dyntick_nmi_nesting?
>
> Yes, this assumes that we don't enter process level twice, but it should
> be easy to add a WARN_ON() to test for that. Except that we don't have
> to because there is already this near the end of rcu_eqs_exit():
>
> WARN_ON_ONCE(rdp->dynticks_nmi_nesting);
>
> So the low-order bit of the combined counter could indicate process-level
> non-idle, the next three bits could be unused to make interpretation
> of hex printouts easier, and then the rest of the bits could be used in
> the same way as currently.
>
> This would allow a single read to see the full state, so that 0x1 means
> at process level in the kernel, 0x11 is interrupt (or NMI) from process
> level, 0x10 is interrupt/NMI from idle/user, and so on.
>
> What am I missing here? Why wouldn't this work, and without adding yet
> another RCU-imposed constraint on some other subsystem?
What about replacing the warning with a WARN_ON_ONCE(in_irq()), would that
address your concern?
Also, considering this warning condition is most likely never occurring as we
know it, and we are considering deleting it soon enough, is it really worth
reimplementing the whole mechanism with a complex bit-sharing scheme just
because of the BH-disable condition you mentioned, which likely doesn't
happen today? In my implementation, this is just a simple counter. I feel
combining bits in the same counter will just introduce more complexity that
this patch tries to address/avoid.
OTOH, I also don't mind with just deleting the warning altogether if you are
Ok with that.
thanks!
- Joel
^ permalink raw reply
* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Mark Brown @ 2019-08-28 20:51 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, Andrew Morton,
Jani Nikula, Enrico Weigelt, metux IT consult, Jonathan Corbet,
linux-doc, linux-kernel, Alexandre Belloni
In-Reply-To: <ebd9e2f6-a6f7-b3df-480d-cd6b818fb065@kleine-koenig.org>
[-- Attachment #1: Type: text/plain, Size: 1423 bytes --]
On Wed, Aug 28, 2019 at 09:59:16PM +0200, Uwe Kleine-König wrote:
> On 8/28/19 1:32 PM, Petr Mladek wrote:
> > Please, do the opposite. Add conversion of few subsystems into the
> > patchset and add more people into CC. We will see immediately whether
> > it makes sense to spend time on this.
> For now I asked in the arm linux irc channel and got two people replying
> (both added to Cc:):
> Mark Brown (maintainer of SPI, regmap, ASoC and regulator) said:
> 1567019926 < broonie> ukleinek: I think that's a great idea and have
> thought about trying to implement it in the past.
> 1567019937 < broonie> ukleinek: Making the logs more directly readable
> is enormously helpful.
In the past I've actually implemented error code decoders like this for
some other systems I've worked on due to annoyance with having to look
up codes, as far as I can tell they went over quite well with other
developers and I certainly found them helpful myself. They're also
useful to end users looking at logs, they're not always aware of how to
find the relevant error code definitions so a slightly more descriptive
messages can help people figure things out.
It does also occur to me that this might be useful to the people who
want to work on making probe deferral quieter since it gives them an
annotation that the number going in is an error code. There's a bunch
more work to do there though.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 0/5] kfree_rcu() additions for -rcu
From: Paul E. McKenney @ 2019-08-28 20:46 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, byungchul.park, Josh Triplett, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, rcu, Steven Rostedt, kernel-team
In-Reply-To: <20190828203458.GA75931@google.com>
On Wed, Aug 28, 2019 at 04:34:58PM -0400, Joel Fernandes wrote:
> On Wed, Aug 28, 2019 at 01:28:08PM -0700, Paul E. McKenney wrote:
> > On Tue, Aug 27, 2019 at 03:01:54PM -0400, Joel Fernandes (Google) wrote:
> > > Hi,
> > >
> > > This is a series on top of the patch "rcu/tree: Add basic support for kfree_rcu() batching".
> > >
> > > Link: http://lore.kernel.org/r/20190814160411.58591-1-joel@joelfernandes.org
> > >
> > > It adds performance tests, some clean ups and removal of "lazy" RCU callbacks.
> > >
> > > Now that kfree_rcu() is handled separately from call_rcu(), we also get rid of
> > > kfree "lazy" handling from tree RCU as suggested by Paul which will be unused.
> > > This also results in a nice negative delta as well.
> > >
> > > Joel Fernandes (Google) (5):
> > > rcu/rcuperf: Add kfree_rcu() performance Tests
> > > rcu/tree: Add multiple in-flight batches of kfree_rcu work
> > > rcu/tree: Add support for debug_objects debugging for kfree_rcu()
> > > rcu: Remove kfree_rcu() special casing and lazy handling
> > > rcu: Remove kfree_call_rcu_nobatch()
> > >
> > > Documentation/RCU/stallwarn.txt | 13 +-
> > > .../admin-guide/kernel-parameters.txt | 13 ++
> > > include/linux/rcu_segcblist.h | 2 -
> > > include/linux/rcutiny.h | 5 -
> > > include/linux/rcutree.h | 1 -
> > > include/trace/events/rcu.h | 32 ++--
> > > kernel/rcu/rcu.h | 27 ---
> > > kernel/rcu/rcu_segcblist.c | 25 +--
> > > kernel/rcu/rcu_segcblist.h | 25 +--
> > > kernel/rcu/rcuperf.c | 173 +++++++++++++++++-
> > > kernel/rcu/srcutree.c | 4 +-
> > > kernel/rcu/tiny.c | 29 ++-
> > > kernel/rcu/tree.c | 145 ++++++++++-----
> > > kernel/rcu/tree.h | 1 -
> > > kernel/rcu/tree_plugin.h | 42 +----
> > > kernel/rcu/tree_stall.h | 6 +-
> > > 16 files changed, 337 insertions(+), 206 deletions(-)
> >
> > Looks like a 131-line positive delta to me. ;-)
>
> Not if you overlook the rcuperf changes which is just test code. :-D ;-)
Which suggests that you should move the "nice negative delta" comment
to the commits that actually have nice negative deltas. ;-)
Thanx, Paul
^ permalink raw reply
* Re: [PATCH v2] rcu/tree: Add multiple in-flight batches of kfree_rcu work
From: Paul E. McKenney @ 2019-08-28 20:45 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, byungchul.park, Josh Triplett, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, rcu, Steven Rostedt, kernel-team
In-Reply-To: <20190828140952.258739-1-joel@joelfernandes.org>
On Wed, Aug 28, 2019 at 10:09:52AM -0400, Joel Fernandes (Google) wrote:
> During testing, it was observed that amount of memory consumed due
> kfree_rcu() batching is 300-400MB. Previously we had only a single
> head_free pointer pointing to the list of rcu_head(s) that are to be
> freed after a grace period. Until this list is drained, we cannot queue
> any more objects on it since such objects may not be ready to be
> reclaimed when the worker thread eventually gets to drainin g the
> head_free list.
>
> We can do better by maintaining multiple lists as done by this patch.
> Testing shows that memory consumption came down by around 100-150MB with
> just adding another list. Adding more than 1 additional list did not
> show any improvement.
Nice! A few comments below. Please address them and post a full v3.
(I am off the next two days, and I guarantee you that upon return I will
mix and match the wrong patches otherwise!)
> Suggested-by: Paul E. McKenney <paulmck@linux.ibm.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> kernel/rcu/tree.c | 61 ++++++++++++++++++++++++++++++++---------------
> 1 file changed, 42 insertions(+), 19 deletions(-)
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 4f7c3096d786..5bf8f7e793ea 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2688,28 +2688,37 @@ EXPORT_SYMBOL_GPL(call_rcu);
>
> /* Maximum number of jiffies to wait before draining a batch. */
> #define KFREE_DRAIN_JIFFIES (HZ / 50)
> +#define KFREE_N_BATCHES 2
> +
> +struct kfree_rcu_work {
> + /* The rcu_work node for queuing work with queue_rcu_work(). The work
> + * is done after a grace period.
> + */
> + struct rcu_work rcu_work;
> +
> + /* The list of objects that have now left ->head and are queued for
> + * freeing after a grace period.
> + */
> + struct rcu_head *head_free;
> +
> + struct kfree_rcu_cpu *krcp;
> +};
>
> /*
> * Maximum number of kfree(s) to batch, if this limit is hit then the batch of
> * kfree(s) is queued for freeing after a grace period, right away.
> */
> struct kfree_rcu_cpu {
> - /* The rcu_work node for queuing work with queue_rcu_work(). The work
> - * is done after a grace period.
> - */
> - struct rcu_work rcu_work;
>
> /* The list of objects being queued in a batch but are not yet
> * scheduled to be freed.
> */
> struct rcu_head *head;
>
> - /* The list of objects that have now left ->head and are queued for
> - * freeing after a grace period.
> - */
> - struct rcu_head *head_free;
> + /* Pointer to the per-cpu array of kfree_rcu_work structures */
> + struct kfree_rcu_work krw_arr[KFREE_N_BATCHES];
>
> - /* Protect concurrent access to this structure. */
> + /* Protect concurrent access to this structure and kfree_rcu_work. */
> spinlock_t lock;
>
> /* The delayed work that flushes ->head to ->head_free incase ->head
> @@ -2730,12 +2739,14 @@ static void kfree_rcu_work(struct work_struct *work)
> {
> unsigned long flags;
> struct rcu_head *head, *next;
> - struct kfree_rcu_cpu *krcp = container_of(to_rcu_work(work),
> - struct kfree_rcu_cpu, rcu_work);
> + struct kfree_rcu_work *krwp = container_of(to_rcu_work(work),
> + struct kfree_rcu_work, rcu_work);
> + struct kfree_rcu_cpu *krcp;
> +
> + krcp = krwp->krcp;
>
> spin_lock_irqsave(&krcp->lock, flags);
> - head = krcp->head_free;
> - krcp->head_free = NULL;
> + head = xchg(&krwp->head_free, NULL);
Given that we hold the lock, why the xchg()? Alternatively, why not
just acquire the lock in the other places you use xchg()? This is a
per-CPU lock, so contention should not be a problem, should it?
> spin_unlock_irqrestore(&krcp->lock, flags);
>
> /*
> @@ -2758,19 +2769,28 @@ static void kfree_rcu_work(struct work_struct *work)
> */
> static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
> {
> + int i = 0;
> + struct kfree_rcu_work *krwp = NULL;
> +
> lockdep_assert_held(&krcp->lock);
> + while (i < KFREE_N_BATCHES) {
> + if (!krcp->krw_arr[i].head_free) {
> + krwp = &(krcp->krw_arr[i]);
> + break;
> + }
> + i++;
> + }
>
> - /* If a previous RCU batch work is already in progress, we cannot queue
> + /* If both RCU batches are already in progress, we cannot queue
> * another one, just refuse the optimization and it will be retried
> * again in KFREE_DRAIN_JIFFIES time.
> */
If you are going to remove the traditional first "/*" line of a comment,
why not go all the way and cut the last one as well? "//".
> - if (krcp->head_free)
> + if (!krwp)
> return false;
>
> - krcp->head_free = krcp->head;
> - krcp->head = NULL;
> - INIT_RCU_WORK(&krcp->rcu_work, kfree_rcu_work);
> - queue_rcu_work(system_wq, &krcp->rcu_work);
> + krwp->head_free = xchg(&krcp->head, NULL);
This isn't anywhere near a fastpath, so just acquiring the lock is a
better choice here.
> + INIT_RCU_WORK(&krwp->rcu_work, kfree_rcu_work);
> + queue_rcu_work(system_wq, &krwp->rcu_work);
>
> return true;
> }
> @@ -3736,8 +3756,11 @@ static void __init kfree_rcu_batch_init(void)
>
> for_each_possible_cpu(cpu) {
> struct kfree_rcu_cpu *krcp = per_cpu_ptr(&krc, cpu);
> + int i = KFREE_N_BATCHES;
>
> spin_lock_init(&krcp->lock);
> + while (i--)
> + krcp->krw_arr[i].krcp = krcp;
This was indeed a nice trick back in the PDP-11 days of 64-kilobyte
address spaces, so thank you for the nostalgia! However, a straight-up
"for" loop is less vulnerable to code being added between the declaration
of "i" and the "while" loop.
> INIT_DELAYED_WORK(&krcp->monitor_work, kfree_rcu_monitor);
> }
> }
> --
> 2.23.0.187.g17f5b7556c-goog
>
^ permalink raw reply
* Re: [PATCH 0/5] kfree_rcu() additions for -rcu
From: Joel Fernandes @ 2019-08-28 20:34 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, byungchul.park, Josh Triplett, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, rcu, Steven Rostedt, kernel-team
In-Reply-To: <20190828202808.GT26530@linux.ibm.com>
On Wed, Aug 28, 2019 at 01:28:08PM -0700, Paul E. McKenney wrote:
> On Tue, Aug 27, 2019 at 03:01:54PM -0400, Joel Fernandes (Google) wrote:
> > Hi,
> >
> > This is a series on top of the patch "rcu/tree: Add basic support for kfree_rcu() batching".
> >
> > Link: http://lore.kernel.org/r/20190814160411.58591-1-joel@joelfernandes.org
> >
> > It adds performance tests, some clean ups and removal of "lazy" RCU callbacks.
> >
> > Now that kfree_rcu() is handled separately from call_rcu(), we also get rid of
> > kfree "lazy" handling from tree RCU as suggested by Paul which will be unused.
> > This also results in a nice negative delta as well.
> >
> > Joel Fernandes (Google) (5):
> > rcu/rcuperf: Add kfree_rcu() performance Tests
> > rcu/tree: Add multiple in-flight batches of kfree_rcu work
> > rcu/tree: Add support for debug_objects debugging for kfree_rcu()
> > rcu: Remove kfree_rcu() special casing and lazy handling
> > rcu: Remove kfree_call_rcu_nobatch()
> >
> > Documentation/RCU/stallwarn.txt | 13 +-
> > .../admin-guide/kernel-parameters.txt | 13 ++
> > include/linux/rcu_segcblist.h | 2 -
> > include/linux/rcutiny.h | 5 -
> > include/linux/rcutree.h | 1 -
> > include/trace/events/rcu.h | 32 ++--
> > kernel/rcu/rcu.h | 27 ---
> > kernel/rcu/rcu_segcblist.c | 25 +--
> > kernel/rcu/rcu_segcblist.h | 25 +--
> > kernel/rcu/rcuperf.c | 173 +++++++++++++++++-
> > kernel/rcu/srcutree.c | 4 +-
> > kernel/rcu/tiny.c | 29 ++-
> > kernel/rcu/tree.c | 145 ++++++++++-----
> > kernel/rcu/tree.h | 1 -
> > kernel/rcu/tree_plugin.h | 42 +----
> > kernel/rcu/tree_stall.h | 6 +-
> > 16 files changed, 337 insertions(+), 206 deletions(-)
>
> Looks like a 131-line positive delta to me. ;-)
Not if you overlook the rcuperf changes which is just test code. :-D ;-)
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH 0/5] kfree_rcu() additions for -rcu
From: Paul E. McKenney @ 2019-08-28 20:28 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, byungchul.park, Josh Triplett, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, rcu, Steven Rostedt, kernel-team
In-Reply-To: <5d657e30.1c69fb81.54250.01dc@mx.google.com>
On Tue, Aug 27, 2019 at 03:01:54PM -0400, Joel Fernandes (Google) wrote:
> Hi,
>
> This is a series on top of the patch "rcu/tree: Add basic support for kfree_rcu() batching".
>
> Link: http://lore.kernel.org/r/20190814160411.58591-1-joel@joelfernandes.org
>
> It adds performance tests, some clean ups and removal of "lazy" RCU callbacks.
>
> Now that kfree_rcu() is handled separately from call_rcu(), we also get rid of
> kfree "lazy" handling from tree RCU as suggested by Paul which will be unused.
> This also results in a nice negative delta as well.
>
> Joel Fernandes (Google) (5):
> rcu/rcuperf: Add kfree_rcu() performance Tests
> rcu/tree: Add multiple in-flight batches of kfree_rcu work
> rcu/tree: Add support for debug_objects debugging for kfree_rcu()
> rcu: Remove kfree_rcu() special casing and lazy handling
> rcu: Remove kfree_call_rcu_nobatch()
>
> Documentation/RCU/stallwarn.txt | 13 +-
> .../admin-guide/kernel-parameters.txt | 13 ++
> include/linux/rcu_segcblist.h | 2 -
> include/linux/rcutiny.h | 5 -
> include/linux/rcutree.h | 1 -
> include/trace/events/rcu.h | 32 ++--
> kernel/rcu/rcu.h | 27 ---
> kernel/rcu/rcu_segcblist.c | 25 +--
> kernel/rcu/rcu_segcblist.h | 25 +--
> kernel/rcu/rcuperf.c | 173 +++++++++++++++++-
> kernel/rcu/srcutree.c | 4 +-
> kernel/rcu/tiny.c | 29 ++-
> kernel/rcu/tree.c | 145 ++++++++++-----
> kernel/rcu/tree.h | 1 -
> kernel/rcu/tree_plugin.h | 42 +----
> kernel/rcu/tree_stall.h | 6 +-
> 16 files changed, 337 insertions(+), 206 deletions(-)
Looks like a 131-line positive delta to me. ;-)
Thanx, Paul
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox