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

From: Marcelo Tosatti <mtosatti@redhat.com>
To: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: kvm-devel <kvm@vger.kernel.org>,
	stable@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
	Andy Lutomirski <luto@amacapital.net>
Subject: Re: x86: kvm: Revert "remove sched notifier for cross-cpu migrations"
Date: Thu, 26 Mar 2015 19:22:38 -0300	[thread overview]
Message-ID: <20150326222238.GB27231@amt.cnet> (raw)
In-Reply-To: <20150326205924.GD13271@potion.brq.redhat.com>

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 ?

> 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;