Message-ID: <2b735ba4-8081-4ddb-9397-4fe83143d97f@paulmck-laptop>
Date: Mon, 19 Feb 2024 08:48:20 -0800
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Ankur Arora <ankur.a.arora@...cle.com>
Cc: linux-kernel@...r.kernel.org, tglx@...utronix.de, peterz@...radead.org,
	torvalds@...ux-foundation.org, akpm@...ux-foundation.org,
	luto@...nel.org, bp@...en8.de, dave.hansen@...ux.intel.com,
	hpa@...or.com, mingo@...hat.com, juri.lelli@...hat.com,
	vincent.guittot@...aro.org, willy@...radead.org, mgorman@...e.de,
	jpoimboe@...nel.org, mark.rutland@....com, jgross@...e.com,
	andrew.cooper3@...rix.com, bristot@...nel.org,
	mathieu.desnoyers@...icios.com, glaubitz@...sik.fu-berlin.de,
	anton.ivanov@...bridgegreys.com, mattst88@...il.com,
	krypton@...ich-teichert.org, rostedt@...dmis.org,
	David.Laight@...lab.com, richard@....at, jon.grimm@....com,
	bharata@....com, boris.ostrovsky@...cle.com, konrad.wilk@...cle.com
Subject: Re: [PATCH 00/30] PREEMPT_AUTO: support lazy rescheduling

