Re: [PATCH 1/3] rcu: Use static initializer for krc.lock

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2020-04-21 08:41:01 [-0700], Paul E. McKenney wrote:
> > Invoking queue_work() is not a problem locking wise. It does however
> > extend the length of the critical section (which is not yet needed).
> 
> I am guessing that by "which is not yet needed" you were meaning that
> it is not absolutely necessary to extend the length of the critical
> section.  Please correct me if my guess is wrong.

By changing the lock type you extend the atomic section from "only"
queue_work() (if invoked) to everything kfree_rcu() does plus
queue_work().

> In the meantime, plunging ahead...
> 
> One approach might be to move queue_work() and friends out of the critical
> section, but only enable interrupts between the two critical sections
> under CONFIG_PREEMPT_RT_FULL.  Would that be a reasonable approach?

Yes but why do we do this raw_spinlock_t here? It is not yet needed on
v5.6-RT as I *did* check. It also complicates the code for !RT but
nobody responded to that part but…

That said: the current memory allocation is the problem here. The
remaining part is fine. The part under the lock is small enough so it
should not cause the trouble if it invokes queue_work() which will
"only" enqueue the timer. 

Side question: Is there any real-life workloads that benefits from this?
I'm asking because rcuperf allocates the kfree_rcu() the pointer right
away. The chances are high that the pointer are fed from the same page.
SLUB's build_detached_freelist() scans the page of RCU's pointers to
ensure that they are in the same page and then slab_free() them in one
go. There is lookahead = 3 so it finds three different pages it stops
further scanning and does slab_free() with what it found so far.

Which means if your kfree_rcu() collects random pointer from the system,
they may belong to different pages (especially if they are part of
different "types").

> 							Thanx, Paul

Sebastian