Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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 > --- > 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 :|
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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 > > --- > > 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
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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 > > > --- > > > 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 :|
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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 --- 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
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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 > --- > 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 :|
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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 >> --- >> 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
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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 > >> --- > >> 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 :|
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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 > --- > 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= 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 >
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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= 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 :|
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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= 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
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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= 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
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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= 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 :|
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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 > >> --- > >> 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
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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= 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
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
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= 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 :|
Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use
On Mon, Oct 02, 2017 at 07:18:04PM +0200, Markus Armbruster wrote: > Stefan Hajnoczi 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 > >> --- > >> 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