Re: [PATCH] KVM_SET_MEMORY_REGION hang in linux 2.6.23.y

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Marko Kohtala wrote:
> On Nov 21, 2007 8:56 AM, Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:
>   
>> Marko Kohtala wrote:
>>     
>>> Wait for right amount of tlb flushes. Completed can be larger than
>>> needed and therefore the loop waiting them to match never ends.
>>>
>>> Signed-off-by: Marko Kohtala <marko.kohtala-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>
>>> ---
>>>
>>> This solves kernel lockup in KVM_SET_MEMORY_REGION ioctl with Linux
>>> 2.6.23.8 and before at kvm-52 start. Not needed in 2.6.24.
>>>
>>> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
>>> index cd05579..b148aff 100644
>>> --- a/drivers/kvm/kvm_main.c
>>> +++ b/drivers/kvm/kvm_main.c
>>> @@ -279,7 +279,8 @@ void kvm_flush_remote_tlbs(struct kvm *kvm)
>>>        * to complete.
>>>        */
>>>       for (cpu = first_cpu(cpus); cpu != NR_CPUS; cpu = next_cpu(cpu, cpus))
>>> -             smp_call_function_single(cpu, ack_flush, &completed, 1, 0);
>>> +             if (cpu_isset(cpu, cpus))
>>> +                     smp_call_function_single(cpu, ack_flush, &completed, 1, 0);
>>>
>>>       
>> Can you explain how this makes a difference?  Aren't first_cpu() and
>> next_cpu() designed to iterate over all cpus which have cpu_isset(cpus)?
>>     
>
> Look in linux/cpumask.h:
>
>  215 #ifdef CONFIG_SMP
>  216 int __first_cpu(const cpumask_t *srcp);
>  217 #define first_cpu(src) __first_cpu(&(src))
>  218 int __next_cpu(int n, const cpumask_t *srcp);
>  219 #define next_cpu(n, src) __next_cpu((n), &(src))
>  220 #else
>  221 #define first_cpu(src)          0
>  222 #define next_cpu(n, src)        1
>  223 #endif
>
> I have uniprocessor kernel, so first_cpu always gives 0, never NR_CPUS.
>
> I quickly looked through all uses of first_cpu, and in all other
> places there is at least one CPU to iterate over. So cpumask.h is not
> really wrong. It is just confusing. But being confusing is wrong, so
> there may be some room for discussion.
>
> Another fix to this issue could be to not enter the loop at all if
> cpus_empty(cpus).
>
>   

Or maybe, change first_cpu() to return !!cpus_empty() on uniprocessor.

Though that's not for 2.6.23.  I think terminating early on cpus_empty() 
in kvm_flush_remote_tlb() is fine.

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/