Re: x86: kvm: Revert "remove sched notifier for cross-cpu migrations"

[Marcelo Tosatti <mtosatti@redhat.com>]

On Thu, Mar 26, 2015 at 03:24:10PM -0700, Andy Lutomirski wrote:
> On Thu, Mar 26, 2015 at 3:22 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> > On Thu, Mar 26, 2015 at 09:59:24PM +0100, Radim Krčmář wrote:
> >> 2015-03-23 20:21-0300, Marcelo Tosatti:
> >> >
> >> > The following point:
> >> >
> >> >     2. per-CPU pvclock time info is updated if the
> >> >        underlying CPU changes.
> >> >
> >> > Is not true anymore since "KVM: x86: update pvclock area conditionally,
> >> > on cpu migration".
> >> >
> >> > Add task migration notification back.
> >> >
> >> > Problem noticed by Andy Lutomirski.
> >> >
> >> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> >> > CC: stable@kernel.org # 3.11+
> >>
> >> Revert contains a bug that got pointed out in the discussion:
> >>
> >> > diff --git a/arch/x86/vdso/vclock_gettime.c b/arch/x86/vdso/vclock_gettime.c
> >> >     do {
> >> >             cpu = __getcpu() & VGETCPU_CPU_MASK;
> >> >
> >> >             pvti = get_pvti(cpu);
> >>
> >> We can migrate to 'other cpu' here.
> >>
> >> > +           migrate_count = pvti->migrate_count;
> >> > +
> >> >             version = __pvclock_read_cycles(&pvti->pvti, &ret, &flags);
> >>
> >> And migrate back to 'cpu' here.
> >
> > Migrating back will increase pvti->migrate_count, right ?
> 
> I thought it only increased the count when we migrated away.

Right.

> --Andy
> 
> >
> >> rdtsc was executed on different cpu, so pvti and tsc might not be in
> >> sync, but migrate_count hasn't changed.
> >>
> >> >             cpu1 = __getcpu() & VGETCPU_CPU_MASK;
> >>
> >> (Reading cpuid here is useless.)
> >>
> >> >     } while (unlikely(cpu != cpu1 ||
> >> >                       (pvti->pvti.version & 1) ||
> >> > -                     pvti->pvti.version != version));
> >> > +                     pvti->pvti.version != version ||
> >> > +                     pvti->migrate_count != migrate_count));
> >>
> >> We can workaround the bug with,
> >>
> >>       cpu = __getcpu() & VGETCPU_CPU_MASK;
> >>       pvti = get_pvti(cpu);
> >>       migrate_count = pvti->migrate_count;
> >>       if (cpu != (__getcpu() & VGETCPU_CPU_MASK))
> >>               continue;

Looks good, please submit a fix.