Re: 2.6.35-rc1 regression with pvclock and smp guests

[Glauber Costa <glommer@redhat.com>]

On Mon, Aug 02, 2010 at 10:26:30AM -1000, Zachary Amsden wrote:
> On 08/02/2010 04:43 AM, Glauber Costa wrote:
> >On Sat, Jul 31, 2010 at 01:55:10PM -1000, Zachary Amsden wrote:
> >>On 07/31/2010 06:36 AM, Arjan Koers wrote:
> >>>On 2010-07-31 13:53, Arjan Koers wrote:
> >>>>The kernel boots successfully when CONFIG_PRINTK_TIME is not set.
> >>>>
> >>>The problem occurs when this message is printed:
> >>>
> >>>[    0.016000] kvm-clock: cpu 1, msr 0:1511c01, secondary cpu clock
> >>>
> >>>When I disable that printk, the kernel boots with
> >>>CONFIG_PRINTK_TIME=y
> >>>
> >>>--- a/arch/x86/kernel/kvmclock.c
> >>>+++ b/arch/x86/kernel/kvmclock.c
> >>>@@ -131,8 +131,8 @@ static int kvm_register_clock(char *txt)
> >>>  	int low, high;
> >>>  	low = (int)__pa(&per_cpu(hv_clock, cpu)) | 1;
> >>>  	high = ((u64)__pa(&per_cpu(hv_clock, cpu))>>   32);
> >>>-	printk(KERN_INFO "kvm-clock: cpu %d, msr %x:%x, %s\n",
> >>>-	       cpu, high, low, txt);
> >>>+	/*printk(KERN_INFO "kvm-clock: cpu %d, msr %x:%x, %s\n",
> >>>+	       cpu, high, low, txt);*/
> >>>
> >>>  	return native_write_msr_safe(msr_kvm_system_time, low, high);
> >>>  }
> >>>
> >>>So the problem appears to be that the clock of the second CPU
> >>>is used too soon (or that clock setup should finish earlier).
> >>That's almost hilarious.  The printk from setting up the kvm clock
> >>is invoking the kvm clock before it is setup.
> >>
> >>There's no reason other printks couldn't do the same thing, however.
> >>I think it's safest to keep an initialized flag and check for it
> >>before attempting to return a meaningful value.
> >I was on vacations, just got back.
> >
> >I think it is safe to just patch our own use of it. Before that, all other
> >printks will be handled by the main cpu anyway, since it'll be the only one active
> >at the moment. The only possible offenders for this are us, and the cpu initialization
> >code, which is already fragile in multiple ways anyway.
> >
> >A flag would only make things more complicated and dirty
> Can we just do this?

> Initialize hv_clock to zero
> 
> This stops callers from getting random values if data is accessed before
> clock is initialized; instead they will get zeroed clock values (because
> computation involves a multiplication by a factor in hv_clock).
> 
> Signed-off-by: Zachary Amsden <zamsden@redhat.com>
> 
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index eb9b76c..e7acd0d 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -40,7 +40,7 @@ static int parse_no_kvmclock(char *arg)
>  early_param("no-kvmclock", parse_no_kvmclock);
>  
>  /* The hypervisor will put information about time periodically here */
> -static DEFINE_PER_CPU_SHARED_ALIGNED(struct pvclock_vcpu_time_info, hv_clock);
> +static DEFINE_PER_CPU_SHARED_ALIGNED(struct pvclock_vcpu_time_info, hv_clock) = {0};
>  static struct pvclock_wall_clock wall_clock;
We can, but I am a little bit afraid that it won't initialize all the per-cpu areas.
If it does, it is fine, though.