Re: 2.6.35-rc1 regression with pvclock and smp guests

Zachary Amsden
2010-08-02T20:26:33Z

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

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?

[-- Attachment #2: zero.patch --]
[-- Type: text/plain, Size: 855 bytes --]

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;

/*