diff mbox series

[RFC,v4,02/49] multi-process: util: Add qemu_thread_cancel() to cancel running thread

Message ID e821aa9bccb49bf68c94e3d49b105c420dde9981.1571905346.git.jag.raman@oracle.com (mailing list archive)
State New, archived
Headers show
Series Initial support of multi-process qemu | expand

Commit Message

Jag Raman Oct. 24, 2019, 9:08 a.m. UTC
qemu_thread_cancel() added to destroy a given running thread.
This will be needed in the following patches.

Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
---
 include/qemu/thread.h    |  1 +
 util/qemu-thread-posix.c | 10 ++++++++++
 2 files changed, 11 insertions(+)

Comments

Stefan Hajnoczi Nov. 13, 2019, 3:30 p.m. UTC | #1
On Thu, Oct 24, 2019 at 05:08:43AM -0400, Jagannathan Raman wrote:
> qemu_thread_cancel() added to destroy a given running thread.
> This will be needed in the following patches.
> 
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> ---
>  include/qemu/thread.h    |  1 +
>  util/qemu-thread-posix.c | 10 ++++++++++
>  2 files changed, 11 insertions(+)

Is this still needed?  I thought previous discussion concluded that
thread cancellation is hard to get right and it's not actually used by
this series?

Stefan
Jag Raman Nov. 13, 2019, 3:38 p.m. UTC | #2
On 11/13/2019 10:30 AM, Stefan Hajnoczi wrote:
> On Thu, Oct 24, 2019 at 05:08:43AM -0400, Jagannathan Raman wrote:
>> qemu_thread_cancel() added to destroy a given running thread.
>> This will be needed in the following patches.
>>
>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> ---
>>   include/qemu/thread.h    |  1 +
>>   util/qemu-thread-posix.c | 10 ++++++++++
>>   2 files changed, 11 insertions(+)
> 
> Is this still needed?  I thought previous discussion concluded that
> thread cancellation is hard to get right and it's not actually used by
> this series?

Hi Stefan,

This is used in PATCH 41/49.

Thank you very much!
--
Jag

> 
> Stefan
>
Daniel P. Berrangé Nov. 13, 2019, 3:51 p.m. UTC | #3
On Wed, Nov 13, 2019 at 10:38:06AM -0500, Jag Raman wrote:
> 
> 
> On 11/13/2019 10:30 AM, Stefan Hajnoczi wrote:
> > On Thu, Oct 24, 2019 at 05:08:43AM -0400, Jagannathan Raman wrote:
> > > qemu_thread_cancel() added to destroy a given running thread.
> > > This will be needed in the following patches.
> > > 
> > > Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> > > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> > > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > > ---
> > >   include/qemu/thread.h    |  1 +
> > >   util/qemu-thread-posix.c | 10 ++++++++++
> > >   2 files changed, 11 insertions(+)
> > 
> > Is this still needed?  I thought previous discussion concluded that
> > thread cancellation is hard to get right and it's not actually used by
> > this series?
> 
> Hi Stefan,
> 
> This is used in PATCH 41/49.

I don't believe the cancellation usage in that patch is safe :-)

Regards,
Daniel
Jag Raman Nov. 13, 2019, 4:04 p.m. UTC | #4
On 11/13/2019 10:51 AM, Daniel P. Berrangé wrote:
> On Wed, Nov 13, 2019 at 10:38:06AM -0500, Jag Raman wrote:
>>
>>
>> On 11/13/2019 10:30 AM, Stefan Hajnoczi wrote:
>>> On Thu, Oct 24, 2019 at 05:08:43AM -0400, Jagannathan Raman wrote:
>>>> qemu_thread_cancel() added to destroy a given running thread.
>>>> This will be needed in the following patches.
>>>>
>>>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>>>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>>>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>>>> ---
>>>>    include/qemu/thread.h    |  1 +
>>>>    util/qemu-thread-posix.c | 10 ++++++++++
>>>>    2 files changed, 11 insertions(+)
>>>
>>> Is this still needed?  I thought previous discussion concluded that
>>> thread cancellation is hard to get right and it's not actually used by
>>> this series?
>>
>> Hi Stefan,
>>
>> This is used in PATCH 41/49.
> 
> I don't believe the cancellation usage in that patch is safe :-)

Thanks for the feedback, we will address that.

May I please ask why it is not safe? Any clarification will help us to
find a better alternative.

Thank you very much!
--
Jag

> 
> Regards,
> Daniel
>
Daniel P. Berrangé Nov. 13, 2019, 4:35 p.m. UTC | #5
On Wed, Nov 13, 2019 at 11:04:58AM -0500, Jag Raman wrote:
> 
> 
> On 11/13/2019 10:51 AM, Daniel P. Berrangé wrote:
> > On Wed, Nov 13, 2019 at 10:38:06AM -0500, Jag Raman wrote:
> > > 
> > > 
> > > On 11/13/2019 10:30 AM, Stefan Hajnoczi wrote:
> > > > On Thu, Oct 24, 2019 at 05:08:43AM -0400, Jagannathan Raman wrote:
> > > > > qemu_thread_cancel() added to destroy a given running thread.
> > > > > This will be needed in the following patches.
> > > > > 
> > > > > Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> > > > > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> > > > > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > > > > ---
> > > > >    include/qemu/thread.h    |  1 +
> > > > >    util/qemu-thread-posix.c | 10 ++++++++++
> > > > >    2 files changed, 11 insertions(+)
> > > > 
> > > > Is this still needed?  I thought previous discussion concluded that
> > > > thread cancellation is hard to get right and it's not actually used by
> > > > this series?
> > > 
> > > Hi Stefan,
> > > 
> > > This is used in PATCH 41/49.
> > 
> > I don't believe the cancellation usage in that patch is safe :-)
> 
> Thanks for the feedback, we will address that.
> 
> May I please ask why it is not safe? Any clarification will help us to
> find a better alternative.

I put some comments inline in the patch 41 explaining my thoughts.

Regards,
Daniel
diff mbox series

Patch

diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index 047db03..fe7fa5a 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -175,6 +175,7 @@  void qemu_thread_create(QemuThread *thread, const char *name,
                         void *(*start_routine)(void *),
                         void *arg, int mode);
 void *qemu_thread_join(QemuThread *thread);
+void qemu_thread_cancel(QemuThread *thread);
 void qemu_thread_get_self(QemuThread *thread);
 bool qemu_thread_is_self(QemuThread *thread);
 void qemu_thread_exit(void *retval);
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 838980a..2fd85ed 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -590,3 +590,13 @@  void *qemu_thread_join(QemuThread *thread)
     }
     return ret;
 }
+
+void qemu_thread_cancel(QemuThread *thread)
+{
+    int err;
+
+    err = pthread_cancel(thread->thread);
+    if (err) {
+        error_exit(err, __func__);
+    }
+}