Message-ID: <875z9m3xo7.fsf@nanos.tec.linutronix.de>
Date:   Fri, 14 Aug 2020 01:59:04 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     peterz@...radead.org, "Paul E. McKenney" <paulmck@...nel.org>
Cc:     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 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 :)

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.

  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.

  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

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

     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.

  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.

     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.

        RT tests showed max latency of cyclictest go up from 30 to 220
        microseconds, i.e. factor 7 just because of that.

        So not really great.

        It would have had the charm to keep the semantics of GFP_NOWAIT,
        but OTOH even if it would work I'm pretty oppposed to open the
        can of worm which allows allocations from the deepest atomic
        contexts in general without a really good reason.

     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.

        That of course does not solve the problem vs. lockdep.

        Also the idea to make this conditional on !preemptible() does
        not work because preemptible() returns always false for
        CONFIG_PREEMPT_COUNT=n kernels.

     C) I suggested to make GFP == 0 fail unconditionally when the
        lockless cache is empty, but that changes the semantics on !RT
        kernels for existing users which hand in 0.
        
     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.

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.     

> 2.	Adding a GFP_ flag.

  The simplest and most consistent solution. If you really need to do
  allocations from such contexts then deal with the limitations whether
  it's RT or not. Paul has no problem with that and this newfangled
  kfree_rcu() is the only user and can live with that restriction.

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

  My argument here is that consistency is the best solution and the
  extra GFP mode is explicitly restrictive due to the context which it
  is called from. Aside of that this affects exactly ONE use case which
  has a really good reason and does not care about the restriction even
  on !RT because in that situation kfree_rcu() performance is not the
  most urgent problem.

> 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.

 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.

Having a distinct GFP mode is technically correct, consistent on all
kernel variants and does not affect any exisiting user of the page
allocator aside of the new kfree_rcu().

I hope that the cloudy and rainy day cured most of my heat wave induced
brain damage to the extent that the above is correctly summarizing the
state of affairs. If not, then please yell and I get an icepack.

Thanks,

        tglx


     

     

Powered by blists - more mailing lists