Date:   Fri, 14 Aug 2020 10:30:37 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     "Paul E. McKenney" <paulmck@...nel.org>,
        Michal Hocko <mhocko@...e.com>,
        Uladzislau Rezki <urezki@...il.com>,
        LKML <linux-kernel@...r.kernel.org>, RCU <rcu@...r.kernel.org>,
        linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Matthew Wilcox <willy@...radead.org>,
        "Theodore Y . Ts'o" <tytso@....edu>,
        Joel Fernandes <joel@...lfernandes.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Oleksiy Avramchenko <oleksiy.avramchenko@...ymobile.com>
Subject: Re: [RFC-PATCH 1/2] mm: Add __GFP_NO_LOCKS flag

On Fri, Aug 14, 2020 at 01:59:04AM +0200, Thomas Gleixner wrote:
> On Fri, Aug 14 2020 at 00:06, peterz wrote:
> > I'm still not getting it, how do we end up trying to allocate memory
> > from under raw spinlocks if you're not allowed to use kfree_rcu() under
> > one ?
> >
> > Can someone please spell out the actual problem?
> 
> Your actual problem is the heat wave. Get an icepack already :)

Sure, but also much of the below wasn't stated anywhere in the thread I
got Cc'ed on. All I got was half arsed solutions without an actual
problem statement.

> To set things straight:
> 
>   1) kfree_rcu() which used to be just a conveniance wrapper around
>      call_rcu() always worked in any context except NMI.
> 
>      Otherwise RT would have never worked or would have needed other
>      horrible hacks to defer kfree in certain contexts including
>      context switch.

I've never used kfree_rcu(), htf would I know.

>   2) RCU grew an optimization for kfree_rcu() which avoids the linked
>      list cache misses when a large number of objects is freed via
>      kfree_rcu(). Paul says it speeds up post grace period processing by
>      a factor of 8 which is impressive.
> 
>      That's done by avoiding the linked list and storing the object
>      pointer in an array of pointers which is page sized. This array is
>      then freed in bulk without having to touch the rcu head linked list
>      which obviously avoids cache misses.
> 
>      Occasionally kfree_rcu() needs to allocate a page for this but it
>      can fallback to the linked list when the allocation fails.
>      
>      Inconveniantly this allocation happens with an RCU internal raw
>      lock held, but even if we could do the allocation outside the RCU
>      internal lock this would not solve the problem that kfree_rcu() can
>      be called in any context except NMI even on RT.
> 
>      So RT forbids allocations from truly atomic contexts even with
>      GFP_ATOMIC/NOWAIT. GFP_ATOMIC is kinda meaningless on RT because
>      everything which calls it is in non-atomic context :) But still
>      GFP_ATOMIC or GFP_NOWAIT retain the semantics of !RT and do not go
>      down into deeper levels or wait for pages to become available.

Right so on RT just cut out the allocation and unconditionally make it
NULL. Since it needs to deal with GFP_ATOMIC|GFP_NOWARN failing the
allocation anyway.

>   3) #2 upsets lockdep (with the raw lock nesting enabled) rightfully
>      when RCU tries to allocate a page, the lockless page cache is empty
>      and it acquires zone->lock which is a regular spinlock

There was no lockdep splat in the thread.

>   4) As kfree_rcu() can be used from any context except NMI and RT
>      relies on it we ran into a circular dependency problem.

Well, which actual usage sites are under a raw spinlock? Most of the
ones I could find are not.

>      If we disable that feature for RT then the problem would be solved
>      except that lockdep still would complain about taking zone->lock
>      within a forbidden context on !RT kernels.
> 
>      Preventing that feature because of that is not a feasible option
>      either. Aside of that we discuss this postfactum, IOW the stuff is
>      upstream already despite the problem being known before.

Well, that's a fail :-( I tought we were supposed to solve known issues
before shit got merged.

