KVM_SET_MEMORY_REGION hang in linux 2.6.23.y

PATCH In review Marko Kohtala 2 participants 4 messages Nov 20, 2007 – Nov 21, 2007 3 messages not in the archive KVM

Share & tools

Marko Kohtala

author

2007-11-20 20:38 UTC #0 lore raw

PATCH 1/1 KVM_SET_MEMORY_REGION hang in linux 2.6.23.y

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 <[email protected]>

---

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.

drivers/kvm/kvm_main.c +2 -1

index cd05579..b148aff 100644
@@ -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/

A message that is not in the archive (sent off-list or not yet received): [email protected]

Avi Kivity

2007-11-21 06:56 UTC #2 lore raw

Quoted 15 lines · 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 <[email protected]>
---
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.
drivers/kvm/kvm_main.c +0 -0
index cd05579..b148aff 100644

drivers/kvm/kvm_main.c void kvm_flush_remote_tlbs(struct kvm *kvm) · line 284

Quoted 7 lines

drivers/kvm/kvm_main.c +2 -1

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

Signature · 7 lines

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/

A message that is not in the archive (sent off-list or not yet received): 4743D69C.7000808-atKUWr5tajBWk0Htik3J/[email protected]

Marko Kohtala

author

2007-11-21 08:04 UTC #4 lore raw

Quoted 11 lines · Avi Kivity

Quoted 10 lines · 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 <[email protected]>
---
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.

Quoted 15 lines

drivers/kvm/kvm_main.c void kvm_flush_remote_tlbs(struct kvm *kvm) · line 284

Quoted 12 lines

drivers/kvm/kvm_main.c +2 -1

index cd05579..b148aff 100644
@@ -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).

A message that is not in the archive (sent off-list or not yet received): 9cfa10eb0711210004o61ef22dbp7e411e2327810aab-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Avi Kivity

2007-11-21 08:12 UTC #6 lore raw

Quoted 45 lines · Marko Kohtala

Quoted 12 lines · Avi Kivity
Quoted 10 lines · 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 <[email protected]>
---
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.
Quoted 16 lines
drivers/kvm/kvm_main.c void kvm_flush_remote_tlbs(struct kvm *kvm) · line 285
Quoted 13 lines
drivers/kvm/kvm_main.c +2 -1
index cd05579..b148aff 100644
@@ -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.

Quoted 9 lines

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.

Signature · 7 lines

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/