On Sun, Feb 18, 2024 at 10:17:48AM -0800, Paul E. McKenney wrote:
> On Fri, Feb 16, 2024 at 07:59:45PM -0800, Ankur Arora wrote:
> > Paul E. McKenney <paulmck@...nel.org> writes:
> > > On Thu, Feb 15, 2024 at 06:59:25PM -0800, Paul E. McKenney wrote:
> > >> On Thu, Feb 15, 2024 at 04:45:17PM -0800, Ankur Arora wrote:
> > >> >
> > >> > Paul E. McKenney <paulmck@...nel.org> writes:
> > >> >
> > >> > > On Thu, Feb 15, 2024 at 01:24:59PM -0800, Ankur Arora wrote:
> > >> > >>
> > >> > >> Paul E. McKenney <paulmck@...nel.org> writes:
> > >> > >>
> > >> > >> > On Wed, Feb 14, 2024 at 07:45:18PM -0800, Paul E. McKenney wrote:
> > >> > >> >> On Wed, Feb 14, 2024 at 06:03:28PM -0800, Ankur Arora wrote:
> > >> > >> >> >
> > >> > >> >> > Paul E. McKenney <paulmck@...nel.org> writes:
> > >> > >> >> >
> > >> > >> >> > > On Mon, Feb 12, 2024 at 09:55:24PM -0800, Ankur Arora wrote:
> > >> > >> >> > >> Hi,
> > >> > >> >> > >>
> > >> > >> >> > >> This series adds a new scheduling model PREEMPT_AUTO, which like
> > >> > >> >> > >> PREEMPT_DYNAMIC allows dynamic switching between a none/voluntary/full
> > >> > >> >> > >> preemption model. However, unlike PREEMPT_DYNAMIC, it doesn't depend
> > >> > >> >> > >> on explicit preemption points for the voluntary models.
> > >> > >> >> > >>
> > >> > >> >> > >> The series is based on Thomas' original proposal which he outlined
> > >> > >> >> > >> in [1], [2] and in his PoC [3].
> > >> > >> >> > >>
> > >> > >> >> > >> An earlier RFC version is at [4].
> > >> > >> >> > >
> > >> > >> >> > > This uncovered a couple of latent bugs in RCU due to its having been
> > >> > >> >> > > a good long time since anyone built a !SMP preemptible kernel with
> > >> > >> >> > > non-preemptible RCU.  I have a couple of fixes queued on -rcu [1], most
> > >> > >> >> > > likely for the merge window after next, but let me know if you need
> > >> > >> >> > > them sooner.
> > >> > >> >> >
> > >> > >> >> > Thanks. As you can probably tell, I skipped out on !SMP in my testing.
> > >> > >> >> > But, the attached diff should tide me over until the fixes are in.
> > >> > >> >>
> > >> > >> >> That was indeed my guess.  ;-)
> > >> > >> >>
> > >> > >> >> > > I am also seeing OOM conditions during rcutorture testing of callback
> > >> > >> >> > > flooding, but I am still looking into this.
> > >> > >> >> >
> > >> > >> >> > That's on the PREEMPT_AUTO && PREEMPT_VOLUNTARY configuration?
> > >> > >> >>
> > >> > >> >> On two of the PREEMPT_AUTO && PREEMPT_NONE configurations, but only on
> > >> > >> >> two of them thus far.  I am running a longer test to see if this might
> > >> > >> >> be just luck.  If not, I look to see what rcutorture scenarios TREE10
> > >> > >> >> and TRACE01 have in common.
> > >> > >> >
> > >> > >> > And still TRACE01 and TREE10 are hitting OOMs, still not seeing what
> > >> > >> > sets them apart.  I also hit a grace-period hang in TREE04, which does
> > >> > >> > CONFIG_PREEMPT_VOLUNTARY=y along with CONFIG_PREEMPT_AUTO=y.  Something
> > >> > >> > to dig into more.
> > >> > >>
> > >> > >> So, the only PREEMPT_VOLUNTARY=y configuration is TREE04. I wonder
> > >> > >> if you would continue to hit the TREE04 hang with CONFIG_PREEMTP_NONE=y
> > >> > >> as well?
> > >> > >> (Just in the interest of minimizing configurations.)
> > >> > >
> > >> > > I would be happy to, but in the spirit of full disclosure...
> > >> > >
> > >> > > First, I have seen that failure only once, which is not enough to
> > >> > > conclude that it has much to do with TREE04.  It might simply be low
> > >> > > probability, so that TREE04 simply was unlucky enough to hit it first.
> > >> > > In contrast, I have sufficient data to be reasonably confident that the
> > >> > > callback-flooding OOMs really do have something to do with the TRACE01 and
> > >> > > TREE10 scenarios, even though I am not yet seeing what these two scenarios
> > >> > > have in common that they don't also have in common with other scenarios.
> > >> > > But what is life without a bit of mystery?  ;-)
> > >> >
> > >> > :).
> > >> >
> > >> > > Second, please see the attached tarball, which contains .csv files showing
> > >> > > Kconfig options and kernel boot parameters for the various torture tests.
> > >> > > The portions of the filenames preceding the "config.csv" correspond to
> > >> > > the directories in tools/testing/selftests/rcutorture/configs.
> > >> >
> > >> > So, at least some of the HZ_FULL=y tests don't run into problems.
> > >> >
> > >> > > Third, there are additional scenarios hand-crafted by the script at
> > >> > > tools/testing/selftests/rcutorture/bin/torture.sh.  Thus far, none of
> > >> > > them have triggered, other than via the newly increased difficulty
> > >> > > of configurating a tracing-free kernel with which to test, but they
> > >> > > can still be useful in ruling out particular Kconfig options or kernel
> > >> > > boot parameters being related to a given issue.
> > >> > >
> > >> > > But please do take a look at the .csv files and let me know what
> > >> > > adjustments would be appropriate given the failure information.
> > >> >
> > >> > Nothing stands out just yet. Let me start a run here and see if
> > >> > that gives me some ideas.
> > >>
> > >> Sounds good, thank you!
> > >>
> > >> > I'm guessing the splats don't give any useful information or
> > >> > you would have attached them ;).
> > >>
> > >> My plan is to extract what can be extracted from the overnight run
> > >> that I just started.  Just in case the fixes have any effect on things,
> > >> unlikely though that might be given those fixes and the runs that failed.
> > >
> > > And I only got no failures from either TREE10 or TRACE01 on last night's
> > > run.
> > 
> > Oh that's great news. Same for my overnight runs for TREE04 and TRACE01.
> > 
> > Ongoing: a 24 hour run for those. Let's see how that goes.
> > 
> > > I merged your series on top of v6.8-rc4 with the -rcu tree's
> > > dev branch, the latter to get the RCU fixes.  But this means that last
> > > night's results are not really comparable to earlier results.
> > >
> > > I did get a few TREE09 failures, but I get those anyway.  I took it
> > > apart below for you because I got confused and thought that it was a
> > > TREE10 failure.  So just in case you were curious what one of these
> > > looks like and because I am too lazy to delete it.  ;-)
> > 
> > Heh. Well, thanks for being lazy /after/ dissecting it nicely.
> > 
> > > So from the viewpoint of moderate rcutorture testing, this series
> > > looks good.  Woo hoo!!!
> > 
> > Awesome!
> > 
> > > We did uncover a separate issue with Tasks RCU, which I will report on
> > > in more detail separately.  However, this issue does not (repeat, *not*)
> > > affect lazy preemption as such, but instead any attempt to remove all
> > > of the cond_resched() invocations.
> > 
> > So, that sounds like it happens even with (CONFIG_PREEMPT_AUTO=n,
> > CONFIG_PREEMPT=y)?
> > Anyway will look out for it when you go into the detail.
> 
> Fair point, normally Tasks RCU isn't present when cond_resched()
> means anything.
> 
> I will look again -- it is quite possible that I was confused by earlier
> in-fleet setups that had Tasks RCU enabled even when preemption was
> disabled.  (We don't do that anymore, and, had I been paying sufficient
> attention, would not have been doing it to start with.  Back in the day,
> enabling rcutorture, even as a module, had the side effect of enabling
> Tasks RCU.  How else to test it, right?  Well...)