>   5) Ulad posted patches which introduce a new GFP allocation mode
>      which makes the allocation fail if the lockless cache is empty,
>      i.e. it does not try to touch zone->lock in that case.
> 
>      That works perfectly fine on RT and !RT, makes lockdep happy and
>      Paul is happy as well.
> 
>      If the lockless cache, which works perfectly fine on RT, is empty
>      then the performance of kfree_rcu() post grace period processing is
>      probably not making the situation of the system worse.
> 
>      Michal is not fond of the new GFP flag and wants to explore options
>      to avoid that completely. But so far there is no real feasible
>      solution.

Not only Michal, those patches looked pretty horrid.

>      A) It was suggested to make zone->lock raw, but that's a total
>         disaster latency wise. With just a kernel compile (!RT kernel)
>         spinwait times are in the hundreds of microseconds.

Yeah, I know, been there done that, like over a decade ago :-)

>      B) Michal suggested to have GFP_RT_ATOMIC defined to 0 which would
>         not require a new GFP bit and bail out when RT is enabled and
>         the context is atomic.

I would've written the code like:

	void *mem = NULL;

	...
#ifndef CONFIG_PREEMPT_RT
	mem = kmalloc(PAGE_SIZE, GFP_ATOMIC|GFP_NOWAIT);
#endif
	if (!mem)
	  ....

Seems much simpler and doesn't pollute the GFP_ space.

>      D) To solve the lockdep issue of #B Michal suggested to have magic
>         lockdep annotations which allow !RT kernels to take zone->lock
>         from the otherwise forbidden contexts because on !RT this lock
>         nesting could be considered a false positive.
> 
>         But this would be horrors of some sorts because there might be
>         locks further down which then need the same treatment or some
>         general 'yay, I'm excempt from everything' annotation which is
>         short of disabling lockdep completly.
> 
>         Even if that could be solved and made correct for both RT and
>         !RT then this opens the can of worms that this kind of
>         annotation would show up all over the place within no time for
>         the completely wrong reasons.

So due to this heat crap I've not slept more than a few hours a night
for the last week, I'm not entirely there, but we already have a bunch
of lockdep magic for this raw nesting stuff.

But yes, we need to avoid abuse, grep for lockdep_off() :-( This
drivers/md/dm-cache-target.c thing is just plain broken. It sorta
'works' on mainline (and even there it can behave badly), but on RT it
will come apart with a bang.

> Paul compiled this options list:
> 
> > 1.	Prohibit invoking allocators from raw atomic context, such
> >	as when holding a raw spinlock.
> 
>   Clearly the simplest solution but not Pauls favourite and
>   unfortunately he has a good reason.

Which isn't actually stated anywhere I suppose ?

> > 2.	Adding a GFP_ flag.
> 
>   Michal does not like the restriction for !RT kernels and tries to
>   avoid the introduction of a new allocation mode.

Like above, I tend to be with Michal on this, just wrap the actual
allocation in CONFIG_PREEMPT_RT, the code needs to handle a NULL pointer
there anyway.

> > 3.	Reusing existing GFP_ flags/values/whatever to communicate
> >	the raw-context information that was to be communicated by
> >	the new GFP_ flag.
> >
> > 4.	Making lockdep forgive acquiring spinlocks while holding
> >	raw spinlocks, but only in CONFIG_PREEMPT_NONE=y kernels.

Uhh, !CONFIG_PREEMPT_RT, the rest is 'fine'.

>  These are not seperate of each other as #3 requires #4. The most
>  horrible solution IMO from a technical POV as it proliferates
>  inconsistency for no good reaosn.
> 
>  Aside of that it'd be solving a problem which does not exist simply
>  because kfree_rcu() does not depend on it and we really don't want to
>  set precedence and encourage the (ab)use of this in any way.

My preferred solution is 1, if you want to use an allocator, you simply
don't get to play under raw_spinlock_t. And from a quick grep, most
kfree_rcu() users are not under raw_spinlock_t context.

This here sounds like someone who wants to have his cake and eat it too.

I'll try and think about a lockdep annotation that isn't utter crap, but
that's probably next week, I need this heat to end and get a few nights
sleep, I'm an utter wreck atm.

Powered by blists - more mailing lists