linux-kernel - Re: [RFC 0/3] mm/page_alloc: Remote per-cpu lists drain support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9c6c0840-a37d-c6fb-78a8-f2c49df2380c@suse.cz>
Date:   Thu, 21 Oct 2021 10:27:01 +0200
From:   Vlastimil Babka <vbabka@...e.cz>
To:     nsaenzju@...hat.com, akpm@...ux-foundation.org
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        frederic@...nel.org, tglx@...utronix.de, peterz@...radead.org,
        mtosatti@...hat.com, nilal@...hat.com, mgorman@...e.de,
        linux-rt-users@...r.kernel.org, cl@...ux.com, paulmck@...nel.org,
        ppandit@...hat.com
Subject: Re: [RFC 0/3] mm/page_alloc: Remote per-cpu lists drain support

On 10/13/21 14:50, nsaenzju@...hat.com 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."
>