Re: [PATCH RFC UGLY] x86,mm,sched: make lazy TLB mode even lazier

From: Rik van Riel
Date: Mon Aug 29 2016 - 12:09:01 EST

On Thu, 2016-08-25 at 12:42 -0700, H. Peter Anvin wrote:

> > +static void set_lazy_tlbstate_flush(int cpu) {
> > + if (per_cpu(cpu_tlbstate.state, cpu) == TLBSTATE_LAZY) {
> > + raw_spin_lock(&cpu_rq(cpu)->lock);
> > + if (per_cpu(cpu_tlbstate.state, cpu) ==
> > + per_cpu(cpu_tlbstate.state, cpu) =
> > + raw_spin_unlock(&cpu_rq(cpu)->lock);
> > + }
> > +}
> > +
> Why grabbing a lock instead of cmpxchg?

The second and third version of the patch had cmpxchg,
instead of grabbing the remote CPU's runqueue lock,
but I am no longer convinced it is safe.

At TLB invalidation time, we have this:

int *tlbstate = &per_cpu(cpu_tlbstate.state, cpu);
int old;

switch (*tlbstate) {
* The CPU is in TLBSTATE_LAZY, which could context switch back
* to TLBSTATE_OK, re-using the old TLB state without a flush.
* If that happened, send a TLB flush IPI.
* Otherwise, the state is now TLBSTATE_FLUSH, and TLB will
* be flushed at the next context switch. Skip the IPI.
old = cmpxchg(tlbstate, TLBSTATE_LAZY, TLBSTATE_FLUSH);
return old != TLBSTATE_OK;

At context switch time, we have this:

int oldstate = this_cpu_read(cpu_tlbstate.state);

this_cpu_write(cpu_tlbstate.state, TLBSTATE_OK);
BUG_ON(this_cpu_read(cpu_tlbstate.active_mm) != next);

if (oldstate == TLBSTATE_FLUSH ||
!cpumask_test_cpu(cpu, mm_cpumask(next))) {

In each case, the read will happen before the write, because
they are to the same address.

If the invalidate and context switch happen concurrently,
the writes can be ordered in two directions:

1) The cmpxchg in the TLB flush code happens after the
this_cpu_write in the context switch code. This is safe.

2) The cmpxchg in the TLB flush code happens before the
this_cpu_write in the context switch code. This is broken.

I can see two ways to fix that:
1) Change the write in the context switch code to a
cmpxchg. I do not know how expensive this is on
modern CPUs, or whether the overhead of doing this
is unacceptable (or even noticeable, considering the
cache line needs to be acquired for write anyway).
2) Acquire the runqueue lock of the remote CPU from the
(much rarer?) TLB flush code, in order to ensure it
does not run concurrently with the context switch

Any preferences?


All Rights Reversed.

Attachment: signature.asc
Description: This is a digitally signed message part