On 10/13/21 14:50, nsaenzju@xxxxxxxxxx wrote: > Hi Vlastimil, thanks for spending time on this. > Also, excuse me if I over explain things. Hi, thanks for spending time on the explanation :) It was very useful. ... > >> and the "write side" (remote draining) actually doesn't take pagesets.lock, >> so it's not true that the "lock required to replace ... is held"? The write >> side uses rcu_replace_pointer(..., mutex_is_locked(&pcpu_drain_mutex)) >> which is a different lock. > > The thing 'pagesets.lock' protects against is concurrent access to pcp->lp's > content, as opposed to its address. pcp->lp is dereferenced atomically, so no > need for locking on that operation. > > The drain side never accesses pcp->lp's contents concurrently, it changes > pcp->lp's address and makes sure all CPUs are in sync with the new address > before clearing the stale data. > > Just for the record, I think a better representation of what 'check' in > rcu_dereference means is: > > * Do an rcu_dereference(), but check that the conditions under which the > * dereference will take place are correct. Typically the conditions > * indicate the various locking conditions that should be held at that > * point. The check should return true if the conditions are satisfied. > * An implicit check for being in an RCU read-side critical section > * (rcu_read_lock()) is included. > > So for the read side, that is, code reading pcp->lp's address and its contents, > the conditions to be met are: being in a RCU critical section, to make sure RCU > is keeping track of it, and holding 'pagesets.lock', to avoid concurrently > accessing pcp->lp's contents. The later is achieved either by disabling local > irqs or disabling migration and getting a per-cpu rt_spinlock. Conveniently > these are actions that implicitly delimit an RCU critical section (see [1] and > [2]). So the 'pagesets.lock' check fully covers the read side locking/RCU > concerns. Yeah, I wasn't aware of [2] especially. It makes sense that RT locks provide the same guarantees for RCU as non-RT. > On the write side, the drain has to make sure pcp->lp address change is atomic > (this is achieved through rcu_replace_pointer()) and that lp->drain is emptied > before a happens. So checking for pcpu_drain_mutex being held is good enough. > >> IOW, synchronize_rcu_expedited() AFAICS has nothing (no rcu_read_lock() to >> synchronize against? Might accidentally work on !RT thanks to disabled irqs, >> but not sure about with RT lock semantics of the local_lock... >> >> So back to overhead, if I'm correct above we can assume that there would be >> also rcu_read_lock() in the fast paths. > > As I explained above, no need. > >> The alternative proposed by tglx was IIRC that there would be a spinlock on >> each cpu, which would be mostly uncontended except when draining. Maybe an >> uncontended spin lock/unlock would have lower overhead than all of the >> above? It would be certainly simpler, so I would probably try that first and >> see if it's acceptable? > > You have a point here. I'll provide a performance rundown of both solutions. > This one is a bit more complex that's for sure. Great, thanks! > Thanks! > > [1] See rcu_read_lock()'s description: "synchronize_rcu() wait for regions of > code with preemption disabled, including regions of code with interrupts or > softirqs disabled." > > [2] See kernel/locking/spinlock_rt.c: "The RT [spinlock] substitutions > explicitly disable migration and take rcu_read_lock() across the lock held > section." >
