* [Qemu-devel] [PATCH 0/3] iothread: allow to create internal iothreads
@ 2017-09-22 8:56 Peter Xu
2017-09-22 8:56 ` [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use Peter Xu
` (2 more replies)
0 siblings, 3 replies; 30+ messages in thread
From: Peter Xu @ 2017-09-22 8:56 UTC (permalink / raw
To: qemu-devel
Cc: Paolo Bonzini, Daniel P . Berrange, Stefan Hajnoczi, Fam Zheng,
Dr . David Alan Gilbert, peterx
When trying to support monitor OOB (out-of-band) commands, I found
that the monitor IO thread I did looks just like iothread. It would
be best if I can use iothread directly. However it seems that it was
mostly used by "-object iothread" before but not friendly to internal
usages. This series tries to export essential functions to do it.
Also, I think patch 2 also fixes a bug in iothread_stop().
Please review. Thanks.
Peter Xu (3):
iothread: provide helpers for internal use
iothread: export iothread_stop()
iothread: delay the context release to finalize
include/sysemu/iothread.h | 9 +++++++++
iothread.c | 51 +++++++++++++++++++++++++++++++++++++----------
2 files changed, 50 insertions(+), 10 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 8:56 [Qemu-devel] [PATCH 0/3] iothread: allow to create internal iothreads Peter Xu
@ 2017-09-22 8:56 ` Peter Xu
2017-09-22 9:04 ` Daniel P. Berrange
2017-09-22 10:16 ` Stefan Hajnoczi
2017-09-22 8:56 ` [Qemu-devel] [PATCH 2/3] iothread: export iothread_stop() Peter Xu
2017-09-22 8:56 ` [Qemu-devel] [PATCH 3/3] iothread: delay the context release to finalize Peter Xu
2 siblings, 2 replies; 30+ messages in thread
From: Peter Xu @ 2017-09-22 8:56 UTC (permalink / raw
To: qemu-devel
Cc: Paolo Bonzini, Daniel P . Berrange, Stefan Hajnoczi, Fam Zheng,
Dr . David Alan Gilbert, peterx
IOThread is a general framework that contains IO loop environment and a
real thread behind. It's also good to be used internally inside qemu.
Provide some helpers for it to create iothreads to be used internally.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/sysemu/iothread.h | 8 ++++++++
iothread.c | 21 +++++++++++++++++++++
2 files changed, 29 insertions(+)
diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
index d2985b3..b07663f 100644
--- a/include/sysemu/iothread.h
+++ b/include/sysemu/iothread.h
@@ -46,4 +46,12 @@ AioContext *iothread_get_aio_context(IOThread *iothread);
void iothread_stop_all(void);
GMainContext *iothread_get_g_main_context(IOThread *iothread);
+/*
+ * Helpers used to allocate iothreads for internal use. These
+ * iothreads will not be seen by monitor clients when query using
+ * "query-iothreads".
+ */
+IOThread *iothread_create(const char *id, Error **errp);
+void iothread_destroy(IOThread *iothread);
+
#endif /* IOTHREAD_H */
diff --git a/iothread.c b/iothread.c
index 44c8944..74e400c 100644
--- a/iothread.c
+++ b/iothread.c
@@ -354,3 +354,24 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread)
return iothread->worker_context;
}
+
+static Object *iothread_get_internal_parent(void)
+{
+ return container_get(object_get_root(), "/internal-iothreads");
+}
+
+IOThread *iothread_create(const char *id, Error **errp)
+{
+ Object *obj;
+
+ obj = object_new_with_props(TYPE_IOTHREAD,
+ iothread_get_internal_parent(),
+ id, errp, NULL);
+
+ return IOTHREAD(obj);
+}
+
+void iothread_destroy(IOThread *iothread)
+{
+ object_unparent(OBJECT(iothread));
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 2/3] iothread: export iothread_stop()
2017-09-22 8:56 [Qemu-devel] [PATCH 0/3] iothread: allow to create internal iothreads Peter Xu
2017-09-22 8:56 ` [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use Peter Xu
@ 2017-09-22 8:56 ` Peter Xu
2017-09-22 13:06 ` Fam Zheng
2017-09-22 8:56 ` [Qemu-devel] [PATCH 3/3] iothread: delay the context release to finalize Peter Xu
2 siblings, 1 reply; 30+ messages in thread
From: Peter Xu @ 2017-09-22 8:56 UTC (permalink / raw
To: qemu-devel
Cc: Paolo Bonzini, Daniel P . Berrange, Stefan Hajnoczi, Fam Zheng,
Dr . David Alan Gilbert, peterx
So that internal iothread users can explicitly stop one iothread without
destroying it.
Since at it, fix iothread_stop() to allow re-entrance. Before this
patch we may call iothread_stop() twice on single iothread, while that
may not be correct since qemu_thread_join() is not allowed to run twice.
>From manual of pthread_join():
Joining with a thread that has previously been joined results in
undefined behavior.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/sysemu/iothread.h | 1 +
iothread.c | 24 ++++++++++++++++--------
2 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
index b07663f..110329b 100644
--- a/include/sysemu/iothread.h
+++ b/include/sysemu/iothread.h
@@ -52,6 +52,7 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread);
* "query-iothreads".
*/
IOThread *iothread_create(const char *id, Error **errp);
+void iothread_stop(IOThread *iothread);
void iothread_destroy(IOThread *iothread);
#endif /* IOTHREAD_H */
diff --git a/iothread.c b/iothread.c
index 74e400c..894756b 100644
--- a/iothread.c
+++ b/iothread.c
@@ -80,13 +80,10 @@ static void *iothread_run(void *opaque)
return NULL;
}
-static int iothread_stop(Object *object, void *opaque)
+void iothread_stop(IOThread *iothread)
{
- IOThread *iothread;
-
- iothread = (IOThread *)object_dynamic_cast(object, TYPE_IOTHREAD);
- if (!iothread || !iothread->ctx) {
- return 0;
+ if (iothread->stopping) {
+ return;
}
iothread->stopping = true;
aio_notify(iothread->ctx);
@@ -94,6 +91,17 @@ static int iothread_stop(Object *object, void *opaque)
g_main_loop_quit(iothread->main_loop);
}
qemu_thread_join(&iothread->thread);
+}
+
+static int iothread_stop_iter(Object *object, void *opaque)
+{
+ IOThread *iothread;
+
+ iothread = (IOThread *)object_dynamic_cast(object, TYPE_IOTHREAD);
+ if (!iothread || !iothread->ctx) {
+ return 0;
+ }
+ iothread_stop(iothread);
return 0;
}
@@ -108,7 +116,7 @@ static void iothread_instance_finalize(Object *obj)
{
IOThread *iothread = IOTHREAD(obj);
- iothread_stop(obj, NULL);
+ iothread_stop(iothread);
qemu_cond_destroy(&iothread->init_done_cond);
qemu_mutex_destroy(&iothread->init_done_lock);
if (!iothread->ctx) {
@@ -328,7 +336,7 @@ void iothread_stop_all(void)
aio_context_release(ctx);
}
- object_child_foreach(container, iothread_stop, NULL);
+ object_child_foreach(container, iothread_stop_iter, NULL);
}
static gpointer iothread_g_main_context_init(gpointer opaque)
--
2.7.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 3/3] iothread: delay the context release to finalize
2017-09-22 8:56 [Qemu-devel] [PATCH 0/3] iothread: allow to create internal iothreads Peter Xu
2017-09-22 8:56 ` [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use Peter Xu
2017-09-22 8:56 ` [Qemu-devel] [PATCH 2/3] iothread: export iothread_stop() Peter Xu
@ 2017-09-22 8:56 ` Peter Xu
2017-09-22 13:09 ` Fam Zheng
2 siblings, 1 reply; 30+ messages in thread
From: Peter Xu @ 2017-09-22 8:56 UTC (permalink / raw
To: qemu-devel
Cc: Paolo Bonzini, Daniel P . Berrange, Stefan Hajnoczi, Fam Zheng,
Dr . David Alan Gilbert, peterx
When gcontext is used with iothread, the context will be destroyed
during iothread_stop(). That's not good since sometimes we would like
to keep the resources until iothread is destroyed, but we may want to
stop the thread before that point.
Delay the destruction of gcontext to iothread finalize. Then we can do:
iothread_stop(thread);
some_cleanup_on_resources();
iothread_destroy(thread);
Signed-off-by: Peter Xu <peterx@redhat.com>
---
iothread.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/iothread.c b/iothread.c
index 894756b..b95369b 100644
--- a/iothread.c
+++ b/iothread.c
@@ -71,8 +71,6 @@ static void *iothread_run(void *opaque)
g_main_loop_unref(loop);
g_main_context_pop_thread_default(iothread->worker_context);
- g_main_context_unref(iothread->worker_context);
- iothread->worker_context = NULL;
}
}
@@ -117,6 +115,10 @@ static void iothread_instance_finalize(Object *obj)
IOThread *iothread = IOTHREAD(obj);
iothread_stop(iothread);
+ if (iothread->worker_context) {
+ g_main_context_unref(iothread->worker_context);
+ iothread->worker_context = NULL;
+ }
qemu_cond_destroy(&iothread->init_done_cond);
qemu_mutex_destroy(&iothread->init_done_lock);
if (!iothread->ctx) {
--
2.7.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 8:56 ` [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use Peter Xu
@ 2017-09-22 9:04 ` Daniel P. Berrange
2017-09-22 9:14 ` Peter Xu
2017-09-22 10:16 ` Stefan Hajnoczi
1 sibling, 1 reply; 30+ messages in thread
From: Daniel P. Berrange @ 2017-09-22 9:04 UTC (permalink / raw
To: Peter Xu
Cc: qemu-devel, Paolo Bonzini, Stefan Hajnoczi, Fam Zheng,
Dr . David Alan Gilbert
On Fri, Sep 22, 2017 at 04:56:10PM +0800, Peter Xu wrote:
> IOThread is a general framework that contains IO loop environment and a
> real thread behind. It's also good to be used internally inside qemu.
> Provide some helpers for it to create iothreads to be used internally.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> include/sysemu/iothread.h | 8 ++++++++
> iothread.c | 21 +++++++++++++++++++++
> 2 files changed, 29 insertions(+)
>
> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
> index d2985b3..b07663f 100644
> --- a/include/sysemu/iothread.h
> +++ b/include/sysemu/iothread.h
> @@ -46,4 +46,12 @@ AioContext *iothread_get_aio_context(IOThread *iothread);
> void iothread_stop_all(void);
> GMainContext *iothread_get_g_main_context(IOThread *iothread);
>
> +/*
> + * Helpers used to allocate iothreads for internal use. These
> + * iothreads will not be seen by monitor clients when query using
> + * "query-iothreads".
> + */
> +IOThread *iothread_create(const char *id, Error **errp);
> +void iothread_destroy(IOThread *iothread);
> +
> #endif /* IOTHREAD_H */
> diff --git a/iothread.c b/iothread.c
> index 44c8944..74e400c 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -354,3 +354,24 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread)
>
> return iothread->worker_context;
> }
> +
> +static Object *iothread_get_internal_parent(void)
> +{
> + return container_get(object_get_root(), "/internal-iothreads");
> +}
I tend to think we might benefit from having this generalized in the
QOM API instead. We have object_get_objects_root() for things that
are created by the mgmt app / user via CLI / QMP. A parallel method
object_get_internal_root() could be useful for cases like this where
we want to create user-creatable objects, but not have them be
visible to the mgmt app / user, as that would confuse the mgmt app.
Example for this scenario - libvirt calls query-iothreads to identify
IO thread PIDs, and would get very unhappy if the IOThread used by
the monitor would appear in that response, which is why Peter has
put it under /internal-iothreads. I think this scenario will apply
more broadly, so benefit from us having a general helper in QOM.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 9:04 ` Daniel P. Berrange
@ 2017-09-22 9:14 ` Peter Xu
2017-09-22 9:36 ` Daniel P. Berrange
0 siblings, 1 reply; 30+ messages in thread
From: Peter Xu @ 2017-09-22 9:14 UTC (permalink / raw
To: Daniel P. Berrange
Cc: qemu-devel, Paolo Bonzini, Stefan Hajnoczi, Fam Zheng,
Dr . David Alan Gilbert
On Fri, Sep 22, 2017 at 10:04:33AM +0100, Daniel P. Berrange wrote:
> On Fri, Sep 22, 2017 at 04:56:10PM +0800, Peter Xu wrote:
> > IOThread is a general framework that contains IO loop environment and a
> > real thread behind. It's also good to be used internally inside qemu.
> > Provide some helpers for it to create iothreads to be used internally.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > include/sysemu/iothread.h | 8 ++++++++
> > iothread.c | 21 +++++++++++++++++++++
> > 2 files changed, 29 insertions(+)
> >
> > diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
> > index d2985b3..b07663f 100644
> > --- a/include/sysemu/iothread.h
> > +++ b/include/sysemu/iothread.h
> > @@ -46,4 +46,12 @@ AioContext *iothread_get_aio_context(IOThread *iothread);
> > void iothread_stop_all(void);
> > GMainContext *iothread_get_g_main_context(IOThread *iothread);
> >
> > +/*
> > + * Helpers used to allocate iothreads for internal use. These
> > + * iothreads will not be seen by monitor clients when query using
> > + * "query-iothreads".
> > + */
> > +IOThread *iothread_create(const char *id, Error **errp);
> > +void iothread_destroy(IOThread *iothread);
> > +
> > #endif /* IOTHREAD_H */
> > diff --git a/iothread.c b/iothread.c
> > index 44c8944..74e400c 100644
> > --- a/iothread.c
> > +++ b/iothread.c
> > @@ -354,3 +354,24 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread)
> >
> > return iothread->worker_context;
> > }
> > +
> > +static Object *iothread_get_internal_parent(void)
> > +{
> > + return container_get(object_get_root(), "/internal-iothreads");
> > +}
>
> I tend to think we might benefit from having this generalized in the
> QOM API instead. We have object_get_objects_root() for things that
> are created by the mgmt app / user via CLI / QMP. A parallel method
> object_get_internal_root() could be useful for cases like this where
> we want to create user-creatable objects, but not have them be
> visible to the mgmt app / user, as that would confuse the mgmt app.
>
> Example for this scenario - libvirt calls query-iothreads to identify
> IO thread PIDs, and would get very unhappy if the IOThread used by
> the monitor would appear in that response, which is why Peter has
> put it under /internal-iothreads. I think this scenario will apply
> more broadly, so benefit from us having a general helper in QOM.
Yeah, I can split the patch if we want it to be exposed to public.
In that case, would the name "object_get_internal_root" be good?
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 9:14 ` Peter Xu
@ 2017-09-22 9:36 ` Daniel P. Berrange
2017-09-22 9:38 ` Paolo Bonzini
0 siblings, 1 reply; 30+ messages in thread
From: Daniel P. Berrange @ 2017-09-22 9:36 UTC (permalink / raw
To: Peter Xu
Cc: qemu-devel, Paolo Bonzini, Stefan Hajnoczi, Fam Zheng,
Dr . David Alan Gilbert
On Fri, Sep 22, 2017 at 05:14:30PM +0800, Peter Xu wrote:
> On Fri, Sep 22, 2017 at 10:04:33AM +0100, Daniel P. Berrange wrote:
> > On Fri, Sep 22, 2017 at 04:56:10PM +0800, Peter Xu wrote:
> > > IOThread is a general framework that contains IO loop environment and a
> > > real thread behind. It's also good to be used internally inside qemu.
> > > Provide some helpers for it to create iothreads to be used internally.
> > >
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > > include/sysemu/iothread.h | 8 ++++++++
> > > iothread.c | 21 +++++++++++++++++++++
> > > 2 files changed, 29 insertions(+)
> > >
> > > diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
> > > index d2985b3..b07663f 100644
> > > --- a/include/sysemu/iothread.h
> > > +++ b/include/sysemu/iothread.h
> > > @@ -46,4 +46,12 @@ AioContext *iothread_get_aio_context(IOThread *iothread);
> > > void iothread_stop_all(void);
> > > GMainContext *iothread_get_g_main_context(IOThread *iothread);
> > >
> > > +/*
> > > + * Helpers used to allocate iothreads for internal use. These
> > > + * iothreads will not be seen by monitor clients when query using
> > > + * "query-iothreads".
> > > + */
> > > +IOThread *iothread_create(const char *id, Error **errp);
> > > +void iothread_destroy(IOThread *iothread);
> > > +
> > > #endif /* IOTHREAD_H */
> > > diff --git a/iothread.c b/iothread.c
> > > index 44c8944..74e400c 100644
> > > --- a/iothread.c
> > > +++ b/iothread.c
> > > @@ -354,3 +354,24 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread)
> > >
> > > return iothread->worker_context;
> > > }
> > > +
> > > +static Object *iothread_get_internal_parent(void)
> > > +{
> > > + return container_get(object_get_root(), "/internal-iothreads");
> > > +}
> >
> > I tend to think we might benefit from having this generalized in the
> > QOM API instead. We have object_get_objects_root() for things that
> > are created by the mgmt app / user via CLI / QMP. A parallel method
> > object_get_internal_root() could be useful for cases like this where
> > we want to create user-creatable objects, but not have them be
> > visible to the mgmt app / user, as that would confuse the mgmt app.
> >
> > Example for this scenario - libvirt calls query-iothreads to identify
> > IO thread PIDs, and would get very unhappy if the IOThread used by
> > the monitor would appear in that response, which is why Peter has
> > put it under /internal-iothreads. I think this scenario will apply
> > more broadly, so benefit from us having a general helper in QOM.
>
> Yeah, I can split the patch if we want it to be exposed to public.
>
> In that case, would the name "object_get_internal_root" be good?
Yeah that's fine with me, but lets see if Paolo has any thoughts from
the QOM side.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 9:36 ` Daniel P. Berrange
@ 2017-09-22 9:38 ` Paolo Bonzini
2017-09-22 9:43 ` Daniel P. Berrange
0 siblings, 1 reply; 30+ messages in thread
From: Paolo Bonzini @ 2017-09-22 9:38 UTC (permalink / raw
To: Daniel P. Berrange, Peter Xu
Cc: qemu-devel, Stefan Hajnoczi, Fam Zheng, Dr . David Alan Gilbert
On 22/09/2017 11:36, Daniel P. Berrange wrote:
> On Fri, Sep 22, 2017 at 05:14:30PM +0800, Peter Xu wrote:
>> On Fri, Sep 22, 2017 at 10:04:33AM +0100, Daniel P. Berrange wrote:
>>> On Fri, Sep 22, 2017 at 04:56:10PM +0800, Peter Xu wrote:
>>>> IOThread is a general framework that contains IO loop environment and a
>>>> real thread behind. It's also good to be used internally inside qemu.
>>>> Provide some helpers for it to create iothreads to be used internally.
>>>>
>>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>>> ---
>>>> include/sysemu/iothread.h | 8 ++++++++
>>>> iothread.c | 21 +++++++++++++++++++++
>>>> 2 files changed, 29 insertions(+)
>>>>
>>>> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
>>>> index d2985b3..b07663f 100644
>>>> --- a/include/sysemu/iothread.h
>>>> +++ b/include/sysemu/iothread.h
>>>> @@ -46,4 +46,12 @@ AioContext *iothread_get_aio_context(IOThread *iothread);
>>>> void iothread_stop_all(void);
>>>> GMainContext *iothread_get_g_main_context(IOThread *iothread);
>>>>
>>>> +/*
>>>> + * Helpers used to allocate iothreads for internal use. These
>>>> + * iothreads will not be seen by monitor clients when query using
>>>> + * "query-iothreads".
>>>> + */
>>>> +IOThread *iothread_create(const char *id, Error **errp);
>>>> +void iothread_destroy(IOThread *iothread);
>>>> +
>>>> #endif /* IOTHREAD_H */
>>>> diff --git a/iothread.c b/iothread.c
>>>> index 44c8944..74e400c 100644
>>>> --- a/iothread.c
>>>> +++ b/iothread.c
>>>> @@ -354,3 +354,24 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread)
>>>>
>>>> return iothread->worker_context;
>>>> }
>>>> +
>>>> +static Object *iothread_get_internal_parent(void)
>>>> +{
>>>> + return container_get(object_get_root(), "/internal-iothreads");
>>>> +}
>>>
>>> I tend to think we might benefit from having this generalized in the
>>> QOM API instead. We have object_get_objects_root() for things that
>>> are created by the mgmt app / user via CLI / QMP. A parallel method
>>> object_get_internal_root() could be useful for cases like this where
>>> we want to create user-creatable objects, but not have them be
>>> visible to the mgmt app / user, as that would confuse the mgmt app.
>>>
>>> Example for this scenario - libvirt calls query-iothreads to identify
>>> IO thread PIDs, and would get very unhappy if the IOThread used by
>>> the monitor would appear in that response, which is why Peter has
>>> put it under /internal-iothreads. I think this scenario will apply
>>> more broadly, so benefit from us having a general helper in QOM.
>>
>> Yeah, I can split the patch if we want it to be exposed to public.
>>
>> In that case, would the name "object_get_internal_root" be good?
>
> Yeah that's fine with me, but lets see if Paolo has any thoughts from
> the QOM side.
No, I don't. However, I wonder if query-iothreads should have an
argument to include internal iothreads.
Paolo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 9:38 ` Paolo Bonzini
@ 2017-09-22 9:43 ` Daniel P. Berrange
2017-09-22 10:14 ` Paolo Bonzini
0 siblings, 1 reply; 30+ messages in thread
From: Daniel P. Berrange @ 2017-09-22 9:43 UTC (permalink / raw
To: Paolo Bonzini
Cc: Peter Xu, qemu-devel, Stefan Hajnoczi, Fam Zheng,
Dr . David Alan Gilbert
On Fri, Sep 22, 2017 at 11:38:31AM +0200, Paolo Bonzini wrote:
> On 22/09/2017 11:36, Daniel P. Berrange wrote:
> > On Fri, Sep 22, 2017 at 05:14:30PM +0800, Peter Xu wrote:
> >> On Fri, Sep 22, 2017 at 10:04:33AM +0100, Daniel P. Berrange wrote:
> >>> On Fri, Sep 22, 2017 at 04:56:10PM +0800, Peter Xu wrote:
> >>>> IOThread is a general framework that contains IO loop environment and a
> >>>> real thread behind. It's also good to be used internally inside qemu.
> >>>> Provide some helpers for it to create iothreads to be used internally.
> >>>>
> >>>> Signed-off-by: Peter Xu <peterx@redhat.com>
> >>>> ---
> >>>> include/sysemu/iothread.h | 8 ++++++++
> >>>> iothread.c | 21 +++++++++++++++++++++
> >>>> 2 files changed, 29 insertions(+)
> >>>>
> >>>> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
> >>>> index d2985b3..b07663f 100644
> >>>> --- a/include/sysemu/iothread.h
> >>>> +++ b/include/sysemu/iothread.h
> >>>> @@ -46,4 +46,12 @@ AioContext *iothread_get_aio_context(IOThread *iothread);
> >>>> void iothread_stop_all(void);
> >>>> GMainContext *iothread_get_g_main_context(IOThread *iothread);
> >>>>
> >>>> +/*
> >>>> + * Helpers used to allocate iothreads for internal use. These
> >>>> + * iothreads will not be seen by monitor clients when query using
> >>>> + * "query-iothreads".
> >>>> + */
> >>>> +IOThread *iothread_create(const char *id, Error **errp);
> >>>> +void iothread_destroy(IOThread *iothread);
> >>>> +
> >>>> #endif /* IOTHREAD_H */
> >>>> diff --git a/iothread.c b/iothread.c
> >>>> index 44c8944..74e400c 100644
> >>>> --- a/iothread.c
> >>>> +++ b/iothread.c
> >>>> @@ -354,3 +354,24 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread)
> >>>>
> >>>> return iothread->worker_context;
> >>>> }
> >>>> +
> >>>> +static Object *iothread_get_internal_parent(void)
> >>>> +{
> >>>> + return container_get(object_get_root(), "/internal-iothreads");
> >>>> +}
> >>>
> >>> I tend to think we might benefit from having this generalized in the
> >>> QOM API instead. We have object_get_objects_root() for things that
> >>> are created by the mgmt app / user via CLI / QMP. A parallel method
> >>> object_get_internal_root() could be useful for cases like this where
> >>> we want to create user-creatable objects, but not have them be
> >>> visible to the mgmt app / user, as that would confuse the mgmt app.
> >>>
> >>> Example for this scenario - libvirt calls query-iothreads to identify
> >>> IO thread PIDs, and would get very unhappy if the IOThread used by
> >>> the monitor would appear in that response, which is why Peter has
> >>> put it under /internal-iothreads. I think this scenario will apply
> >>> more broadly, so benefit from us having a general helper in QOM.
> >>
> >> Yeah, I can split the patch if we want it to be exposed to public.
> >>
> >> In that case, would the name "object_get_internal_root" be good?
> >
> > Yeah that's fine with me, but lets see if Paolo has any thoughts from
> > the QOM side.
>
> No, I don't. However, I wonder if query-iothreads should have an
> argument to include internal iothreads.
I guess it depends whether we consider use of iothreads for migration
to be a private impl detail that may be ripped out & replaced at any
time, or a semi-public detail that apps may rely on. Personally I
would suggest it should be a private impl detail of migration code
that is never exposed to anything outside QEMU, so we have flexibility
to change it at will later.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 9:43 ` Daniel P. Berrange
@ 2017-09-22 10:14 ` Paolo Bonzini
2017-09-22 10:17 ` Daniel P. Berrange
2017-09-22 10:54 ` Peter Xu
0 siblings, 2 replies; 30+ messages in thread
From: Paolo Bonzini @ 2017-09-22 10:14 UTC (permalink / raw
To: Daniel P. Berrange
Cc: Peter Xu, qemu-devel, Stefan Hajnoczi, Fam Zheng,
Dr . David Alan Gilbert
On 22/09/2017 11:43, Daniel P. Berrange wrote:
> On Fri, Sep 22, 2017 at 11:38:31AM +0200, Paolo Bonzini wrote:
>> On 22/09/2017 11:36, Daniel P. Berrange wrote:
>>> On Fri, Sep 22, 2017 at 05:14:30PM +0800, Peter Xu wrote:
>>>> On Fri, Sep 22, 2017 at 10:04:33AM +0100, Daniel P. Berrange wrote:
>>>>> On Fri, Sep 22, 2017 at 04:56:10PM +0800, Peter Xu wrote:
>>>>>> IOThread is a general framework that contains IO loop environment and a
>>>>>> real thread behind. It's also good to be used internally inside qemu.
>>>>>> Provide some helpers for it to create iothreads to be used internally.
>>>>>>
>>>>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>>>>> ---
>>>>>> include/sysemu/iothread.h | 8 ++++++++
>>>>>> iothread.c | 21 +++++++++++++++++++++
>>>>>> 2 files changed, 29 insertions(+)
>>>>>>
>>>>>> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
>>>>>> index d2985b3..b07663f 100644
>>>>>> --- a/include/sysemu/iothread.h
>>>>>> +++ b/include/sysemu/iothread.h
>>>>>> @@ -46,4 +46,12 @@ AioContext *iothread_get_aio_context(IOThread *iothread);
>>>>>> void iothread_stop_all(void);
>>>>>> GMainContext *iothread_get_g_main_context(IOThread *iothread);
>>>>>>
>>>>>> +/*
>>>>>> + * Helpers used to allocate iothreads for internal use. These
>>>>>> + * iothreads will not be seen by monitor clients when query using
>>>>>> + * "query-iothreads".
>>>>>> + */
>>>>>> +IOThread *iothread_create(const char *id, Error **errp);
>>>>>> +void iothread_destroy(IOThread *iothread);
>>>>>> +
>>>>>> #endif /* IOTHREAD_H */
>>>>>> diff --git a/iothread.c b/iothread.c
>>>>>> index 44c8944..74e400c 100644
>>>>>> --- a/iothread.c
>>>>>> +++ b/iothread.c
>>>>>> @@ -354,3 +354,24 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread)
>>>>>>
>>>>>> return iothread->worker_context;
>>>>>> }
>>>>>> +
>>>>>> +static Object *iothread_get_internal_parent(void)
>>>>>> +{
>>>>>> + return container_get(object_get_root(), "/internal-iothreads");
>>>>>> +}
>>>>>
>>>>> I tend to think we might benefit from having this generalized in the
>>>>> QOM API instead. We have object_get_objects_root() for things that
>>>>> are created by the mgmt app / user via CLI / QMP. A parallel method
>>>>> object_get_internal_root() could be useful for cases like this where
>>>>> we want to create user-creatable objects, but not have them be
>>>>> visible to the mgmt app / user, as that would confuse the mgmt app.
>>>>>
>>>>> Example for this scenario - libvirt calls query-iothreads to identify
>>>>> IO thread PIDs, and would get very unhappy if the IOThread used by
>>>>> the monitor would appear in that response, which is why Peter has
>>>>> put it under /internal-iothreads. I think this scenario will apply
>>>>> more broadly, so benefit from us having a general helper in QOM.
>>>>
>>>> Yeah, I can split the patch if we want it to be exposed to public.
>>>>
>>>> In that case, would the name "object_get_internal_root" be good?
>>>
>>> Yeah that's fine with me, but lets see if Paolo has any thoughts from
>>> the QOM side.
>>
>> No, I don't. However, I wonder if query-iothreads should have an
>> argument to include internal iothreads.
>
> I guess it depends whether we consider use of iothreads for migration
> to be a private impl detail that may be ripped out & replaced at any
> time, or a semi-public detail that apps may rely on. Personally I
> would suggest it should be a private impl detail of migration code
> that is never exposed to anything outside QEMU, so we have flexibility
> to change it at will later.
As far as I understood it's not migration, it's the whole monitor that
moves to the iothread. So it's a thing that lasts for the whole
existence of the QEMU process. But I may be wrong.
Paolo
Paolo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 8:56 ` [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use Peter Xu
2017-09-22 9:04 ` Daniel P. Berrange
@ 2017-09-22 10:16 ` Stefan Hajnoczi
2017-09-22 10:18 ` Paolo Bonzini
2017-10-02 17:18 ` Markus Armbruster
1 sibling, 2 replies; 30+ messages in thread
From: Stefan Hajnoczi @ 2017-09-22 10:16 UTC (permalink / raw
To: Markus Armbruster
Cc: qemu-devel, Paolo Bonzini, Daniel P . Berrange, Stefan Hajnoczi,
Fam Zheng, Dr . David Alan Gilbert, Peter Xu
On Fri, Sep 22, 2017 at 04:56:10PM +0800, Peter Xu wrote:
> IOThread is a general framework that contains IO loop environment and a
> real thread behind. It's also good to be used internally inside qemu.
> Provide some helpers for it to create iothreads to be used internally.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> include/sysemu/iothread.h | 8 ++++++++
> iothread.c | 21 +++++++++++++++++++++
> 2 files changed, 29 insertions(+)
>
> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
> index d2985b3..b07663f 100644
> --- a/include/sysemu/iothread.h
> +++ b/include/sysemu/iothread.h
> @@ -46,4 +46,12 @@ AioContext *iothread_get_aio_context(IOThread *iothread);
> void iothread_stop_all(void);
> GMainContext *iothread_get_g_main_context(IOThread *iothread);
>
> +/*
> + * Helpers used to allocate iothreads for internal use. These
> + * iothreads will not be seen by monitor clients when query using
> + * "query-iothreads".
> + */
> +IOThread *iothread_create(const char *id, Error **errp);
> +void iothread_destroy(IOThread *iothread);
> +
> #endif /* IOTHREAD_H */
> diff --git a/iothread.c b/iothread.c
> index 44c8944..74e400c 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -354,3 +354,24 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread)
>
> return iothread->worker_context;
> }
> +
> +static Object *iothread_get_internal_parent(void)
> +{
> + return container_get(object_get_root(), "/internal-iothreads");
> +}
Markus, please advise on the following QMP API design issue.
The query-iothreads QMP command lists IOThread objects. This patch
introduces "internal" IOThreads whose lifetime is dictated by QEMU, not
the user's command-line or object-add command.
QMP clients might still want to know the thread IDs of internal
IOThreads for CPU affinity. Clients may also want to tweak the
AioContext polling properties on the internal objects for performance
tuning.
I suggest adding internal IOThreads alongside user-created IOThreads
instead of hiding them. IOThread also needs a bool user_created field
and a UserCreatableClass->can_be_deleted() function:
static bool iothread_can_be_deleted(UserCreatable *uc)
{
return IOTHREAD(uc)->user_created;
}
This way users cannot delete internal IOThreads.
But how should object ids be handled? In theory existing -object
iothread,id=<id> users could use any name. How can QEMU generate ids
for internal IOThreads without conflicting with existing users's ids?
> +
> +IOThread *iothread_create(const char *id, Error **errp)
> +{
> + Object *obj;
> +
> + obj = object_new_with_props(TYPE_IOTHREAD,
> + iothread_get_internal_parent(),
> + id, errp, NULL);
> +
> + return IOTHREAD(obj);
> +}
> +
> +void iothread_destroy(IOThread *iothread)
> +{
> + object_unparent(OBJECT(iothread));
> +}
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 10:14 ` Paolo Bonzini
@ 2017-09-22 10:17 ` Daniel P. Berrange
2017-09-22 12:59 ` Fam Zheng
2017-09-22 10:54 ` Peter Xu
1 sibling, 1 reply; 30+ messages in thread
From: Daniel P. Berrange @ 2017-09-22 10:17 UTC (permalink / raw
To: Paolo Bonzini
Cc: Peter Xu, qemu-devel, Stefan Hajnoczi, Fam Zheng,
Dr . David Alan Gilbert
On Fri, Sep 22, 2017 at 12:14:04PM +0200, Paolo Bonzini wrote:
> On 22/09/2017 11:43, Daniel P. Berrange wrote:
> > On Fri, Sep 22, 2017 at 11:38:31AM +0200, Paolo Bonzini wrote:
> >> On 22/09/2017 11:36, Daniel P. Berrange wrote:
> >>> On Fri, Sep 22, 2017 at 05:14:30PM +0800, Peter Xu wrote:
> >>>> On Fri, Sep 22, 2017 at 10:04:33AM +0100, Daniel P. Berrange wrote:
> >>>>> On Fri, Sep 22, 2017 at 04:56:10PM +0800, Peter Xu wrote:
> >>>>>> IOThread is a general framework that contains IO loop environment and a
> >>>>>> real thread behind. It's also good to be used internally inside qemu.
> >>>>>> Provide some helpers for it to create iothreads to be used internally.
> >>>>>>
> >>>>>> Signed-off-by: Peter Xu <peterx@redhat.com>
> >>>>>> ---
> >>>>>> include/sysemu/iothread.h | 8 ++++++++
> >>>>>> iothread.c | 21 +++++++++++++++++++++
> >>>>>> 2 files changed, 29 insertions(+)
> >>>>>>
> >>>>>> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
> >>>>>> index d2985b3..b07663f 100644
> >>>>>> --- a/include/sysemu/iothread.h
> >>>>>> +++ b/include/sysemu/iothread.h
> >>>>>> @@ -46,4 +46,12 @@ AioContext *iothread_get_aio_context(IOThread *iothread);
> >>>>>> void iothread_stop_all(void);
> >>>>>> GMainContext *iothread_get_g_main_context(IOThread *iothread);
> >>>>>>
> >>>>>> +/*
> >>>>>> + * Helpers used to allocate iothreads for internal use. These
> >>>>>> + * iothreads will not be seen by monitor clients when query using
> >>>>>> + * "query-iothreads".
> >>>>>> + */
> >>>>>> +IOThread *iothread_create(const char *id, Error **errp);
> >>>>>> +void iothread_destroy(IOThread *iothread);
> >>>>>> +
> >>>>>> #endif /* IOTHREAD_H */
> >>>>>> diff --git a/iothread.c b/iothread.c
> >>>>>> index 44c8944..74e400c 100644
> >>>>>> --- a/iothread.c
> >>>>>> +++ b/iothread.c
> >>>>>> @@ -354,3 +354,24 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread)
> >>>>>>
> >>>>>> return iothread->worker_context;
> >>>>>> }
> >>>>>> +
> >>>>>> +static Object *iothread_get_internal_parent(void)
> >>>>>> +{
> >>>>>> + return container_get(object_get_root(), "/internal-iothreads");
> >>>>>> +}
> >>>>>
> >>>>> I tend to think we might benefit from having this generalized in the
> >>>>> QOM API instead. We have object_get_objects_root() for things that
> >>>>> are created by the mgmt app / user via CLI / QMP. A parallel method
> >>>>> object_get_internal_root() could be useful for cases like this where
> >>>>> we want to create user-creatable objects, but not have them be
> >>>>> visible to the mgmt app / user, as that would confuse the mgmt app.
> >>>>>
> >>>>> Example for this scenario - libvirt calls query-iothreads to identify
> >>>>> IO thread PIDs, and would get very unhappy if the IOThread used by
> >>>>> the monitor would appear in that response, which is why Peter has
> >>>>> put it under /internal-iothreads. I think this scenario will apply
> >>>>> more broadly, so benefit from us having a general helper in QOM.
> >>>>
> >>>> Yeah, I can split the patch if we want it to be exposed to public.
> >>>>
> >>>> In that case, would the name "object_get_internal_root" be good?
> >>>
> >>> Yeah that's fine with me, but lets see if Paolo has any thoughts from
> >>> the QOM side.
> >>
> >> No, I don't. However, I wonder if query-iothreads should have an
> >> argument to include internal iothreads.
> >
> > I guess it depends whether we consider use of iothreads for migration
> > to be a private impl detail that may be ripped out & replaced at any
> > time, or a semi-public detail that apps may rely on. Personally I
> > would suggest it should be a private impl detail of migration code
> > that is never exposed to anything outside QEMU, so we have flexibility
> > to change it at will later.
>
> As far as I understood it's not migration, it's the whole monitor that
> moves to the iothread. So it's a thing that lasts for the whole
> existence of the QEMU process. But I may be wrong.
Sorry yes, my bad - its the iothread behind the monitor. I still think that
is a detail worth keeping private in case we want to refactor how the
monitor threading works later.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 10:16 ` Stefan Hajnoczi
@ 2017-09-22 10:18 ` Paolo Bonzini
2017-09-22 10:20 ` Daniel P. Berrange
2017-10-02 17:18 ` Markus Armbruster
1 sibling, 1 reply; 30+ messages in thread
From: Paolo Bonzini @ 2017-09-22 10:18 UTC (permalink / raw
To: Stefan Hajnoczi, Markus Armbruster
Cc: qemu-devel, Daniel P . Berrange, Stefan Hajnoczi, Fam Zheng,
Dr . David Alan Gilbert, Peter Xu
On 22/09/2017 12:16, Stefan Hajnoczi wrote:
> I suggest adding internal IOThreads alongside user-created IOThreads
> instead of hiding them. IOThread also needs a bool user_created field
> and a UserCreatableClass->can_be_deleted() function:
>
> static bool iothread_can_be_deleted(UserCreatable *uc)
> {
> return IOTHREAD(uc)->user_created;
> }
>
> This way users cannot delete internal IOThreads.
>
> But how should object ids be handled? In theory existing -object
> iothread,id=<id> users could use any name. How can QEMU generate ids
> for internal IOThreads without conflicting with existing users's ids?
I would add an 'internal' boolean to query-iothreads' response and a new
'show-internal' boolean to the command. This way, applications that
request internal iothreads would know that the "primary key" is
(internal, id) rather than just the id.
Paolo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 10:18 ` Paolo Bonzini
@ 2017-09-22 10:20 ` Daniel P. Berrange
2017-09-22 10:26 ` Paolo Bonzini
0 siblings, 1 reply; 30+ messages in thread
From: Daniel P. Berrange @ 2017-09-22 10:20 UTC (permalink / raw
To: Paolo Bonzini
Cc: Stefan Hajnoczi, Markus Armbruster, qemu-devel, Stefan Hajnoczi,
Fam Zheng, Dr . David Alan Gilbert, Peter Xu
On Fri, Sep 22, 2017 at 12:18:44PM +0200, Paolo Bonzini wrote:
> On 22/09/2017 12:16, Stefan Hajnoczi wrote:
> > I suggest adding internal IOThreads alongside user-created IOThreads
> > instead of hiding them. IOThread also needs a bool user_created field
> > and a UserCreatableClass->can_be_deleted() function:
> >
> > static bool iothread_can_be_deleted(UserCreatable *uc)
> > {
> > return IOTHREAD(uc)->user_created;
> > }
> >
> > This way users cannot delete internal IOThreads.
> >
> > But how should object ids be handled? In theory existing -object
> > iothread,id=<id> users could use any name. How can QEMU generate ids
> > for internal IOThreads without conflicting with existing users's ids?
>
> I would add an 'internal' boolean to query-iothreads' response and a new
> 'show-internal' boolean to the command. This way, applications that
> request internal iothreads would know that the "primary key" is
> (internal, id) rather than just the id.
What is the app going to do with iothreads if it sees "internal" flag
set ? They have no way of knowing what part of QEMU internally is using
this iothread, so I don't see that they can do anything intelligent
once they find out they exist.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 10:20 ` Daniel P. Berrange
@ 2017-09-22 10:26 ` Paolo Bonzini
2017-09-22 10:28 ` Daniel P. Berrange
0 siblings, 1 reply; 30+ messages in thread
From: Paolo Bonzini @ 2017-09-22 10:26 UTC (permalink / raw
To: Daniel P. Berrange
Cc: Stefan Hajnoczi, Markus Armbruster, qemu-devel, Stefan Hajnoczi,
Fam Zheng, Dr . David Alan Gilbert, Peter Xu
On 22/09/2017 12:20, Daniel P. Berrange wrote:
> On Fri, Sep 22, 2017 at 12:18:44PM +0200, Paolo Bonzini wrote:
>> On 22/09/2017 12:16, Stefan Hajnoczi wrote:
>>> I suggest adding internal IOThreads alongside user-created IOThreads
>>> instead of hiding them. IOThread also needs a bool user_created field
>>> and a UserCreatableClass->can_be_deleted() function:
>>>
>>> static bool iothread_can_be_deleted(UserCreatable *uc)
>>> {
>>> return IOTHREAD(uc)->user_created;
>>> }
>>>
>>> This way users cannot delete internal IOThreads.
>>>
>>> But how should object ids be handled? In theory existing -object
>>> iothread,id=<id> users could use any name. How can QEMU generate ids
>>> for internal IOThreads without conflicting with existing users's ids?
>>
>> I would add an 'internal' boolean to query-iothreads' response and a new
>> 'show-internal' boolean to the command. This way, applications that
>> request internal iothreads would know that the "primary key" is
>> (internal, id) rather than just the id.
>
> What is the app going to do with iothreads if it sees "internal" flag
> set ? They have no way of knowing what part of QEMU internally is using
> this iothread, so I don't see that they can do anything intelligent
> once they find out they exist.
The application could apply them default settings for scheduler policy
or CPU affinity.
Unlike the main or the I/O thread, the monitor thread doesn't interrupt
the CPU, so it need not run at SCHED_FIFO even in real-time settings.
Alternatively, the application could ensure that such threads would not
get in the way of VCPU or I/O threads, providing slightly more stable
performance.
Paolo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 10:26 ` Paolo Bonzini
@ 2017-09-22 10:28 ` Daniel P. Berrange
2017-09-22 14:35 ` Stefan Hajnoczi
0 siblings, 1 reply; 30+ messages in thread
From: Daniel P. Berrange @ 2017-09-22 10:28 UTC (permalink / raw
To: Paolo Bonzini
Cc: Stefan Hajnoczi, Markus Armbruster, qemu-devel, Stefan Hajnoczi,
Fam Zheng, Dr . David Alan Gilbert, Peter Xu
On Fri, Sep 22, 2017 at 12:26:16PM +0200, Paolo Bonzini wrote:
> On 22/09/2017 12:20, Daniel P. Berrange wrote:
> > On Fri, Sep 22, 2017 at 12:18:44PM +0200, Paolo Bonzini wrote:
> >> On 22/09/2017 12:16, Stefan Hajnoczi wrote:
> >>> I suggest adding internal IOThreads alongside user-created IOThreads
> >>> instead of hiding them. IOThread also needs a bool user_created field
> >>> and a UserCreatableClass->can_be_deleted() function:
> >>>
> >>> static bool iothread_can_be_deleted(UserCreatable *uc)
> >>> {
> >>> return IOTHREAD(uc)->user_created;
> >>> }
> >>>
> >>> This way users cannot delete internal IOThreads.
> >>>
> >>> But how should object ids be handled? In theory existing -object
> >>> iothread,id=<id> users could use any name. How can QEMU generate ids
> >>> for internal IOThreads without conflicting with existing users's ids?
> >>
> >> I would add an 'internal' boolean to query-iothreads' response and a new
> >> 'show-internal' boolean to the command. This way, applications that
> >> request internal iothreads would know that the "primary key" is
> >> (internal, id) rather than just the id.
> >
> > What is the app going to do with iothreads if it sees "internal" flag
> > set ? They have no way of knowing what part of QEMU internally is using
> > this iothread, so I don't see that they can do anything intelligent
> > once they find out they exist.
>
> The application could apply them default settings for scheduler policy
> or CPU affinity.
>
> Unlike the main or the I/O thread, the monitor thread doesn't interrupt
> the CPU, so it need not run at SCHED_FIFO even in real-time settings.
There's no way for the application to know that. ALl it sees is that there
is an extra unexpected IOthread created internally by QEMU. There's no way
to decide whether it nedes SCHED_FIFO or not as we've no clue what it is
being used for.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 10:14 ` Paolo Bonzini
2017-09-22 10:17 ` Daniel P. Berrange
@ 2017-09-22 10:54 ` Peter Xu
1 sibling, 0 replies; 30+ messages in thread
From: Peter Xu @ 2017-09-22 10:54 UTC (permalink / raw
To: Paolo Bonzini
Cc: Daniel P. Berrange, qemu-devel, Stefan Hajnoczi, Fam Zheng,
Dr . David Alan Gilbert
On Fri, Sep 22, 2017 at 12:14:04PM +0200, Paolo Bonzini wrote:
> On 22/09/2017 11:43, Daniel P. Berrange wrote:
> > On Fri, Sep 22, 2017 at 11:38:31AM +0200, Paolo Bonzini wrote:
> >> On 22/09/2017 11:36, Daniel P. Berrange wrote:
> >>> On Fri, Sep 22, 2017 at 05:14:30PM +0800, Peter Xu wrote:
> >>>> On Fri, Sep 22, 2017 at 10:04:33AM +0100, Daniel P. Berrange wrote:
> >>>>> On Fri, Sep 22, 2017 at 04:56:10PM +0800, Peter Xu wrote:
> >>>>>> IOThread is a general framework that contains IO loop environment and a
> >>>>>> real thread behind. It's also good to be used internally inside qemu.
> >>>>>> Provide some helpers for it to create iothreads to be used internally.
> >>>>>>
> >>>>>> Signed-off-by: Peter Xu <peterx@redhat.com>
> >>>>>> ---
> >>>>>> include/sysemu/iothread.h | 8 ++++++++
> >>>>>> iothread.c | 21 +++++++++++++++++++++
> >>>>>> 2 files changed, 29 insertions(+)
> >>>>>>
> >>>>>> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
> >>>>>> index d2985b3..b07663f 100644
> >>>>>> --- a/include/sysemu/iothread.h
> >>>>>> +++ b/include/sysemu/iothread.h
> >>>>>> @@ -46,4 +46,12 @@ AioContext *iothread_get_aio_context(IOThread *iothread);
> >>>>>> void iothread_stop_all(void);
> >>>>>> GMainContext *iothread_get_g_main_context(IOThread *iothread);
> >>>>>>
> >>>>>> +/*
> >>>>>> + * Helpers used to allocate iothreads for internal use. These
> >>>>>> + * iothreads will not be seen by monitor clients when query using
> >>>>>> + * "query-iothreads".
> >>>>>> + */
> >>>>>> +IOThread *iothread_create(const char *id, Error **errp);
> >>>>>> +void iothread_destroy(IOThread *iothread);
> >>>>>> +
> >>>>>> #endif /* IOTHREAD_H */
> >>>>>> diff --git a/iothread.c b/iothread.c
> >>>>>> index 44c8944..74e400c 100644
> >>>>>> --- a/iothread.c
> >>>>>> +++ b/iothread.c
> >>>>>> @@ -354,3 +354,24 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread)
> >>>>>>
> >>>>>> return iothread->worker_context;
> >>>>>> }
> >>>>>> +
> >>>>>> +static Object *iothread_get_internal_parent(void)
> >>>>>> +{
> >>>>>> + return container_get(object_get_root(), "/internal-iothreads");
> >>>>>> +}
> >>>>>
> >>>>> I tend to think we might benefit from having this generalized in the
> >>>>> QOM API instead. We have object_get_objects_root() for things that
> >>>>> are created by the mgmt app / user via CLI / QMP. A parallel method
> >>>>> object_get_internal_root() could be useful for cases like this where
> >>>>> we want to create user-creatable objects, but not have them be
> >>>>> visible to the mgmt app / user, as that would confuse the mgmt app.
> >>>>>
> >>>>> Example for this scenario - libvirt calls query-iothreads to identify
> >>>>> IO thread PIDs, and would get very unhappy if the IOThread used by
> >>>>> the monitor would appear in that response, which is why Peter has
> >>>>> put it under /internal-iothreads. I think this scenario will apply
> >>>>> more broadly, so benefit from us having a general helper in QOM.
> >>>>
> >>>> Yeah, I can split the patch if we want it to be exposed to public.
> >>>>
> >>>> In that case, would the name "object_get_internal_root" be good?
> >>>
> >>> Yeah that's fine with me, but lets see if Paolo has any thoughts from
> >>> the QOM side.
> >>
> >> No, I don't. However, I wonder if query-iothreads should have an
> >> argument to include internal iothreads.
> >
> > I guess it depends whether we consider use of iothreads for migration
> > to be a private impl detail that may be ripped out & replaced at any
> > time, or a semi-public detail that apps may rely on. Personally I
> > would suggest it should be a private impl detail of migration code
> > that is never exposed to anything outside QEMU, so we have flexibility
> > to change it at will later.
>
> As far as I understood it's not migration, it's the whole monitor that
> moves to the iothread. So it's a thing that lasts for the whole
> existence of the QEMU process. But I may be wrong.
Yes it is about the monitor thread.
One thing to mention is that, it's not the whole monitor thread - my
plan is only to handle QMP IOs in that thread (and execute oob
commands only, while oob commands should be really rare). Most of the
QMP command dispatching and execution will still be in main thread
(which I suppose should be most of the real workload).
IMHO I would prefer user know nothing of this thread, or tune it in
any way. Maybe there will be a time when we would like user to tune
monitor threads, but IMHO current OOB threading is not that far yet.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 10:17 ` Daniel P. Berrange
@ 2017-09-22 12:59 ` Fam Zheng
2017-09-22 13:03 ` Paolo Bonzini
0 siblings, 1 reply; 30+ messages in thread
From: Fam Zheng @ 2017-09-22 12:59 UTC (permalink / raw
To: Daniel P. Berrange
Cc: Paolo Bonzini, Peter Xu, qemu-devel, Stefan Hajnoczi,
Dr . David Alan Gilbert
On Fri, 09/22 11:17, Daniel P. Berrange wrote:
> Sorry yes, my bad - its the iothread behind the monitor. I still think that
> is a detail worth keeping private in case we want to refactor how the
> monitor threading works later.
I agree. I convinced Peter to reuse IOThread just because we can, but we don't
need to expose it to user just because we can. Like Daniel, I also see no reason
to do that at this point, and I think we can always do it later when necesssary.
Fam
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 12:59 ` Fam Zheng
@ 2017-09-22 13:03 ` Paolo Bonzini
0 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2017-09-22 13:03 UTC (permalink / raw
To: Fam Zheng, Daniel P. Berrange
Cc: Peter Xu, qemu-devel, Stefan Hajnoczi, Dr . David Alan Gilbert
On 22/09/2017 14:59, Fam Zheng wrote:
> On Fri, 09/22 11:17, Daniel P. Berrange wrote:
>> Sorry yes, my bad - its the iothread behind the monitor. I still think that
>> is a detail worth keeping private in case we want to refactor how the
>> monitor threading works later.
> I agree. I convinced Peter to reuse IOThread just because we can, but we don't
> need to expose it to user just because we can. Like Daniel, I also see no reason
> to do that at this point, and I think we can always do it later when necesssary.
Great. :)
Paolo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] iothread: export iothread_stop()
2017-09-22 8:56 ` [Qemu-devel] [PATCH 2/3] iothread: export iothread_stop() Peter Xu
@ 2017-09-22 13:06 ` Fam Zheng
2017-09-25 3:29 ` Peter Xu
0 siblings, 1 reply; 30+ messages in thread
From: Fam Zheng @ 2017-09-22 13:06 UTC (permalink / raw
To: Peter Xu
Cc: qemu-devel, Dr . David Alan Gilbert, Stefan Hajnoczi,
Paolo Bonzini
On Fri, 09/22 16:56, Peter Xu wrote:
> So that internal iothread users can explicitly stop one iothread without
> destroying it.
>
> Since at it, fix iothread_stop() to allow re-entrance. Before this
I don't think there is any re-entrace here. Maybe you mean
s/re-entrance/calling multiple times/
?
> patch we may call iothread_stop() twice on single iothread, while that
> may not be correct since qemu_thread_join() is not allowed to run twice.
> From manual of pthread_join():
Is one call from iothread_stop_all() and one from object finalize?
>
> Joining with a thread that has previously been joined results in
> undefined behavior.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> include/sysemu/iothread.h | 1 +
> iothread.c | 24 ++++++++++++++++--------
> 2 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
> index b07663f..110329b 100644
> --- a/include/sysemu/iothread.h
> +++ b/include/sysemu/iothread.h
> @@ -52,6 +52,7 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread);
> * "query-iothreads".
> */
> IOThread *iothread_create(const char *id, Error **errp);
> +void iothread_stop(IOThread *iothread);
> void iothread_destroy(IOThread *iothread);
>
> #endif /* IOTHREAD_H */
> diff --git a/iothread.c b/iothread.c
> index 74e400c..894756b 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -80,13 +80,10 @@ static void *iothread_run(void *opaque)
> return NULL;
> }
>
> -static int iothread_stop(Object *object, void *opaque)
> +void iothread_stop(IOThread *iothread)
> {
> - IOThread *iothread;
> -
> - iothread = (IOThread *)object_dynamic_cast(object, TYPE_IOTHREAD);
> - if (!iothread || !iothread->ctx) {
> - return 0;
> + if (iothread->stopping) {
> + return;
> }
> iothread->stopping = true;
> aio_notify(iothread->ctx);
> @@ -94,6 +91,17 @@ static int iothread_stop(Object *object, void *opaque)
> g_main_loop_quit(iothread->main_loop);
> }
> qemu_thread_join(&iothread->thread);
> +}
> +
> +static int iothread_stop_iter(Object *object, void *opaque)
> +{
> + IOThread *iothread;
> +
> + iothread = (IOThread *)object_dynamic_cast(object, TYPE_IOTHREAD);
> + if (!iothread || !iothread->ctx) {
> + return 0;
> + }
I think the check of iothread->ctx can be moved to iothread_stop() too.
Fam
> + iothread_stop(iothread);
> return 0;
> }
>
> @@ -108,7 +116,7 @@ static void iothread_instance_finalize(Object *obj)
> {
> IOThread *iothread = IOTHREAD(obj);
>
> - iothread_stop(obj, NULL);
> + iothread_stop(iothread);
> qemu_cond_destroy(&iothread->init_done_cond);
> qemu_mutex_destroy(&iothread->init_done_lock);
> if (!iothread->ctx) {
> @@ -328,7 +336,7 @@ void iothread_stop_all(void)
> aio_context_release(ctx);
> }
>
> - object_child_foreach(container, iothread_stop, NULL);
> + object_child_foreach(container, iothread_stop_iter, NULL);
> }
>
> static gpointer iothread_g_main_context_init(gpointer opaque)
> --
> 2.7.4
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] iothread: delay the context release to finalize
2017-09-22 8:56 ` [Qemu-devel] [PATCH 3/3] iothread: delay the context release to finalize Peter Xu
@ 2017-09-22 13:09 ` Fam Zheng
2017-09-25 5:23 ` Peter Xu
0 siblings, 1 reply; 30+ messages in thread
From: Fam Zheng @ 2017-09-22 13:09 UTC (permalink / raw
To: Peter Xu
Cc: qemu-devel, Dr . David Alan Gilbert, Stefan Hajnoczi,
Paolo Bonzini
On Fri, 09/22 16:56, Peter Xu wrote:
> When gcontext is used with iothread, the context will be destroyed
> during iothread_stop(). That's not good since sometimes we would like
> to keep the resources until iothread is destroyed, but we may want to
> stop the thread before that point.
Would be nice if you can also mention the glib bug that "required" this in the
commit message.
Reviewed-by: Fam Zheng <famz@redhat.com>
>
> Delay the destruction of gcontext to iothread finalize. Then we can do:
>
> iothread_stop(thread);
> some_cleanup_on_resources();
> iothread_destroy(thread);
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> iothread.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/iothread.c b/iothread.c
> index 894756b..b95369b 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -71,8 +71,6 @@ static void *iothread_run(void *opaque)
> g_main_loop_unref(loop);
>
> g_main_context_pop_thread_default(iothread->worker_context);
> - g_main_context_unref(iothread->worker_context);
> - iothread->worker_context = NULL;
> }
> }
>
> @@ -117,6 +115,10 @@ static void iothread_instance_finalize(Object *obj)
> IOThread *iothread = IOTHREAD(obj);
>
> iothread_stop(iothread);
> + if (iothread->worker_context) {
> + g_main_context_unref(iothread->worker_context);
> + iothread->worker_context = NULL;
> + }
> qemu_cond_destroy(&iothread->init_done_cond);
> qemu_mutex_destroy(&iothread->init_done_lock);
> if (!iothread->ctx) {
> --
> 2.7.4
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 10:28 ` Daniel P. Berrange
@ 2017-09-22 14:35 ` Stefan Hajnoczi
2017-09-22 14:55 ` Daniel P. Berrange
0 siblings, 1 reply; 30+ messages in thread
From: Stefan Hajnoczi @ 2017-09-22 14:35 UTC (permalink / raw
To: Daniel P. Berrange
Cc: Paolo Bonzini, Markus Armbruster, qemu-devel, Stefan Hajnoczi,
Fam Zheng, Dr . David Alan Gilbert, Peter Xu
On Fri, Sep 22, 2017 at 11:28:08AM +0100, Daniel P. Berrange wrote:
> On Fri, Sep 22, 2017 at 12:26:16PM +0200, Paolo Bonzini wrote:
> > On 22/09/2017 12:20, Daniel P. Berrange wrote:
> > > On Fri, Sep 22, 2017 at 12:18:44PM +0200, Paolo Bonzini wrote:
> > >> On 22/09/2017 12:16, Stefan Hajnoczi wrote:
> > >>> I suggest adding internal IOThreads alongside user-created IOThreads
> > >>> instead of hiding them. IOThread also needs a bool user_created field
> > >>> and a UserCreatableClass->can_be_deleted() function:
> > >>>
> > >>> static bool iothread_can_be_deleted(UserCreatable *uc)
> > >>> {
> > >>> return IOTHREAD(uc)->user_created;
> > >>> }
> > >>>
> > >>> This way users cannot delete internal IOThreads.
> > >>>
> > >>> But how should object ids be handled? In theory existing -object
> > >>> iothread,id=<id> users could use any name. How can QEMU generate ids
> > >>> for internal IOThreads without conflicting with existing users's ids?
> > >>
> > >> I would add an 'internal' boolean to query-iothreads' response and a new
> > >> 'show-internal' boolean to the command. This way, applications that
> > >> request internal iothreads would know that the "primary key" is
> > >> (internal, id) rather than just the id.
> > >
> > > What is the app going to do with iothreads if it sees "internal" flag
> > > set ? They have no way of knowing what part of QEMU internally is using
> > > this iothread, so I don't see that they can do anything intelligent
> > > once they find out they exist.
> >
> > The application could apply them default settings for scheduler policy
> > or CPU affinity.
> >
> > Unlike the main or the I/O thread, the monitor thread doesn't interrupt
> > the CPU, so it need not run at SCHED_FIFO even in real-time settings.
>
> There's no way for the application to know that. ALl it sees is that there
> is an extra unexpected IOthread created internally by QEMU. There's no way
> to decide whether it nedes SCHED_FIFO or not as we've no clue what it is
> being used for.
For observability (troubleshooting, debugging, etc) I would like all
IOThreads to be visible. That makes it much easier to troubleshoot in a
unified way rather than having to get at internal IOThreads through
different means (e.g. gdb).
If QEMU uses a well-known ID for the internal monitor IOThread then
libvirt could also interact with it (e.g. CPU affinity).
One day maybe the main loop thread will also be an internal IOThread.
Stefan
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 14:35 ` Stefan Hajnoczi
@ 2017-09-22 14:55 ` Daniel P. Berrange
0 siblings, 0 replies; 30+ messages in thread
From: Daniel P. Berrange @ 2017-09-22 14:55 UTC (permalink / raw
To: Stefan Hajnoczi
Cc: Paolo Bonzini, Markus Armbruster, qemu-devel, Stefan Hajnoczi,
Fam Zheng, Dr . David Alan Gilbert, Peter Xu
On Fri, Sep 22, 2017 at 03:35:05PM +0100, Stefan Hajnoczi wrote:
> On Fri, Sep 22, 2017 at 11:28:08AM +0100, Daniel P. Berrange wrote:
> > On Fri, Sep 22, 2017 at 12:26:16PM +0200, Paolo Bonzini wrote:
> > > On 22/09/2017 12:20, Daniel P. Berrange wrote:
> > > > On Fri, Sep 22, 2017 at 12:18:44PM +0200, Paolo Bonzini wrote:
> > > >> On 22/09/2017 12:16, Stefan Hajnoczi wrote:
> > > >>> I suggest adding internal IOThreads alongside user-created IOThreads
> > > >>> instead of hiding them. IOThread also needs a bool user_created field
> > > >>> and a UserCreatableClass->can_be_deleted() function:
> > > >>>
> > > >>> static bool iothread_can_be_deleted(UserCreatable *uc)
> > > >>> {
> > > >>> return IOTHREAD(uc)->user_created;
> > > >>> }
> > > >>>
> > > >>> This way users cannot delete internal IOThreads.
> > > >>>
> > > >>> But how should object ids be handled? In theory existing -object
> > > >>> iothread,id=<id> users could use any name. How can QEMU generate ids
> > > >>> for internal IOThreads without conflicting with existing users's ids?
> > > >>
> > > >> I would add an 'internal' boolean to query-iothreads' response and a new
> > > >> 'show-internal' boolean to the command. This way, applications that
> > > >> request internal iothreads would know that the "primary key" is
> > > >> (internal, id) rather than just the id.
> > > >
> > > > What is the app going to do with iothreads if it sees "internal" flag
> > > > set ? They have no way of knowing what part of QEMU internally is using
> > > > this iothread, so I don't see that they can do anything intelligent
> > > > once they find out they exist.
> > >
> > > The application could apply them default settings for scheduler policy
> > > or CPU affinity.
> > >
> > > Unlike the main or the I/O thread, the monitor thread doesn't interrupt
> > > the CPU, so it need not run at SCHED_FIFO even in real-time settings.
> >
> > There's no way for the application to know that. ALl it sees is that there
> > is an extra unexpected IOthread created internally by QEMU. There's no way
> > to decide whether it nedes SCHED_FIFO or not as we've no clue what it is
> > being used for.
>
> For observability (troubleshooting, debugging, etc) I would like all
> IOThreads to be visible. That makes it much easier to troubleshoot in a
> unified way rather than having to get at internal IOThreads through
> different means (e.g. gdb).
>
> If QEMU uses a well-known ID for the internal monitor IOThread then
> libvirt could also interact with it (e.g. CPU affinity).
I don't think we should abuse ID values by turning them into stable
ABI - currently they are opaque tokens whose value has no special
meaning and make change at will over time.
If we need to be able to report the IOthread for the monitor, then it
would have to be done via a formal API, but that turns the use of
IOThreads for the monitor into a feature we have to keep indefinitely,
even if we wanted to re-implement it differently later.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] iothread: export iothread_stop()
2017-09-22 13:06 ` Fam Zheng
@ 2017-09-25 3:29 ` Peter Xu
0 siblings, 0 replies; 30+ messages in thread
From: Peter Xu @ 2017-09-25 3:29 UTC (permalink / raw
To: Fam Zheng
Cc: qemu-devel, Dr . David Alan Gilbert, Stefan Hajnoczi,
Paolo Bonzini
On Fri, Sep 22, 2017 at 09:06:26PM +0800, Fam Zheng wrote:
> On Fri, 09/22 16:56, Peter Xu wrote:
> > So that internal iothread users can explicitly stop one iothread without
> > destroying it.
> >
> > Since at it, fix iothread_stop() to allow re-entrance. Before this
>
> I don't think there is any re-entrace here. Maybe you mean
>
> s/re-entrance/calling multiple times/
>
> ?
Yes, you are right.
>
> > patch we may call iothread_stop() twice on single iothread, while that
> > may not be correct since qemu_thread_join() is not allowed to run twice.
> > From manual of pthread_join():
>
> Is one call from iothread_stop_all() and one from object finalize?
Yes.
>
> >
> > Joining with a thread that has previously been joined results in
> > undefined behavior.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > include/sysemu/iothread.h | 1 +
> > iothread.c | 24 ++++++++++++++++--------
> > 2 files changed, 17 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
> > index b07663f..110329b 100644
> > --- a/include/sysemu/iothread.h
> > +++ b/include/sysemu/iothread.h
> > @@ -52,6 +52,7 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread);
> > * "query-iothreads".
> > */
> > IOThread *iothread_create(const char *id, Error **errp);
> > +void iothread_stop(IOThread *iothread);
> > void iothread_destroy(IOThread *iothread);
> >
> > #endif /* IOTHREAD_H */
> > diff --git a/iothread.c b/iothread.c
> > index 74e400c..894756b 100644
> > --- a/iothread.c
> > +++ b/iothread.c
> > @@ -80,13 +80,10 @@ static void *iothread_run(void *opaque)
> > return NULL;
> > }
> >
> > -static int iothread_stop(Object *object, void *opaque)
> > +void iothread_stop(IOThread *iothread)
> > {
> > - IOThread *iothread;
> > -
> > - iothread = (IOThread *)object_dynamic_cast(object, TYPE_IOTHREAD);
> > - if (!iothread || !iothread->ctx) {
> > - return 0;
> > + if (iothread->stopping) {
> > + return;
> > }
> > iothread->stopping = true;
> > aio_notify(iothread->ctx);
> > @@ -94,6 +91,17 @@ static int iothread_stop(Object *object, void *opaque)
> > g_main_loop_quit(iothread->main_loop);
> > }
> > qemu_thread_join(&iothread->thread);
> > +}
> > +
> > +static int iothread_stop_iter(Object *object, void *opaque)
> > +{
> > + IOThread *iothread;
> > +
> > + iothread = (IOThread *)object_dynamic_cast(object, TYPE_IOTHREAD);
> > + if (!iothread || !iothread->ctx) {
> > + return 0;
> > + }
>
> I think the check of iothread->ctx can be moved to iothread_stop() too.
Yes, will do.
--
Peter Xu
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] iothread: delay the context release to finalize
2017-09-22 13:09 ` Fam Zheng
@ 2017-09-25 5:23 ` Peter Xu
2017-09-25 5:30 ` Fam Zheng
0 siblings, 1 reply; 30+ messages in thread
From: Peter Xu @ 2017-09-25 5:23 UTC (permalink / raw
To: Fam Zheng
Cc: qemu-devel, Dr . David Alan Gilbert, Stefan Hajnoczi,
Paolo Bonzini
On Fri, Sep 22, 2017 at 09:09:22PM +0800, Fam Zheng wrote:
> On Fri, 09/22 16:56, Peter Xu wrote:
> > When gcontext is used with iothread, the context will be destroyed
> > during iothread_stop(). That's not good since sometimes we would like
> > to keep the resources until iothread is destroyed, but we may want to
> > stop the thread before that point.
>
> Would be nice if you can also mention the glib bug that "required" this in the
> commit message.
I can add it, but I am not sure it's very closely related (and I'm
afraid that may confuse more people). Say, even without that bug, I
would still think it not a good idea to free the context in the loop,
especially considering that we have the finalize function there. Thanks,
>
> Reviewed-by: Fam Zheng <famz@redhat.com>
>
> >
> > Delay the destruction of gcontext to iothread finalize. Then we can do:
> >
> > iothread_stop(thread);
> > some_cleanup_on_resources();
> > iothread_destroy(thread);
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > iothread.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/iothread.c b/iothread.c
> > index 894756b..b95369b 100644
> > --- a/iothread.c
> > +++ b/iothread.c
> > @@ -71,8 +71,6 @@ static void *iothread_run(void *opaque)
> > g_main_loop_unref(loop);
> >
> > g_main_context_pop_thread_default(iothread->worker_context);
> > - g_main_context_unref(iothread->worker_context);
> > - iothread->worker_context = NULL;
> > }
> > }
> >
> > @@ -117,6 +115,10 @@ static void iothread_instance_finalize(Object *obj)
> > IOThread *iothread = IOTHREAD(obj);
> >
> > iothread_stop(iothread);
> > + if (iothread->worker_context) {
> > + g_main_context_unref(iothread->worker_context);
> > + iothread->worker_context = NULL;
> > + }
> > qemu_cond_destroy(&iothread->init_done_cond);
> > qemu_mutex_destroy(&iothread->init_done_lock);
> > if (!iothread->ctx) {
> > --
> > 2.7.4
> >
> >
--
Peter Xu
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] iothread: delay the context release to finalize
2017-09-25 5:23 ` Peter Xu
@ 2017-09-25 5:30 ` Fam Zheng
2017-09-25 5:50 ` Peter Xu
0 siblings, 1 reply; 30+ messages in thread
From: Fam Zheng @ 2017-09-25 5:30 UTC (permalink / raw
To: Peter Xu
Cc: Paolo Bonzini, Stefan Hajnoczi, qemu-devel,
Dr . David Alan Gilbert
On Mon, 09/25 13:23, Peter Xu wrote:
> On Fri, Sep 22, 2017 at 09:09:22PM +0800, Fam Zheng wrote:
> > On Fri, 09/22 16:56, Peter Xu wrote:
> > > When gcontext is used with iothread, the context will be destroyed
> > > during iothread_stop(). That's not good since sometimes we would like
> > > to keep the resources until iothread is destroyed, but we may want to
> > > stop the thread before that point.
> >
> > Would be nice if you can also mention the glib bug that "required" this in the
> > commit message.
>
> I can add it, but I am not sure it's very closely related (and I'm
> afraid that may confuse more people). Say, even without that bug, I
> would still think it not a good idea to free the context in the loop,
> especially considering that we have the finalize function there. Thanks,
It's interesting to know if or not your future change will break without this
patch, this is especially useful for backport.
Fam
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] iothread: delay the context release to finalize
2017-09-25 5:30 ` Fam Zheng
@ 2017-09-25 5:50 ` Peter Xu
2017-09-25 5:58 ` Fam Zheng
0 siblings, 1 reply; 30+ messages in thread
From: Peter Xu @ 2017-09-25 5:50 UTC (permalink / raw
To: Fam Zheng
Cc: Paolo Bonzini, Stefan Hajnoczi, qemu-devel,
Dr . David Alan Gilbert
On Mon, Sep 25, 2017 at 01:30:02PM +0800, Fam Zheng wrote:
> On Mon, 09/25 13:23, Peter Xu wrote:
> > On Fri, Sep 22, 2017 at 09:09:22PM +0800, Fam Zheng wrote:
> > > On Fri, 09/22 16:56, Peter Xu wrote:
> > > > When gcontext is used with iothread, the context will be destroyed
> > > > during iothread_stop(). That's not good since sometimes we would like
> > > > to keep the resources until iothread is destroyed, but we may want to
> > > > stop the thread before that point.
> > >
> > > Would be nice if you can also mention the glib bug that "required" this in the
> > > commit message.
> >
> > I can add it, but I am not sure it's very closely related (and I'm
> > afraid that may confuse more people). Say, even without that bug, I
> > would still think it not a good idea to free the context in the loop,
> > especially considering that we have the finalize function there. Thanks,
>
> It's interesting to know if or not your future change will break without this
> patch, this is especially useful for backport.
I haven't tried to run with iothread and without this patch, but I
think it should fail, so this patch should be needed.
The point is that we should not destroy the context before explicitly
calling remove_fd_in_watch() if the context is running chardevs.
Without this patch, this rule does not satisfy. And IIUC this rule
comes from the glib bug.
Anyway, I'll mention it in commit message to clarify.
--
Peter Xu
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] iothread: delay the context release to finalize
2017-09-25 5:50 ` Peter Xu
@ 2017-09-25 5:58 ` Fam Zheng
0 siblings, 0 replies; 30+ messages in thread
From: Fam Zheng @ 2017-09-25 5:58 UTC (permalink / raw
To: Peter Xu
Cc: Paolo Bonzini, Stefan Hajnoczi, qemu-devel,
Dr . David Alan Gilbert
On Mon, 09/25 13:50, Peter Xu wrote:
> On Mon, Sep 25, 2017 at 01:30:02PM +0800, Fam Zheng wrote:
> > On Mon, 09/25 13:23, Peter Xu wrote:
> > > On Fri, Sep 22, 2017 at 09:09:22PM +0800, Fam Zheng wrote:
> > > > On Fri, 09/22 16:56, Peter Xu wrote:
> > > > > When gcontext is used with iothread, the context will be destroyed
> > > > > during iothread_stop(). That's not good since sometimes we would like
> > > > > to keep the resources until iothread is destroyed, but we may want to
> > > > > stop the thread before that point.
> > > >
> > > > Would be nice if you can also mention the glib bug that "required" this in the
> > > > commit message.
> > >
> > > I can add it, but I am not sure it's very closely related (and I'm
> > > afraid that may confuse more people). Say, even without that bug, I
> > > would still think it not a good idea to free the context in the loop,
> > > especially considering that we have the finalize function there. Thanks,
> >
> > It's interesting to know if or not your future change will break without this
> > patch, this is especially useful for backport.
>
> I haven't tried to run with iothread and without this patch, but I
> think it should fail, so this patch should be needed.
>
> The point is that we should not destroy the context before explicitly
> calling remove_fd_in_watch() if the context is running chardevs.
> Without this patch, this rule does not satisfy. And IIUC this rule
> comes from the glib bug.
>
> Anyway, I'll mention it in commit message to clarify.
OK, thanks for the explanations! My r-b still stands with the amended commit
log.
Fam
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-09-22 10:16 ` Stefan Hajnoczi
2017-09-22 10:18 ` Paolo Bonzini
@ 2017-10-02 17:18 ` Markus Armbruster
2017-10-08 12:46 ` Peter Xu
1 sibling, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2017-10-02 17:18 UTC (permalink / raw
To: Stefan Hajnoczi
Cc: Fam Zheng, Peter Xu, qemu-devel, Stefan Hajnoczi, Paolo Bonzini,
Dr . David Alan Gilbert
Stefan Hajnoczi <stefanha@redhat.com> writes:
> On Fri, Sep 22, 2017 at 04:56:10PM +0800, Peter Xu wrote:
>> IOThread is a general framework that contains IO loop environment and a
>> real thread behind. It's also good to be used internally inside qemu.
>> Provide some helpers for it to create iothreads to be used internally.
>>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>> include/sysemu/iothread.h | 8 ++++++++
>> iothread.c | 21 +++++++++++++++++++++
>> 2 files changed, 29 insertions(+)
>>
>> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
>> index d2985b3..b07663f 100644
>> --- a/include/sysemu/iothread.h
>> +++ b/include/sysemu/iothread.h
>> @@ -46,4 +46,12 @@ AioContext *iothread_get_aio_context(IOThread *iothread);
>> void iothread_stop_all(void);
>> GMainContext *iothread_get_g_main_context(IOThread *iothread);
>>
>> +/*
>> + * Helpers used to allocate iothreads for internal use. These
>> + * iothreads will not be seen by monitor clients when query using
>> + * "query-iothreads".
>> + */
>> +IOThread *iothread_create(const char *id, Error **errp);
>> +void iothread_destroy(IOThread *iothread);
>> +
>> #endif /* IOTHREAD_H */
>> diff --git a/iothread.c b/iothread.c
>> index 44c8944..74e400c 100644
>> --- a/iothread.c
>> +++ b/iothread.c
>> @@ -354,3 +354,24 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread)
>>
>> return iothread->worker_context;
>> }
>> +
>> +static Object *iothread_get_internal_parent(void)
>> +{
>> + return container_get(object_get_root(), "/internal-iothreads");
>> +}
>
> Markus, please advise on the following QMP API design issue.
Sorry for the slow response. Is my advice still needed, or are you
good?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
2017-10-02 17:18 ` Markus Armbruster
@ 2017-10-08 12:46 ` Peter Xu
0 siblings, 0 replies; 30+ messages in thread
From: Peter Xu @ 2017-10-08 12:46 UTC (permalink / raw
To: Markus Armbruster
Cc: Stefan Hajnoczi, Fam Zheng, qemu-devel, Stefan Hajnoczi,
Paolo Bonzini, Dr . David Alan Gilbert
On Mon, Oct 02, 2017 at 07:18:04PM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
> > On Fri, Sep 22, 2017 at 04:56:10PM +0800, Peter Xu wrote:
> >> IOThread is a general framework that contains IO loop environment and a
> >> real thread behind. It's also good to be used internally inside qemu.
> >> Provide some helpers for it to create iothreads to be used internally.
> >>
> >> Signed-off-by: Peter Xu <peterx@redhat.com>
> >> ---
> >> include/sysemu/iothread.h | 8 ++++++++
> >> iothread.c | 21 +++++++++++++++++++++
> >> 2 files changed, 29 insertions(+)
> >>
> >> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
> >> index d2985b3..b07663f 100644
> >> --- a/include/sysemu/iothread.h
> >> +++ b/include/sysemu/iothread.h
> >> @@ -46,4 +46,12 @@ AioContext *iothread_get_aio_context(IOThread *iothread);
> >> void iothread_stop_all(void);
> >> GMainContext *iothread_get_g_main_context(IOThread *iothread);
> >>
> >> +/*
> >> + * Helpers used to allocate iothreads for internal use. These
> >> + * iothreads will not be seen by monitor clients when query using
> >> + * "query-iothreads".
> >> + */
> >> +IOThread *iothread_create(const char *id, Error **errp);
> >> +void iothread_destroy(IOThread *iothread);
> >> +
> >> #endif /* IOTHREAD_H */
> >> diff --git a/iothread.c b/iothread.c
> >> index 44c8944..74e400c 100644
> >> --- a/iothread.c
> >> +++ b/iothread.c
> >> @@ -354,3 +354,24 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread)
> >>
> >> return iothread->worker_context;
> >> }
> >> +
> >> +static Object *iothread_get_internal_parent(void)
> >> +{
> >> + return container_get(object_get_root(), "/internal-iothreads");
> >> +}
> >
> > Markus, please advise on the following QMP API design issue.
>
> Sorry for the slow response. Is my advice still needed, or are you
> good?
I see that the patches are in master now (latest version, not this
one), and the consensus should be that internally used iothreads won't
affect results of query-iothreads. So I think we are good now. Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2017-10-08 12:46 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-22 8:56 [Qemu-devel] [PATCH 0/3] iothread: allow to create internal iothreads Peter Xu
2017-09-22 8:56 ` [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use Peter Xu
2017-09-22 9:04 ` Daniel P. Berrange
2017-09-22 9:14 ` Peter Xu
2017-09-22 9:36 ` Daniel P. Berrange
2017-09-22 9:38 ` Paolo Bonzini
2017-09-22 9:43 ` Daniel P. Berrange
2017-09-22 10:14 ` Paolo Bonzini
2017-09-22 10:17 ` Daniel P. Berrange
2017-09-22 12:59 ` Fam Zheng
2017-09-22 13:03 ` Paolo Bonzini
2017-09-22 10:54 ` Peter Xu
2017-09-22 10:16 ` Stefan Hajnoczi
2017-09-22 10:18 ` Paolo Bonzini
2017-09-22 10:20 ` Daniel P. Berrange
2017-09-22 10:26 ` Paolo Bonzini
2017-09-22 10:28 ` Daniel P. Berrange
2017-09-22 14:35 ` Stefan Hajnoczi
2017-09-22 14:55 ` Daniel P. Berrange
2017-10-02 17:18 ` Markus Armbruster
2017-10-08 12:46 ` Peter Xu
2017-09-22 8:56 ` [Qemu-devel] [PATCH 2/3] iothread: export iothread_stop() Peter Xu
2017-09-22 13:06 ` Fam Zheng
2017-09-25 3:29 ` Peter Xu
2017-09-22 8:56 ` [Qemu-devel] [PATCH 3/3] iothread: delay the context release to finalize Peter Xu
2017-09-22 13:09 ` Fam Zheng
2017-09-25 5:23 ` Peter Xu
2017-09-25 5:30 ` Fam Zheng
2017-09-25 5:50 ` Peter Xu
2017-09-25 5:58 ` Fam Zheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.