[PATCH] KVM_SET_MEMORY_REGION hang in linux 2.6.23.y

[PATCH] KVM_SET_MEMORY_REGION hang in linux 2.6.23.y

[Marko Kohtala]

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);
 	while (atomic_read(&completed) != needed) {
 		cpu_relax();
 		barrier();

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

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

[Avi Kivity]

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)?

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

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

[Marko Kohtala]

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).

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

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

[Avi Kivity]

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/