Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Uladzislau Rezki <urezki@gmail.com>
Date:   Tue, 3 Nov 2020 17:33:50 +0100
To:     Joel Fernandes <joel@joelfernandes.org>
Cc:     "Uladzislau Rezki (Sony)" <urezki@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>, RCU <rcu@vger.kernel.org>,
        "Paul E . McKenney" <paulmck@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Michal Hocko <mhocko@suse.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        "Theodore Y . Ts'o" <tytso@mit.edu>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Oleksiy Avramchenko <oleksiy.avramchenko@sonymobile.com>,
        willy@infradead.org
Subject: Re: [PATCH 01/16] rcu/tree: Add a work to allocate pages from
 regular context
Message-ID: <20201103163350.GA10665@pc636>
References: <20201029165019.14218-1-urezki@gmail.com>
 <20201103154723.GA1310511@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201103154723.GA1310511@google.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Tue, Nov 03, 2020 at 10:47:23AM -0500, Joel Fernandes wrote:
> On Thu, Oct 29, 2020 at 05:50:04PM +0100, Uladzislau Rezki (Sony) wrote:
> > The current memmory-allocation interface presents to following
> > difficulties that this patch is designed to overcome:
> > 
> > a) If built with CONFIG_PROVE_RAW_LOCK_NESTING, the lockdep will
> >    complain about violation("BUG: Invalid wait context") of the
> >    nesting rules. It does the raw_spinlock vs. spinlock nesting
> >    checks, i.e. it is not legal to acquire a spinlock_t while
> >    holding a raw_spinlock_t.
> > 
> >    Internally the kfree_rcu() uses raw_spinlock_t whereas the
> >    "page allocator" internally deals with spinlock_t to access
> >    to its zones. The code also can be broken from higher level
> >    of view:
> >    <snip>
> >        raw_spin_lock(&some_lock);
> >        kfree_rcu(some_pointer, some_field_offset);
> >    <snip>
> > 
> > b) If built with CONFIG_PREEMPT_RT. Please note, in that case spinlock_t
> >    is converted into sleepable variant. Invoking the page allocator from
> >    atomic contexts leads to "BUG: scheduling while atomic".
> > 
> > c) call_rcu() is invoked from raw atomic context and kfree_rcu()
> >    and kvfree_rcu() are expected to be called from atomic raw context
> >    as well.
> > 
> > Move out a page allocation from contexts which trigger kvfree_rcu()
> > function to the separate worker. When a k[v]free_rcu() per-cpu page
> > cache is empty a fallback mechanism is used and a special job is
> > scheduled to refill the per-cpu cache.
> 
> Looks good, still reviewing here. BTW just for my education, I was wondering
> about Thomas's email:
> https://lkml.org/lkml/2020/8/11/939
> 
> If slab allocations in pure raw-atomic context on RT is not allowed or
> recommended, should kfree_rcu() be allowed?
>
Thanks for reviewing, Joel :)

The decision was made that we need to support kfree_rcu() from "real atomic contexts",
to align with how it used to be before. We can go and just convert our local locks
to the spinlock_t variant but that was not Paul goal, it can be that some users need
kfree_rcu() for raw atomics.

>
> slab can have same issue right? If per-cpu cache is drained, it has to
> allocate page from buddy allocator and there's no GFP flag to tell it about
> context where alloc is happening from.
> 
Sounds like that. Apart of that, it might turn out soon that we or somebody
else will rise a question one more time about something GFP_RAW or GFP_NOLOCKS.
So who knows..

>
> Or are we saying that we want to support kfree on RT from raw atomic atomic
> context, even though kmalloc is not supported? I hate to bring up this
> elephant in the room, but since I am a part of the people maintaining this
> code, I believe I would rather set some rules than supporting unsupported
> usages. :-\ (Once I know what is supported and what isn't that is). If indeed
> raw atomic kfree_rcu() is a bogus use case because of -RT, then we ought to
> put a giant warning than supporting it :-(.
> 
We discussed it several times, the conclusion was that we need to support 
kfree_rcu() from raw contexts. At least that was a clear signal from Paul 
to me. I think, if we obtain the preemtable(), so it becomes versatile, we
can drop the patch that is in question later on in the future.

--
Vlad Rezki