Re: 2.6.35-rc1 regression with pvclock and smp guests

[Arjan Koers <0h61vkll2ly8@xutrox.com>]

On 2010-08-02 16:43, 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

Maybe you could add a sanity check in pvclock_clocksource_read
after 'do { ... } while (version != src->version)' that
returns last_value if offset is extremely large?


I've performed some more boot tests (about 20) with the patch that
moves the printk after native_write_msr_safe and it works for me.
Andre Przywara confirmed to me that it also fixes his problem.

A slightly modified version of the patch for 2.6.34.1 also works
(800+ successful boot cycles).