Re: 答复: [PATCH v1 1/2] system/cpus: Fix pause_all_vcpus() under concurrent environment
On Tue, 19 Mar 2024 at 14:46, David Hildenbrand wrote: > > On 19.03.24 15:23, Peter Maydell wrote: > > On Tue, 19 Mar 2024 at 09:24, David Hildenbrand wrote: > >> I spotted new pause_all_vcpus() / resume_all_vcpus() calls in > >> hw/intc/arm_gicv3_kvm.c and > >> thought they would be the problematic bit. > >> > >> Yeah, that's going to be problematic. Further note that a lot of code does > >> not expect > >> that the BQL is suddenly dropped. > > > > Agreed; we already have one nasty set of bugs in the framebuffer > > devices because a function drops the BQL briefly: > > https://lore.kernel.org/qemu-devel/CAFEAcA9odnPo2LPip295Uztri7JfoVnQbkJ=wn+k8dqneb_...@mail.gmail.com/T/#u > > so let's avoid introducing any more of a similar kind. > > > > Side note, the pause_all_vcpus()/resume_all_vcpus() calls in > > hw/i386/vapic.c are probably a bit suspect for similar reasons. > > Exactly my thoughts. But there, it was less clear "why" it is even > required. It's only performed for KVM. > > Do we also just want to stop KVM threads from executing instructions?, > so blocking KVM ioctls might be a reasonable "replacement"? Really not sure. I think the vapic code wants to stop other threads from executing instructions while it's patching them, yes. -- PMM
Re: 答复: [PATCH v1 1/2] system/cpus: Fix pause_all_vcpus() under concurrent environment
On 19.03.24 15:23, Peter Maydell wrote: On Tue, 19 Mar 2024 at 09:24, David Hildenbrand wrote: I spotted new pause_all_vcpus() / resume_all_vcpus() calls in hw/intc/arm_gicv3_kvm.c and thought they would be the problematic bit. Yeah, that's going to be problematic. Further note that a lot of code does not expect that the BQL is suddenly dropped. Agreed; we already have one nasty set of bugs in the framebuffer devices because a function drops the BQL briefly: https://lore.kernel.org/qemu-devel/CAFEAcA9odnPo2LPip295Uztri7JfoVnQbkJ=wn+k8dqneb_...@mail.gmail.com/T/#u so let's avoid introducing any more of a similar kind. Side note, the pause_all_vcpus()/resume_all_vcpus() calls in hw/i386/vapic.c are probably a bit suspect for similar reasons. Exactly my thoughts. But there, it was less clear "why" it is even required. It's only performed for KVM. Do we also just want to stop KVM threads from executing instructions?, so blocking KVM ioctls might be a reasonable "replacement"? Really not sure. -- Cheers, David / dhildenb
Re: 答复: [PATCH v1 1/2] system/cpus: Fix pause_all_vcpus() under concurrent environment
On Tue, 19 Mar 2024 at 09:24, David Hildenbrand wrote: > I spotted new pause_all_vcpus() / resume_all_vcpus() calls in > hw/intc/arm_gicv3_kvm.c and > thought they would be the problematic bit. > > Yeah, that's going to be problematic. Further note that a lot of code does > not expect > that the BQL is suddenly dropped. Agreed; we already have one nasty set of bugs in the framebuffer devices because a function drops the BQL briefly: https://lore.kernel.org/qemu-devel/CAFEAcA9odnPo2LPip295Uztri7JfoVnQbkJ=wn+k8dqneb_...@mail.gmail.com/T/#u so let's avoid introducing any more of a similar kind. Side note, the pause_all_vcpus()/resume_all_vcpus() calls in hw/i386/vapic.c are probably a bit suspect for similar reasons. -- PMM
Re: 答复: [PATCH v1 1/2] system/cpus: Fix pause_all_vcpus() under concurrent environment
On 19.03.24 10:24, David Hildenbrand wrote: On 19.03.24 06:06, zhukeqian wrote: Hi David, Thanks for reviewing. On 17.03.24 09:37, Keqian Zhu via wrote: Both main loop thread and vCPU thread are allowed to call pause_all_vcpus(), and in general resume_all_vcpus() is called after it. Two issues live in pause_all_vcpus(): In general, calling pause_all_vcpus() from VCPU threads is quite dangerous. Do we have reproducers for the cases below? I produce the issues by testing ARM vCPU hotplug feature: QEMU changes for vCPU hotplug could be cloned from below site, https://github.com/salil-mehta/qemu.git virt-cpuhp-armv8/rfc-v2 Guest Kernel changes (by James Morse, ARM) are available here: https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git virtual_cpu_hotplug/rfc/v2 Thanks for these infos (would be reasonable to include that in the cover letter). Okay, so likely this is not actually a "fix" for upstream as it is. Understood. The procedure to produce problems: 1. Startup a Linux VM (e.g., called OS-vcpuhotplug) with 32 possible vCPUs and 16 current vCPUs. 2. Log in guestOS and run script[1] to continuously online/offline CPU. 3. At host side, run script[2] to continuously hotplug/unhotplug vCPU. After several minutes, we can hit these problems. Script[1] to online/offline CPU: for ((time=1;time<1000;time++)); do for ((cpu=16;cpu<32;cpu++)); do echo 1 > /sys/devices/system/cpu/cpu$cpu/online done for ((cpu=16;cpu<32;cpu++)); do echo 0 > /sys/devices/system/cpu/cpu$cpu/online done done Script[2] to hotplug/unhotplug vCPU: for ((time=1;time<100;time++)); do echo $time for ((cpu=16;cpu<=32;cpu++)); do echo "virsh setvcpus OS-vcpuhotplug --count $cpu --live" virsh setvcpus OS-vcpuhotplug --count $cpu --live sleep 2 done for ((cpu=32;cpu>=16;cpu--)); do echo "virsh setvcpus OS-vcpuhotplug --count $cpu --live" virsh setvcpus OS-vcpuhotplug --count $cpu --live sleep 2 done for ((cpu=16;cpu<=32;cpu+=2)); do echo "virsh setvcpus OS-vcpuhotplug --count $cpu --live" virsh setvcpus OS-vcpuhotplug --count $cpu --live sleep 2 done for ((cpu=32;cpu>=16;cpu-=2)); do echo "virsh setvcpus OS-vcpuhotplug --count $cpu --live" virsh setvcpus OS-vcpuhotplug --count $cpu --live sleep 2 done done The script[1] will call PSCI CPU_ON which emulated by QEMU, which result in calling cpu_reset() on vCPU thread. I spotted new pause_all_vcpus() / resume_all_vcpus() calls in hw/intc/arm_gicv3_kvm.c and thought they would be the problematic bit. Yeah, that's going to be problematic. Further note that a lot of code does not expect that the BQL is suddenly dropped. We had issues with that in different context where we ended up wanting to use pause/resume from VCPU context: https://lore.kernel.org/all/294a987d-b0ef-1b58-98ac-0d4d43075...@redhat.com/ This sounds like a bad idea. Read below. For ARM architecture, it needs to reset GICC registers, which is only possible when all vcpus paused. So script[1] will call pause_all_vcpus() in vCPU thread. The script[2] also calls cpu_reset() for newly hotplugged vCPU, which is done in main loop thread. So this scenario causes problems as I state in commit message. 1. There is possibility that during thread T1 waits on qemu_pause_cond with bql unlocked, other thread has called pause_all_vcpus() and resume_all_vcpus(), then thread T1 will stuck, because the condition all_vcpus_paused() is always false. How can this happen? Two threads calling pause_all_vcpus() is borderline broken, as you note. IIRC, we should call pause_all_vcpus() only if some other mechanism prevents these races. For example, based on runstate changes. We already has bql to prevent concurrent calling of pause_all_vcpus() and resume_all_vcpus(). But pause_all_vcpus() will unlock bql in the half way, which gives change for other thread to call pause and resume. In the past, code does not consider this problem, now I add retry mechanism to fix it. Note that BQL did not prevent concurrent calling of pause_all_vcpus(). There had to be something else. Likely that was runstate transitions. Just imagine one thread calling pause_all_vcpus() while another one calls resume_all_vcpus(). It cannot possibly work. With bql, we can make sure all vcpus are paused after pause_all_vcpus() finish, and all vcpus are resumed after resume_all_vcpus() finish. For example, the following situation may occur: Thread T1: lock bql ->pause_all_vcpus -> wait on cond and unlock bql -> wait T2 unlock bql to lock bql
Re: 答复: [PATCH v1 1/2] system/cpus: Fix pause_all_vcpus() under concurrent environment
On 19.03.24 06:06, zhukeqian wrote: Hi David, Thanks for reviewing. On 17.03.24 09:37, Keqian Zhu via wrote: Both main loop thread and vCPU thread are allowed to call pause_all_vcpus(), and in general resume_all_vcpus() is called after it. Two issues live in pause_all_vcpus(): In general, calling pause_all_vcpus() from VCPU threads is quite dangerous. Do we have reproducers for the cases below? I produce the issues by testing ARM vCPU hotplug feature: QEMU changes for vCPU hotplug could be cloned from below site, https://github.com/salil-mehta/qemu.git virt-cpuhp-armv8/rfc-v2 Guest Kernel changes (by James Morse, ARM) are available here: https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git virtual_cpu_hotplug/rfc/v2 Thanks for these infos (would be reasonable to include that in the cover letter). Okay, so likely this is not actually a "fix" for upstream as it is. Understood. The procedure to produce problems: 1. Startup a Linux VM (e.g., called OS-vcpuhotplug) with 32 possible vCPUs and 16 current vCPUs. 2. Log in guestOS and run script[1] to continuously online/offline CPU. 3. At host side, run script[2] to continuously hotplug/unhotplug vCPU. After several minutes, we can hit these problems. Script[1] to online/offline CPU: for ((time=1;time<1000;time++)); do for ((cpu=16;cpu<32;cpu++)); do echo 1 > /sys/devices/system/cpu/cpu$cpu/online done for ((cpu=16;cpu<32;cpu++)); do echo 0 > /sys/devices/system/cpu/cpu$cpu/online done done Script[2] to hotplug/unhotplug vCPU: for ((time=1;time<100;time++)); do echo $time for ((cpu=16;cpu<=32;cpu++)); do echo "virsh setvcpus OS-vcpuhotplug --count $cpu --live" virsh setvcpus OS-vcpuhotplug --count $cpu --live sleep 2 done for ((cpu=32;cpu>=16;cpu--)); do echo "virsh setvcpus OS-vcpuhotplug --count $cpu --live" virsh setvcpus OS-vcpuhotplug --count $cpu --live sleep 2 done for ((cpu=16;cpu<=32;cpu+=2)); do echo "virsh setvcpus OS-vcpuhotplug --count $cpu --live" virsh setvcpus OS-vcpuhotplug --count $cpu --live sleep 2 done for ((cpu=32;cpu>=16;cpu-=2)); do echo "virsh setvcpus OS-vcpuhotplug --count $cpu --live" virsh setvcpus OS-vcpuhotplug --count $cpu --live sleep 2 done done The script[1] will call PSCI CPU_ON which emulated by QEMU, which result in calling cpu_reset() on vCPU thread. I spotted new pause_all_vcpus() / resume_all_vcpus() calls in hw/intc/arm_gicv3_kvm.c and thought they would be the problematic bit. Yeah, that's going to be problematic. Further note that a lot of code does not expect that the BQL is suddenly dropped. We had issues with that in different context where we ended up wanting to use pause/resume from VCPU context: https://lore.kernel.org/all/294a987d-b0ef-1b58-98ac-0d4d43075...@redhat.com/ This sounds like a bad idea. Read below. For ARM architecture, it needs to reset GICC registers, which is only possible when all vcpus paused. So script[1] will call pause_all_vcpus() in vCPU thread. The script[2] also calls cpu_reset() for newly hotplugged vCPU, which is done in main loop thread. So this scenario causes problems as I state in commit message. 1. There is possibility that during thread T1 waits on qemu_pause_cond with bql unlocked, other thread has called pause_all_vcpus() and resume_all_vcpus(), then thread T1 will stuck, because the condition all_vcpus_paused() is always false. How can this happen? Two threads calling pause_all_vcpus() is borderline broken, as you note. IIRC, we should call pause_all_vcpus() only if some other mechanism prevents these races. For example, based on runstate changes. We already has bql to prevent concurrent calling of pause_all_vcpus() and resume_all_vcpus(). But pause_all_vcpus() will unlock bql in the half way, which gives change for other thread to call pause and resume. In the past, code does not consider this problem, now I add retry mechanism to fix it. Note that BQL did not prevent concurrent calling of pause_all_vcpus(). There had to be something else. Likely that was runstate transitions. Just imagine one thread calling pause_all_vcpus() while another one calls resume_all_vcpus(). It cannot possibly work. With bql, we can make sure all vcpus are paused after pause_all_vcpus() finish, and all vcpus are resumed after resume_all_vcpus() finish. For example, the following situation may occur: Thread T1: lock bql ->pause_all_vcpus -> wait on cond and unlock bql -> wait T2 unlock bql to lock bql-> lock bql && all_vcpu_paused ->
答复: [PATCH v1 1/2] system/cpus: Fix pause_all_vcpus() under concurrent environment
Hi David, Thanks for reviewing. On 17.03.24 09:37, Keqian Zhu via wrote: >> Both main loop thread and vCPU thread are allowed to call >> pause_all_vcpus(), and in general resume_all_vcpus() is called after >> it. Two issues live in pause_all_vcpus(): > >In general, calling pause_all_vcpus() from VCPU threads is quite dangerous. > >Do we have reproducers for the cases below? > I produce the issues by testing ARM vCPU hotplug feature: QEMU changes for vCPU hotplug could be cloned from below site, https://github.com/salil-mehta/qemu.git virt-cpuhp-armv8/rfc-v2 Guest Kernel changes (by James Morse, ARM) are available here: https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git virtual_cpu_hotplug/rfc/v2 The procedure to produce problems: 1. Startup a Linux VM (e.g., called OS-vcpuhotplug) with 32 possible vCPUs and 16 current vCPUs. 2. Log in guestOS and run script[1] to continuously online/offline CPU. 3. At host side, run script[2] to continuously hotplug/unhotplug vCPU. After several minutes, we can hit these problems. Script[1] to online/offline CPU: for ((time=1;time<1000;time++)); do for ((cpu=16;cpu<32;cpu++)); do echo 1 > /sys/devices/system/cpu/cpu$cpu/online done for ((cpu=16;cpu<32;cpu++)); do echo 0 > /sys/devices/system/cpu/cpu$cpu/online done done Script[2] to hotplug/unhotplug vCPU: for ((time=1;time<100;time++)); do echo $time for ((cpu=16;cpu<=32;cpu++)); do echo "virsh setvcpus OS-vcpuhotplug --count $cpu --live" virsh setvcpus OS-vcpuhotplug --count $cpu --live sleep 2 done for ((cpu=32;cpu>=16;cpu--)); do echo "virsh setvcpus OS-vcpuhotplug --count $cpu --live" virsh setvcpus OS-vcpuhotplug --count $cpu --live sleep 2 done for ((cpu=16;cpu<=32;cpu+=2)); do echo "virsh setvcpus OS-vcpuhotplug --count $cpu --live" virsh setvcpus OS-vcpuhotplug --count $cpu --live sleep 2 done for ((cpu=32;cpu>=16;cpu-=2)); do echo "virsh setvcpus OS-vcpuhotplug --count $cpu --live" virsh setvcpus OS-vcpuhotplug --count $cpu --live sleep 2 done done The script[1] will call PSCI CPU_ON which emulated by QEMU, which result in calling cpu_reset() on vCPU thread. For ARM architecture, it needs to reset GICC registers, which is only possible when all vcpus paused. So script[1] will call pause_all_vcpus() in vCPU thread. The script[2] also calls cpu_reset() for newly hotplugged vCPU, which is done in main loop thread. So this scenario causes problems as I state in commit message. >> >> 1. There is possibility that during thread T1 waits on qemu_pause_cond >> with bql unlocked, other thread has called >> pause_all_vcpus() and resume_all_vcpus(), then thread T1 will stuck, >> because the condition all_vcpus_paused() is always false. > >How can this happen? > >Two threads calling pause_all_vcpus() is borderline broken, as you note. > >IIRC, we should call pause_all_vcpus() only if some other mechanism prevents >these races. For example, based on runstate changes. > We already has bql to prevent concurrent calling of pause_all_vcpus() and resume_all_vcpus(). But pause_all_vcpus() will unlock bql in the half way, which gives change for other thread to call pause and resume. In the past, code does not consider this problem, now I add retry mechanism to fix it. > >Just imagine one thread calling pause_all_vcpus() while another one >calls resume_all_vcpus(). It cannot possibly work. With bql, we can make sure all vcpus are paused after pause_all_vcpus() finish, and all vcpus are resumed after resume_all_vcpus() finish. For example, the following situation may occur: Thread T1: lock bql ->pause_all_vcpus -> wait on cond and unlock bql -> wait T2 unlock bql to lock bql -> lock bql && all_vcpu_paused -> success and do other work -> unlock bql Thread T2: wait T1 unlock bql to lock bql -> lock bql-> resume_all_vcpus -> success and do other work -> unlock bql Thanks, Keqian > > >> >> 2. After all_vcpus_paused() has been checked as true, we will >> unlock bql to relock replay_mutex. During the bql was unlocked, >> the vcpu's state may has been changed by other thread, so we >> must retry. >> >> Signed-off-by: Keqian Zhu >> --- >> system/cpus.c | 29 - >> 1 file changed, 24 insertions(+), 5 deletions(-) >> > diff --git a/system/cpus.c b/system/cpus.c > index 68d161d96b..4e41abe23e 100644 > --- a/system/cpus.c > +++ b/system/cpus.c > @@ -571,12 +571,14 @@ static bool all_vcpus_paused(void) > return true; > } > > -void