OK, I got my head straight on this one...

And the problem is in fact that Tasks RCU isn't normally present
in non-preemptible kernels.  This is because normal RCU will wait
for preemption-disabled regions of code, and in PREMPT_NONE and
PREEMPT_VOLUNTARY kernels, that includes pretty much any region of code
lacking an explicit schedule() or similar.  And as I understand it,
tracing trampolines rely on this implicit lack of preemption.

So, with lazy preemption, we could preempt in the middle of a
trampoline, and synchronize_rcu() won't save us.

Steve and Mathieu will correct me if I am wrong.

If I do understand this correctly, one workaround is to remove the
"if PREEMPTIBLE" on all occurrences of "select TASKS_RCU".  That way,
all kernels would use synchronize_rcu_tasks(), which would wait for
a voluntary context switch.

This workaround does increase the overhead and tracepoint-removal
latency on non-preemptible kernels, so it might be time to revisit the
synchronization of trampolines.  Unfortunately, the things I have come
up with thus far have disadvantages:

o	Keep a set of permanent trampolines that enter and exit
	some sort of explicit RCU read-side critical section.
	If the address for this trampoline to call is in a register,
	then these permanent trampolines remain constant so that
	no synchronization of them is required.  The selected
	flavor of RCU can then be used to deal with the non-permanent
	trampolines.

	The disadvantage here is a significant increase in the complexity
	and overhead of trampoline code and the code that invokes the
	trampolines.  This overhead limits where tracing may be used
	in the kernel, which is of course undesirable.

o	Check for being preempted within a trampoline, and track this
	within the tasks structure.  The disadvantage here is that this
	requires keeping track of all of the trampolines and adding a
	check for being in one on a scheduler fast path.

o	Have a variant of Tasks RCU which checks the stack of preempted
	tasks, waiting until all have been seen without being preempted
	in a trampoline.  This still requires keeping track of all the
	trampolines in an easy-to-search manner, but gets the overhead
	of searching off of the scheduler fastpaths.

	It is also necessary to check running tasks, which might have
	been interrupted from within a trampoline.

	I would have a hard time convincing myself that these return
	addresses were unconditionally reliable.  But maybe they are?

o	Your idea here!

Again, the short-term workaround is to remove the "if PREEMPTIBLE" from
all of the "select TASKS_RCU" clauses.

> > > My next step is to try this on bare metal on a system configured as
> > > is the fleet.  But good progress for a week!!!
> > 
> > Yeah this is great. Fingers crossed for the wider set of tests.
> 
> I got what might be a one-off when hitting rcutorture and KASAN harder.
> I am running 320*TRACE01 to see if it reproduces.

[ . . . ]

> So, first see if it is reproducible, second enable more diagnostics,
> third make more grace-period sequence numbers available to rcutorture,
> fourth recheck the diagnostics code, and then see where we go from there.
> It might be that lazy preemption needs adjustment, or it might be that
> it just tickled latent diagnostic issues in rcutorture.
> 
> (I rarely hit this WARN_ON() except in early development, when the
> problem is usually glaringly obvious, hence all the uncertainty.)

And it is eminently reproducible.  Digging into it...

							Thanx, Paul

Powered by blists - more mailing lists