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

[Rik van Riel <riel@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2709 bytes --]

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) ==
> > TLBSTATE_LAZY)
> > +			per_cpu(cpu_tlbstate.state, cpu) =
> > TLBSTATE_FLUSH;
> > +		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) {
        case TLBSTATE_LAZY:
                /*
                 * 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
   code.

Any preferences?

-- 

All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]