Re: [RFC 2/2] x86, vdso, pvclock: Simplify and speed up the vdso pvclock reader

Marcelo Tosatti
2015-01-07T14:45:27Z

On Tue, Jan 06, 2015 at 11:18:21PM -0800, Andy Lutomirski wrote:
> On Tue, Jan 6, 2015 at 9:38 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> >
> > On 06/01/2015 17:56, Andy Lutomirski wrote:
> >> Still no good. We can migrate a bunch of times so we see the same CPU
> >> all three times
> >
> > There are no three times. The CPU you see here:
> >
> >>>
> >>>
> >>> // ... compute nanoseconds from pvti and tsc ...
> >>> rmb();
> >>> } while(v != pvti->version);
> >
> > is the same you read here:
> >
> >>> cpu = get_cpu();
> >
> > The algorithm is:
>
> I still don't see why this is safe, and I think that the issue is that
> you left out part of the loop.
>
> >
> > 1) get a consistent (cpu, version, tsc)
> >
> > 1.a) get cpu
>
> Suppose we observe cpu 0.
>
> > 1.b) get pvti[cpu]->version, ignoring low bit
>
> Missing step, presumably here: read pvti[cpu]->tsc_timestamp, scale,
> etc. This could all execute on vCPU 1. We could read values that are
> inconsistent with each other.
>
> > 1.c) get (tsc, cpu)
>
> Now we could end up back on vCPU 0.
>
> > 1.d) if cpu from 1.a and 1.c do not match, loop
> > 1.e) if pvti[cpu] was being updated, we'll loop later
> >
> > 2) compute nanoseconds from pvti[cpu] and tsc
> >
> > 3) if pvti[cpu] changed under our feet during (2), i.e. version doesn't
> > match, retry.
> >
> > As long as the CPU is consistent between get_cpu() and rdtscp(), there
> > is no problem with migration, because pvti is always accessed for that
> > one CPU. If there were any problem, it would be caught by the version
> > check. Writing it down with two nested do...whiles makes it clearer IMHO.
>
> Why exactly would it be caught by the version check?
>
> My ugly patch tries to make the argument that, at any point at which
> we observe ourselves to be on a given vCPU, that vCPU won't be
> updating pvti. That means that, if version doesn't change between two
> consecutive observations that we're on that vCPU, then we're okay.
> This IMO sucks. It's fragile, it's hard to make a coherent argument
> about correctness, and it requires at least two getcpu-like operations
> to read the time. Those operations are *slow*. One is much better
> than two, and zero is much better than one.
>
> >
> >> and *still* don't get a consistent read, unless we
> >> play nasty games with lots of version checks (I have a patch for that,
> >> but I don't like it very much). The patch is here:
> >>
> >> https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=x86/vdso_paranoia&id=a69754dc5ff33f5187162b5338854ad23dd7be8d
> >>
> >> but I don't like it.
> >>
> >> Thus far, I've been told unambiguously that a guest can't observe pvti
> >> while it's being written, and I think you're now telling me that this
> >> isn't true and that a guest *can* observe pvti while it's being
> >> written while the low bit of the version field is not set. If so,
> >> this is rather strongly incompatible with the spec in the KVM docs.
> >
> > Where am I saying that?
>
> I thought the conclusion from what you and Marcelo pointed out about
> the code was that, once the first vCPU updated its pvti, it could
> start running guest code while the other vCPUs are still updating
> pvti, so its guest code can observe the other vCPUs mid-update.
>
> >> Also, if you do this, can you also make setting and clearing
> >> STABLE_BIT properly atomic across all vCPUs? Or at least do something
> >> like setting it last and clearing it first on vPCU 0?
> >
> > That would be nice if you want to make the pvclock area fit in a single
> > page. However, it would have to be a separate flag bit, or a separate
> > CPUID feature.
>
> It would be nice to have. Although I think that fixing the host to
> increment version pre-update and post-update may actually be good
> enough. Is there any case in which it would fail in practice if we
> made that fix and always looked at pvti 0?

TSC_STABLE_BIT -> ~TSC_STABLE_BIT transition steps would finish but
still allow VCPU-1 to use stale values from VCPU-0.

To fix, do one of the following:

1) Check validity of local TSC_STABLE_BIT in addition (slow).
2) Perform update of VCPU-0 pvclock area before allowing
any other VCPU to VM-entry.

>
> --Andy
>
> >
> > Paolo
>
>
>
> --
> Andy Lutomirski
> AMA Capital Management, LLC