[Qemu-devel] [PATCH] cpus: ignore ESRCH in qemu_cpu_kick_thread()

[Qemu-devel] [PATCH] cpus: ignore ESRCH in qemu_cpu_kick_thread()

[Laurent Vivier]

We can have a race condition between qemu_cpu_kick_thread() and
qemu_kvm_cpu_thread_fn() when we hotunplug a CPU. In this case,
qemu_cpu_kick_thread() can try to kick a thread that is exiting.
pthread_kill() returns an error and qemu is stopped by an exit(1).

   qemu:qemu_cpu_kick_thread: No such process

We can ignore safely this error.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 cpus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cpus.c b/cpus.c
index 0ddeeefc14..4717490bd0 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1778,7 +1778,7 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
     }
     cpu->thread_kicked = true;
     err = pthread_kill(cpu->thread->thread, SIG_IPI);
-    if (err) {
+    if (err && err != ESRCH) {
         fprintf(stderr, "qemu:%s: %s", __func__, strerror(err));
         exit(1);
     }
-- 
2.20.1

Re: [Qemu-devel] [PATCH] cpus: ignore ESRCH in qemu_cpu_kick_thread()

[Philippe Mathieu-Daudé]

On 1/2/19 3:16 PM, Laurent Vivier wrote:
> We can have a race condition between qemu_cpu_kick_thread() and
> qemu_kvm_cpu_thread_fn() when we hotunplug a CPU. In this case,
> qemu_cpu_kick_thread() can try to kick a thread that is exiting.
> pthread_kill() returns an error and qemu is stopped by an exit(1).
> 
>    qemu:qemu_cpu_kick_thread: No such process
> 
> We can ignore safely this error.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  cpus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 0ddeeefc14..4717490bd0 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1778,7 +1778,7 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
>      }
>      cpu->thread_kicked = true;
>      err = pthread_kill(cpu->thread->thread, SIG_IPI);
> -    if (err) {
> +    if (err && err != ESRCH) {
>          fprintf(stderr, "qemu:%s: %s", __func__, strerror(err));
>          exit(1);
>      }
> 

Re: [Qemu-devel] [PATCH] cpus: ignore ESRCH in qemu_cpu_kick_thread()

[Paolo Bonzini]

On 02/01/19 15:16, Laurent Vivier wrote:
> We can have a race condition between qemu_cpu_kick_thread() and
> qemu_kvm_cpu_thread_fn() when we hotunplug a CPU. In this case,
> qemu_cpu_kick_thread() can try to kick a thread that is exiting.
> pthread_kill() returns an error and qemu is stopped by an exit(1).
> 
>    qemu:qemu_cpu_kick_thread: No such process
> 
> We can ignore safely this error.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  cpus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 0ddeeefc14..4717490bd0 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1778,7 +1778,7 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
>      }
>      cpu->thread_kicked = true;
>      err = pthread_kill(cpu->thread->thread, SIG_IPI);
> -    if (err) {
> +    if (err && err != ESRCH) {
>          fprintf(stderr, "qemu:%s: %s", __func__, strerror(err));
>          exit(1);
>      }
> 

You could in principle be sending the signal to another thread, so the
fix is a bit hackish.  However, I don't have a better idea that is not
racy. :(

The problem is that qemu_cpu_kick does not use any spinlock or mutex to
synchronize against cpu_remove_sync's qemu_thread_join.  I think once
the you reach qemu_cpu_kick in cpu_remove_sync (so if cpu->unplug) you
do not need to reset cpu->thread_kicked anymore, but I don't think
that's enough to fix it.

Paolo

Re: [Qemu-devel] [PATCH] cpus: ignore ESRCH in qemu_cpu_kick_thread()

[Laurent Vivier]

On 08/01/2019 00:02, Paolo Bonzini wrote:
> On 02/01/19 15:16, Laurent Vivier wrote:
>> We can have a race condition between qemu_cpu_kick_thread() and
>> qemu_kvm_cpu_thread_fn() when we hotunplug a CPU. In this case,
>> qemu_cpu_kick_thread() can try to kick a thread that is exiting.
>> pthread_kill() returns an error and qemu is stopped by an exit(1).
>>
>>    qemu:qemu_cpu_kick_thread: No such process
>>
>> We can ignore safely this error.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>  cpus.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index 0ddeeefc14..4717490bd0 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -1778,7 +1778,7 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
>>      }
>>      cpu->thread_kicked = true;
>>      err = pthread_kill(cpu->thread->thread, SIG_IPI);
>> -    if (err) {
>> +    if (err && err != ESRCH) {
>>          fprintf(stderr, "qemu:%s: %s", __func__, strerror(err));
>>          exit(1);
>>      }
>>
> 
> You could in principle be sending the signal to another thread, so the
> fix is a bit hackish.  However, I don't have a better idea that is not
> racy. :(
> 
> The problem is that qemu_cpu_kick does not use any spinlock or mutex to
> synchronize against cpu_remove_sync's qemu_thread_join.  I think once
> the you reach qemu_cpu_kick in cpu_remove_sync (so if cpu->unplug) you
> do not need to reset cpu->thread_kicked anymore, but I don't think
> that's enough to fix it.

Will you take the patch through one of your pull requests or should I
add it to the trivial-patches branch?

Thanks,
Laurent

Re: [Qemu-devel] [PATCH] cpus: ignore ESRCH in qemu_cpu_kick_thread()

[Laurent Vivier]

On 08/01/2019 19:47, Laurent Vivier wrote:
> On 08/01/2019 00:02, Paolo Bonzini wrote:
>> On 02/01/19 15:16, Laurent Vivier wrote:
>>> We can have a race condition between qemu_cpu_kick_thread() and
>>> qemu_kvm_cpu_thread_fn() when we hotunplug a CPU. In this case,
>>> qemu_cpu_kick_thread() can try to kick a thread that is exiting.
>>> pthread_kill() returns an error and qemu is stopped by an exit(1).
>>>
>>>     qemu:qemu_cpu_kick_thread: No such process
>>>
>>> We can ignore safely this error.
>>>
>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>> ---
>>>   cpus.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/cpus.c b/cpus.c
>>> index 0ddeeefc14..4717490bd0 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -1778,7 +1778,7 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
>>>       }
>>>       cpu->thread_kicked = true;
>>>       err = pthread_kill(cpu->thread->thread, SIG_IPI);
>>> -    if (err) {
>>> +    if (err && err != ESRCH) {
>>>           fprintf(stderr, "qemu:%s: %s", __func__, strerror(err));
>>>           exit(1);
>>>       }
>>>
>>
>> You could in principle be sending the signal to another thread, so the
>> fix is a bit hackish.  However, I don't have a better idea that is not
>> racy. :(
>>
>> The problem is that qemu_cpu_kick does not use any spinlock or mutex to
>> synchronize against cpu_remove_sync's qemu_thread_join.  I think once
>> the you reach qemu_cpu_kick in cpu_remove_sync (so if cpu->unplug) you
>> do not need to reset cpu->thread_kicked anymore, but I don't think
>> that's enough to fix it.
> 
> Will you take the patch through one of your pull requests or should I
> add it to the trivial-patches branch?

Paolo?

Thanks,
Laurent

Re: [Qemu-devel] [PATCH] cpus: ignore ESRCH in qemu_cpu_kick_thread()

[Paolo Bonzini]

On 15/01/19 17:34, Laurent Vivier wrote:
> On 08/01/2019 19:47, Laurent Vivier wrote:
>> On 08/01/2019 00:02, Paolo Bonzini wrote:
>>> On 02/01/19 15:16, Laurent Vivier wrote:
>>>> We can have a race condition between qemu_cpu_kick_thread() and
>>>> qemu_kvm_cpu_thread_fn() when we hotunplug a CPU. In this case,
>>>> qemu_cpu_kick_thread() can try to kick a thread that is exiting.
>>>> pthread_kill() returns an error and qemu is stopped by an exit(1).
>>>>
>>>>     qemu:qemu_cpu_kick_thread: No such process
>>>>
>>>> We can ignore safely this error.
>>>>
>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>> ---
>>>>   cpus.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/cpus.c b/cpus.c
>>>> index 0ddeeefc14..4717490bd0 100644
>>>> --- a/cpus.c
>>>> +++ b/cpus.c
>>>> @@ -1778,7 +1778,7 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
>>>>       }
>>>>       cpu->thread_kicked = true;
>>>>       err = pthread_kill(cpu->thread->thread, SIG_IPI);
>>>> -    if (err) {
>>>> +    if (err && err != ESRCH) {
>>>>           fprintf(stderr, "qemu:%s: %s", __func__, strerror(err));
>>>>           exit(1);
>>>>       }
>>>>
>>>
>>> You could in principle be sending the signal to another thread, so the
>>> fix is a bit hackish.  However, I don't have a better idea that is not
>>> racy. :(
>>>
>>> The problem is that qemu_cpu_kick does not use any spinlock or mutex to
>>> synchronize against cpu_remove_sync's qemu_thread_join.  I think once
>>> the you reach qemu_cpu_kick in cpu_remove_sync (so if cpu->unplug) you
>>> do not need to reset cpu->thread_kicked anymore, but I don't think
>>> that's enough to fix it.
>>
>> Will you take the patch through one of your pull requests or should I
>> add it to the trivial-patches branch?
> 
> Paolo?

I queued it now, thanks.

Paolo

Re: [Qemu-devel] [PATCH] cpus: ignore ESRCH in qemu_cpu_kick_thread()

[Emilio G. Cota]

On Tue, Jan 08, 2019 at 00:02:36 +0100, Paolo Bonzini wrote:
> On 02/01/19 15:16, Laurent Vivier wrote:
> > We can have a race condition between qemu_cpu_kick_thread() and
> > qemu_kvm_cpu_thread_fn() when we hotunplug a CPU. In this case,
> > qemu_cpu_kick_thread() can try to kick a thread that is exiting.
> > pthread_kill() returns an error and qemu is stopped by an exit(1).
> > 
> >    qemu:qemu_cpu_kick_thread: No such process
> > 
> > We can ignore safely this error.
> > 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > ---
> >  cpus.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/cpus.c b/cpus.c
> > index 0ddeeefc14..4717490bd0 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -1778,7 +1778,7 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
> >      }
> >      cpu->thread_kicked = true;
> >      err = pthread_kill(cpu->thread->thread, SIG_IPI);
> > -    if (err) {
> > +    if (err && err != ESRCH) {
> >          fprintf(stderr, "qemu:%s: %s", __func__, strerror(err));
> >          exit(1);
> >      }
> > 
> 
> You could in principle be sending the signal to another thread, so the
> fix is a bit hackish.  However, I don't have a better idea that is not
> racy. :(
> 
> The problem is that qemu_cpu_kick does not use any spinlock or mutex to
> synchronize against cpu_remove_sync's qemu_thread_join.  I think once
> the you reach qemu_cpu_kick in cpu_remove_sync (so if cpu->unplug) you
> do not need to reset cpu->thread_kicked anymore, but I don't think
> that's enough to fix it.

I think the per-cpu lock series[1] can help here. For instance, in
qemu_cpu_kick_thread we can acquire the CPU lock, then check cpu->unplug.
If it's set, then we don't send the signal, because the thread is on its
way out. If it isn't set, then we send the signal while still holding the
CPU lock. This guarantees that the thread exists, since cpu_remove_sync
will acquire the CPU lock to set cpu->unplug.

Thanks,

		Emilio

[1] https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02979.html