Re: [PATCH v1 1/2] system/cpus: Fix pause_all_vcpus() under concurrent environment

[David Hildenbrand]

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?

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


Just imagine one thread calling pause_all_vcpus() while another one 
calls resume_all_vcpus(). It cannot possibly work.

> 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 <zhukeqian1@huawei.com>
> ---
>   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 pause_all_vcpus(void)
> +static void request_pause_all_vcpus(void)
>   {
>       CPUState *cpu;
>   
> -    qemu_clock_enable(QEMU_CLOCK_VIRTUAL, false);
>       CPU_FOREACH(cpu) {
> +        if (cpu->stopped) {
> +            continue;
> +        }
>           if (qemu_cpu_is_self(cpu)) {
>               qemu_cpu_stop(cpu, true);
>           } else {
> @@ -584,6 +586,14 @@ void pause_all_vcpus(void)
>               qemu_cpu_kick(cpu);
>           }
>       }
> +}
> +
> +void pause_all_vcpus(void)
> +{
> +    qemu_clock_enable(QEMU_CLOCK_VIRTUAL, false);
> +
> +retry:
> +    request_pause_all_vcpus();
>   
>       /* We need to drop the replay_lock so any vCPU threads woken up
>        * can finish their replay tasks
> @@ -592,14 +602,23 @@ void pause_all_vcpus(void)
>   
>       while (!all_vcpus_paused()) {
>           qemu_cond_wait(&qemu_pause_cond, &bql);
> -        CPU_FOREACH(cpu) {
> -            qemu_cpu_kick(cpu);
> -        }
> +        /* During we waited on qemu_pause_cond the bql was unlocked,
> +         * the vcpu's state may has been changed by other thread, so
> +         * we must request the pause state on all vcpus again.
> +         */
> +        request_pause_all_vcpus();
>       }
>   
>       bql_unlock();
>       replay_mutex_lock();
>       bql_lock();
> +
> +    /* During the bql was unlocked, the vcpu's state may has been
> +     * changed by other thread, so we must retry.
> +     */
> +    if (!all_vcpus_paused()) {
> +        goto retry;
> +    }
>   }
>   
>   void cpu_resume(CPUState *cpu)

--
Cheers,

David / dhildenb