* [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()
@ 2020-07-16 15:41 Peter Maydell
2020-07-28 15:02 ` Peter Maydell
2020-07-28 15:16 ` Eric Blake
0 siblings, 2 replies; 13+ messages in thread
From: Peter Maydell @ 2020-07-16 15:41 UTC (permalink / raw)
To: qemu-devel, qemu-trivial; +Cc: Daniel P. Berrangé
Add a documentation comment for qemu_get_thread_id(): since this
is rather host-OS-specific it's useful if people writing the
implementation and people thinking of using the function know
what the purpose and limitations are.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Based on conversation with Dan on IRC, and prompted by the recent
patch to add OpenBSD support.
Q: should we document exactly what the thread-id value is for
each host platform in the QMP documentation ? Somebody writing
a management layer app should ideally not have to grovel through
the application to figure out what they should do with the
integer value they get back from query-cpus...
include/qemu/osdep.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 4841b5c6b5f..8279f72e5ed 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -515,6 +515,20 @@ bool qemu_has_ofd_lock(void);
bool qemu_write_pidfile(const char *pidfile, Error **errp);
+/**
+ * qemu_get_thread_id: Return OS-specific ID of current thread
+ *
+ * This function returns an OS-specific identifier of the
+ * current thread. This will be used for the "thread-id" field in
+ * the response to the QMP query-cpus and query-iothreads commands.
+ * The intention is that a VM management layer application can then
+ * use it to tie specific QEMU vCPU and IO threads to specific host
+ * CPUs using whatever the host OS's CPU affinity setting API is.
+ * New implementations of this function for new host OSes should
+ * return the most sensible integer ID that works for that purpose.
+ *
+ * This function should not be used for anything else inside QEMU.
+ */
int qemu_get_thread_id(void);
#ifndef CONFIG_IOVEC
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()
2020-07-16 15:41 [PATCH] osdep.h: Add doc comment for qemu_get_thread_id() Peter Maydell
@ 2020-07-28 15:02 ` Peter Maydell
2020-07-28 15:16 ` Eric Blake
1 sibling, 0 replies; 13+ messages in thread
From: Peter Maydell @ 2020-07-28 15:02 UTC (permalink / raw)
To: QEMU Developers, QEMU Trivial; +Cc: Daniel P. Berrangé, Markus Armbruster
Ping? I've cc'd a few new people who might have a view on the
"should the QMP documentation define what a thread-id is?"
question below the --- fold...
On Thu, 16 Jul 2020 at 16:41, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Add a documentation comment for qemu_get_thread_id(): since this
> is rather host-OS-specific it's useful if people writing the
> implementation and people thinking of using the function know
> what the purpose and limitations are.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Based on conversation with Dan on IRC, and prompted by the recent
> patch to add OpenBSD support.
>
> Q: should we document exactly what the thread-id value is for
> each host platform in the QMP documentation ? Somebody writing
> a management layer app should ideally not have to grovel through
> the application to figure out what they should do with the
> integer value they get back from query-cpus...
>
> include/qemu/osdep.h | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 4841b5c6b5f..8279f72e5ed 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -515,6 +515,20 @@ bool qemu_has_ofd_lock(void);
>
> bool qemu_write_pidfile(const char *pidfile, Error **errp);
>
> +/**
> + * qemu_get_thread_id: Return OS-specific ID of current thread
> + *
> + * This function returns an OS-specific identifier of the
> + * current thread. This will be used for the "thread-id" field in
> + * the response to the QMP query-cpus and query-iothreads commands.
> + * The intention is that a VM management layer application can then
> + * use it to tie specific QEMU vCPU and IO threads to specific host
> + * CPUs using whatever the host OS's CPU affinity setting API is.
> + * New implementations of this function for new host OSes should
> + * return the most sensible integer ID that works for that purpose.
> + *
> + * This function should not be used for anything else inside QEMU.
> + */
> int qemu_get_thread_id(void);
>
> #ifndef CONFIG_IOVEC
> --
> 2.20.1
thanks
-- PMM
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()
2020-07-16 15:41 [PATCH] osdep.h: Add doc comment for qemu_get_thread_id() Peter Maydell
2020-07-28 15:02 ` Peter Maydell
@ 2020-07-28 15:16 ` Eric Blake
2020-07-28 15:25 ` Peter Maydell
1 sibling, 1 reply; 13+ messages in thread
From: Eric Blake @ 2020-07-28 15:16 UTC (permalink / raw)
To: Peter Maydell, qemu-devel, qemu-trivial; +Cc: Daniel P. Berrangé
On 7/16/20 10:41 AM, Peter Maydell wrote:
> Add a documentation comment for qemu_get_thread_id(): since this
> is rather host-OS-specific it's useful if people writing the
> implementation and people thinking of using the function know
> what the purpose and limitations are.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Based on conversation with Dan on IRC, and prompted by the recent
> patch to add OpenBSD support.
>
> Q: should we document exactly what the thread-id value is for
> each host platform in the QMP documentation ? Somebody writing
> a management layer app should ideally not have to grovel through
> the application to figure out what they should do with the
> integer value they get back from query-cpus...
>
> include/qemu/osdep.h | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
Do we need a counterpart change...
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 4841b5c6b5f..8279f72e5ed 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -515,6 +515,20 @@ bool qemu_has_ofd_lock(void);
>
> bool qemu_write_pidfile(const char *pidfile, Error **errp);
>
> +/**
> + * qemu_get_thread_id: Return OS-specific ID of current thread
> + *
> + * This function returns an OS-specific identifier of the
> + * current thread. This will be used for the "thread-id" field in
> + * the response to the QMP query-cpus and query-iothreads commands.
...to the qapi definition of query-cpus and query-iothreads?
> + * The intention is that a VM management layer application can then
> + * use it to tie specific QEMU vCPU and IO threads to specific host
> + * CPUs using whatever the host OS's CPU affinity setting API is.
> + * New implementations of this function for new host OSes should
> + * return the most sensible integer ID that works for that purpose.
> + *
> + * This function should not be used for anything else inside QEMU.
> + */
> int qemu_get_thread_id(void);
Otherwise this change looks sensible to me.
Reviewed-by: Eric Blake <eblake@redhat.com>
>
> #ifndef CONFIG_IOVEC
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()
2020-07-28 15:16 ` Eric Blake
@ 2020-07-28 15:25 ` Peter Maydell
2020-07-30 15:10 ` Markus Armbruster
0 siblings, 1 reply; 13+ messages in thread
From: Peter Maydell @ 2020-07-28 15:25 UTC (permalink / raw)
To: Eric Blake; +Cc: QEMU Trivial, Daniel P. Berrangé, QEMU Developers
On Tue, 28 Jul 2020 at 16:17, Eric Blake <eblake@redhat.com> wrote:
>
> On 7/16/20 10:41 AM, Peter Maydell wrote:
> > Add a documentation comment for qemu_get_thread_id(): since this
> > is rather host-OS-specific it's useful if people writing the
> > implementation and people thinking of using the function know
> > what the purpose and limitations are.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > Based on conversation with Dan on IRC, and prompted by the recent
> > patch to add OpenBSD support.
> >
> > Q: should we document exactly what the thread-id value is for
> > each host platform in the QMP documentation ? Somebody writing
> > a management layer app should ideally not have to grovel through
> > the application to figure out what they should do with the
> > integer value they get back from query-cpus...
> >
> > include/qemu/osdep.h | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
>
> Do we need a counterpart change...
>
> >
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index 4841b5c6b5f..8279f72e5ed 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -515,6 +515,20 @@ bool qemu_has_ofd_lock(void);
> >
> > bool qemu_write_pidfile(const char *pidfile, Error **errp);
> >
> > +/**
> > + * qemu_get_thread_id: Return OS-specific ID of current thread
> > + *
> > + * This function returns an OS-specific identifier of the
> > + * current thread. This will be used for the "thread-id" field in
> > + * the response to the QMP query-cpus and query-iothreads commands.
>
> ...to the qapi definition of query-cpus and query-iothreads?
Well, that was my question above. Currently the QAPI documentation
says absolutely nothing about what the thread-id values mean
for any host OS (beyond "ID of the underlying host thread"), which
means that any management layer application needs to look in the
implementation to find out what they actually are...
Improving the QAPI docs would probably be something like:
* add a list of host OSes and semantics to the doc comment
for CpuInfoFast
* add cross-references to that definition from everywhere
else in QAPI that uses a thread-id/thread_id
* add a comment in the C file to say "if you're adding another
OS ifdef here please update the QAPI doc comment"
thanks
-- PMM
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()
2020-07-28 15:25 ` Peter Maydell
@ 2020-07-30 15:10 ` Markus Armbruster
2020-07-30 15:52 ` Peter Maydell
0 siblings, 1 reply; 13+ messages in thread
From: Markus Armbruster @ 2020-07-30 15:10 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Trivial, Daniel P. Berrangé, QEMU Developers
Peter Maydell <peter.maydell@linaro.org> writes:
> On Tue, 28 Jul 2020 at 16:17, Eric Blake <eblake@redhat.com> wrote:
>>
>> On 7/16/20 10:41 AM, Peter Maydell wrote:
>> > Add a documentation comment for qemu_get_thread_id(): since this
>> > is rather host-OS-specific it's useful if people writing the
>> > implementation and people thinking of using the function know
>> > what the purpose and limitations are.
>> >
>> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> > ---
>> > Based on conversation with Dan on IRC, and prompted by the recent
>> > patch to add OpenBSD support.
>> >
>> > Q: should we document exactly what the thread-id value is for
>> > each host platform in the QMP documentation ? Somebody writing
>> > a management layer app should ideally not have to grovel through
>> > the application to figure out what they should do with the
>> > integer value they get back from query-cpus...
>> >
>> > include/qemu/osdep.h | 14 ++++++++++++++
>> > 1 file changed, 14 insertions(+)
>>
>> Do we need a counterpart change...
>>
>> >
>> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> > index 4841b5c6b5f..8279f72e5ed 100644
>> > --- a/include/qemu/osdep.h
>> > +++ b/include/qemu/osdep.h
>> > @@ -515,6 +515,20 @@ bool qemu_has_ofd_lock(void);
>> >
>> > bool qemu_write_pidfile(const char *pidfile, Error **errp);
>> >
>> > +/**
>> > + * qemu_get_thread_id: Return OS-specific ID of current thread
>> > + *
>> > + * This function returns an OS-specific identifier of the
>> > + * current thread. This will be used for the "thread-id" field in
>> > + * the response to the QMP query-cpus and query-iothreads commands.
>>
>> ...to the qapi definition of query-cpus and query-iothreads?
>
> Well, that was my question above. Currently the QAPI documentation
> says absolutely nothing about what the thread-id values mean
> for any host OS (beyond "ID of the underlying host thread"), which
> means that any management layer application needs to look in the
> implementation to find out what they actually are...
... which they will have to do to actually use it for the purpose we
have in mind, namely:
>> > + * The intention is that a VM management layer application can then
>> > + * use it to tie specific QEMU vCPU and IO threads to specific host
>> > + * CPUs using whatever the host OS's CPU affinity setting API is.
>> > + * New implementations of this function for new host OSes should
>> > + * return the most sensible integer ID that works for that purpose.
>> > + *
>> > + * This function should not be used for anything else inside QEMU.
>> > + */
Do they?
> Improving the QAPI docs would probably be something like:
> * add a list of host OSes and semantics to the doc comment
> for CpuInfoFast
> * add cross-references to that definition from everywhere
> else in QAPI that uses a thread-id/thread_id
> * add a comment in the C file to say "if you're adding another
> OS ifdef here please update the QAPI doc comment"
If they do, then this sounds like a plan.
By the way, the #else case smells:
#else
return getpid();
#endif
The PID is quite unlikely to be "an OS-specific identifier of the
current thread". Shouldn't we fail instead of lie when we don't know
how to compute the truth?
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()
2020-07-30 15:10 ` Markus Armbruster
@ 2020-07-30 15:52 ` Peter Maydell
2020-07-30 15:59 ` Daniel P. Berrangé
0 siblings, 1 reply; 13+ messages in thread
From: Peter Maydell @ 2020-07-30 15:52 UTC (permalink / raw)
To: Markus Armbruster; +Cc: QEMU Trivial, Daniel P. Berrangé, QEMU Developers
On Thu, 30 Jul 2020 at 16:11, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Tue, 28 Jul 2020 at 16:17, Eric Blake <eblake@redhat.com> wrote:
> >>
> >> On 7/16/20 10:41 AM, Peter Maydell wrote:
> >> > Add a documentation comment for qemu_get_thread_id(): since this
> >> > is rather host-OS-specific it's useful if people writing the
> >> > implementation and people thinking of using the function know
> >> > what the purpose and limitations are.
> >> >
> >> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >> > ---
> >> > Based on conversation with Dan on IRC, and prompted by the recent
> >> > patch to add OpenBSD support.
> >> >
> >> > Q: should we document exactly what the thread-id value is for
> >> > each host platform in the QMP documentation ? Somebody writing
> >> > a management layer app should ideally not have to grovel through
> >> > the application to figure out what they should do with the
> >> > integer value they get back from query-cpus...
> >> >
> >> > include/qemu/osdep.h | 14 ++++++++++++++
> >> > 1 file changed, 14 insertions(+)
> >>
> >> Do we need a counterpart change...
> >>
> >> >
> >> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> >> > index 4841b5c6b5f..8279f72e5ed 100644
> >> > --- a/include/qemu/osdep.h
> >> > +++ b/include/qemu/osdep.h
> >> > @@ -515,6 +515,20 @@ bool qemu_has_ofd_lock(void);
> >> >
> >> > bool qemu_write_pidfile(const char *pidfile, Error **errp);
> >> >
> >> > +/**
> >> > + * qemu_get_thread_id: Return OS-specific ID of current thread
> >> > + *
> >> > + * This function returns an OS-specific identifier of the
> >> > + * current thread. This will be used for the "thread-id" field in
> >> > + * the response to the QMP query-cpus and query-iothreads commands.
> >>
> >> ...to the qapi definition of query-cpus and query-iothreads?
> >
> > Well, that was my question above. Currently the QAPI documentation
> > says absolutely nothing about what the thread-id values mean
> > for any host OS (beyond "ID of the underlying host thread"), which
> > means that any management layer application needs to look in the
> > implementation to find out what they actually are...
>
> ... which they will have to do to actually use it for the purpose we
> have in mind, namely:
>
> >> > + * The intention is that a VM management layer application can then
> >> > + * use it to tie specific QEMU vCPU and IO threads to specific host
> >> > + * CPUs using whatever the host OS's CPU affinity setting API is.
> >> > + * New implementations of this function for new host OSes should
> >> > + * return the most sensible integer ID that works for that purpose.
> >> > + *
> >> > + * This function should not be used for anything else inside QEMU.
> >> > + */
>
> Do they?
Well, I suspect that management-layer code currently has
gone for "assume we're always running on Linux" and was
written by people who knew they were getting a Linux tid...
> The PID is quite unlikely to be "an OS-specific identifier of the
> current thread". Shouldn't we fail instead of lie when we don't know
> how to compute the truth?
Yeah, I think the default codepath is pretty bogus too. Should
the QMP functions have a mechanism for saying "we don't know
a thread-id on this platform" ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()
2020-07-30 15:52 ` Peter Maydell
@ 2020-07-30 15:59 ` Daniel P. Berrangé
2020-07-30 16:24 ` Eric Blake
0 siblings, 1 reply; 13+ messages in thread
From: Daniel P. Berrangé @ 2020-07-30 15:59 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Trivial, Markus Armbruster, QEMU Developers
On Thu, Jul 30, 2020 at 04:52:49PM +0100, Peter Maydell wrote:
> On Thu, 30 Jul 2020 at 16:11, Markus Armbruster <armbru@redhat.com> wrote:
> >
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >
> > > On Tue, 28 Jul 2020 at 16:17, Eric Blake <eblake@redhat.com> wrote:
> > >>
> > >> On 7/16/20 10:41 AM, Peter Maydell wrote:
> > >> > Add a documentation comment for qemu_get_thread_id(): since this
> > >> > is rather host-OS-specific it's useful if people writing the
> > >> > implementation and people thinking of using the function know
> > >> > what the purpose and limitations are.
> > >> >
> > >> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > >> > ---
> > >> > Based on conversation with Dan on IRC, and prompted by the recent
> > >> > patch to add OpenBSD support.
> > >> >
> > >> > Q: should we document exactly what the thread-id value is for
> > >> > each host platform in the QMP documentation ? Somebody writing
> > >> > a management layer app should ideally not have to grovel through
> > >> > the application to figure out what they should do with the
> > >> > integer value they get back from query-cpus...
> > >> >
> > >> > include/qemu/osdep.h | 14 ++++++++++++++
> > >> > 1 file changed, 14 insertions(+)
> > >>
> > >> Do we need a counterpart change...
> > >>
> > >> >
> > >> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > >> > index 4841b5c6b5f..8279f72e5ed 100644
> > >> > --- a/include/qemu/osdep.h
> > >> > +++ b/include/qemu/osdep.h
> > >> > @@ -515,6 +515,20 @@ bool qemu_has_ofd_lock(void);
> > >> >
> > >> > bool qemu_write_pidfile(const char *pidfile, Error **errp);
> > >> >
> > >> > +/**
> > >> > + * qemu_get_thread_id: Return OS-specific ID of current thread
> > >> > + *
> > >> > + * This function returns an OS-specific identifier of the
> > >> > + * current thread. This will be used for the "thread-id" field in
> > >> > + * the response to the QMP query-cpus and query-iothreads commands.
> > >>
> > >> ...to the qapi definition of query-cpus and query-iothreads?
> > >
> > > Well, that was my question above. Currently the QAPI documentation
> > > says absolutely nothing about what the thread-id values mean
> > > for any host OS (beyond "ID of the underlying host thread"), which
> > > means that any management layer application needs to look in the
> > > implementation to find out what they actually are...
> >
> > ... which they will have to do to actually use it for the purpose we
> > have in mind, namely:
> >
> > >> > + * The intention is that a VM management layer application can then
> > >> > + * use it to tie specific QEMU vCPU and IO threads to specific host
> > >> > + * CPUs using whatever the host OS's CPU affinity setting API is.
> > >> > + * New implementations of this function for new host OSes should
> > >> > + * return the most sensible integer ID that works for that purpose.
> > >> > + *
> > >> > + * This function should not be used for anything else inside QEMU.
> > >> > + */
> >
> > Do they?
>
> Well, I suspect that management-layer code currently has
> gone for "assume we're always running on Linux" and was
> written by people who knew they were getting a Linux tid...
Yes, on the libvirt side, the functionality that relies on thread_is is
only compiled on Linux. If someone wants to use it on other OS, they'll
have to provide an impl using their platforms equivalent of
sched_setaffinity and friends since none of this stuff is standardized
across OS.
> > The PID is quite unlikely to be "an OS-specific identifier of the
> > current thread". Shouldn't we fail instead of lie when we don't know
> > how to compute the truth?
>
> Yeah, I think the default codepath is pretty bogus too. Should
> the QMP functions have a mechanism for saying "we don't know
> a thread-id on this platform" ?
Thread_id should be optional and thus not filled in if we
can't provide a sensible value. Unfortunately we made it
mandatory in QMP.
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] 13+ messages in thread
* Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()
2020-07-30 15:59 ` Daniel P. Berrangé
@ 2020-07-30 16:24 ` Eric Blake
2020-07-30 16:40 ` Daniel P. Berrangé
2020-07-31 7:44 ` Markus Armbruster
0 siblings, 2 replies; 13+ messages in thread
From: Eric Blake @ 2020-07-30 16:24 UTC (permalink / raw)
To: Daniel P. Berrangé, Peter Maydell
Cc: QEMU Trivial, Markus Armbruster, QEMU Developers
On 7/30/20 10:59 AM, Daniel P. Berrangé wrote:
>> Well, I suspect that management-layer code currently has
>> gone for "assume we're always running on Linux" and was
>> written by people who knew they were getting a Linux tid...
>
> Yes, on the libvirt side, the functionality that relies on thread_is is
> only compiled on Linux. If someone wants to use it on other OS, they'll
> have to provide an impl using their platforms equivalent of
> sched_setaffinity and friends since none of this stuff is standardized
> across OS.
>
>
>>> The PID is quite unlikely to be "an OS-specific identifier of the
>>> current thread". Shouldn't we fail instead of lie when we don't know
>>> how to compute the truth?
>>
>> Yeah, I think the default codepath is pretty bogus too. Should
>> the QMP functions have a mechanism for saying "we don't know
>> a thread-id on this platform" ?
>
> Thread_id should be optional and thus not filled in if we
> can't provide a sensible value. Unfortunately we made it
> mandatory in QMP.
Normally, converting a mandatory output value to optional is a
back-compatibility risk (we could break apps that depended on it being
present). But if the only apps that depended on it being present are
compiled on Linux, where the member will actually be present, I think
that changing the schema to make it optional for non-Linux platforms
won't be a back-compatibility nightmare (but we will have to be careful
in our documentation).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()
2020-07-30 16:24 ` Eric Blake
@ 2020-07-30 16:40 ` Daniel P. Berrangé
2020-07-31 7:44 ` Markus Armbruster
1 sibling, 0 replies; 13+ messages in thread
From: Daniel P. Berrangé @ 2020-07-30 16:40 UTC (permalink / raw)
To: Eric Blake
Cc: QEMU Trivial, Peter Maydell, Markus Armbruster, QEMU Developers
On Thu, Jul 30, 2020 at 11:24:51AM -0500, Eric Blake wrote:
> On 7/30/20 10:59 AM, Daniel P. Berrangé wrote:
>
> > > Well, I suspect that management-layer code currently has
> > > gone for "assume we're always running on Linux" and was
> > > written by people who knew they were getting a Linux tid...
> >
> > Yes, on the libvirt side, the functionality that relies on thread_is is
> > only compiled on Linux. If someone wants to use it on other OS, they'll
> > have to provide an impl using their platforms equivalent of
> > sched_setaffinity and friends since none of this stuff is standardized
> > across OS.
> >
> >
> > > > The PID is quite unlikely to be "an OS-specific identifier of the
> > > > current thread". Shouldn't we fail instead of lie when we don't know
> > > > how to compute the truth?
> > >
> > > Yeah, I think the default codepath is pretty bogus too. Should
> > > the QMP functions have a mechanism for saying "we don't know
> > > a thread-id on this platform" ?
> >
> > Thread_id should be optional and thus not filled in if we
> > can't provide a sensible value. Unfortunately we made it
> > mandatory in QMP.
>
> Normally, converting a mandatory output value to optional is a
> back-compatibility risk (we could break apps that depended on it being
> present). But if the only apps that depended on it being present are
> compiled on Linux, where the member will actually be present, I think that
> changing the schema to make it optional for non-Linux platforms won't be a
> back-compatibility nightmare (but we will have to be careful in our
> documentation).
FWIW, libvirt treats it as mandatory for query-iothreads, but optional
for query-cpus because it was missing in some older QEMU versions
entirely.
Libvirt explicitly only supports macOS, Linux and FreeBSD, so if those
platforms all report a value, libvirt won't care if you make it optional
and omit it for other platforms.
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] 13+ messages in thread
* Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()
2020-07-30 16:24 ` Eric Blake
2020-07-30 16:40 ` Daniel P. Berrangé
@ 2020-07-31 7:44 ` Markus Armbruster
2020-07-31 13:46 ` Eric Blake
1 sibling, 1 reply; 13+ messages in thread
From: Markus Armbruster @ 2020-07-31 7:44 UTC (permalink / raw)
To: Eric Blake
Cc: QEMU Trivial, Peter Maydell, Daniel P. Berrangé, QEMU Developers
Eric Blake <eblake@redhat.com> writes:
> On 7/30/20 10:59 AM, Daniel P. Berrangé wrote:
>
>>> Well, I suspect that management-layer code currently has
>>> gone for "assume we're always running on Linux" and was
>>> written by people who knew they were getting a Linux tid...
>>
>> Yes, on the libvirt side, the functionality that relies on thread_is is
>> only compiled on Linux. If someone wants to use it on other OS, they'll
>> have to provide an impl using their platforms equivalent of
>> sched_setaffinity and friends since none of this stuff is standardized
>> across OS.
>>
>>
>>>> The PID is quite unlikely to be "an OS-specific identifier of the
>>>> current thread". Shouldn't we fail instead of lie when we don't know
>>>> how to compute the truth?
>>>
>>> Yeah, I think the default codepath is pretty bogus too. Should
>>> the QMP functions have a mechanism for saying "we don't know
>>> a thread-id on this platform" ?
>>
>> Thread_id should be optional and thus not filled in if we
>> can't provide a sensible value. Unfortunately we made it
>> mandatory in QMP.
>
> Normally, converting a mandatory output value to optional is a
> back-compatibility risk (we could break apps that depended on it being
> present). But if the only apps that depended on it being present are
> compiled on Linux, where the member will actually be present, I think
> that changing the schema to make it optional for non-Linux platforms
> won't be a back-compatibility nightmare (but we will have to be
> careful in our documentation).
Options for systems where don't know how to compute a system-wide thread
ID:
0. Return a bogus value: the PID. This is the status quo.
1. Return a more obviously bogus value: -1. Semantic compatibility
break. Should be harmless, because a QMP client relying on the
thread-id being the PID would be insane.
2. Make thread-id optional, present iff we can compute a value.
This is what we should have done, but we didn't, and now it's a
syntactic compatibility break. Matters only if it actually breaks
QMP clients. We believe the one we know shouldn't break.
Preferences?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()
2020-07-31 7:44 ` Markus Armbruster
@ 2020-07-31 13:46 ` Eric Blake
2020-07-31 13:51 ` Daniel P. Berrangé
0 siblings, 1 reply; 13+ messages in thread
From: Eric Blake @ 2020-07-31 13:46 UTC (permalink / raw)
To: Markus Armbruster
Cc: QEMU Trivial, Peter Maydell, Daniel P. Berrangé, QEMU Developers
On 7/31/20 2:44 AM, Markus Armbruster wrote:
>>> Thread_id should be optional and thus not filled in if we
>>> can't provide a sensible value. Unfortunately we made it
>>> mandatory in QMP.
>>
>> Normally, converting a mandatory output value to optional is a
>> back-compatibility risk (we could break apps that depended on it being
>> present). But if the only apps that depended on it being present are
>> compiled on Linux, where the member will actually be present, I think
>> that changing the schema to make it optional for non-Linux platforms
>> won't be a back-compatibility nightmare (but we will have to be
>> careful in our documentation).
>
> Options for systems where don't know how to compute a system-wide thread
> ID:
>
> 0. Return a bogus value: the PID. This is the status quo.
>
> 1. Return a more obviously bogus value: -1. Semantic compatibility
> break. Should be harmless, because a QMP client relying on the
> thread-id being the PID would be insane.
>
> 2. Make thread-id optional, present iff we can compute a value.
>
> This is what we should have done, but we didn't, and now it's a
> syntactic compatibility break. Matters only if it actually breaks
> QMP clients. We believe the one we know shouldn't break.
>
> Preferences?
I'm in favor of 2, but can easily live with 1 if we decide to be that
much more conservative. Tooling that can't handle a missing value is
not going to fare any better with a value that is unusable because it is
-1, but the important point is that I don't think we have a scenario
with such tooling depending on the value (the tools that DO depend on
the value are built on platforms where the value is usable).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()
2020-07-31 13:46 ` Eric Blake
@ 2020-07-31 13:51 ` Daniel P. Berrangé
0 siblings, 0 replies; 13+ messages in thread
From: Daniel P. Berrangé @ 2020-07-31 13:51 UTC (permalink / raw)
To: Eric Blake
Cc: QEMU Trivial, Peter Maydell, Markus Armbruster, QEMU Developers
On Fri, Jul 31, 2020 at 08:46:40AM -0500, Eric Blake wrote:
> On 7/31/20 2:44 AM, Markus Armbruster wrote:
>
> > > > Thread_id should be optional and thus not filled in if we
> > > > can't provide a sensible value. Unfortunately we made it
> > > > mandatory in QMP.
> > >
> > > Normally, converting a mandatory output value to optional is a
> > > back-compatibility risk (we could break apps that depended on it being
> > > present). But if the only apps that depended on it being present are
> > > compiled on Linux, where the member will actually be present, I think
> > > that changing the schema to make it optional for non-Linux platforms
> > > won't be a back-compatibility nightmare (but we will have to be
> > > careful in our documentation).
> >
> > Options for systems where don't know how to compute a system-wide thread
> > ID:
> >
> > 0. Return a bogus value: the PID. This is the status quo.
> >
> > 1. Return a more obviously bogus value: -1. Semantic compatibility
> > break. Should be harmless, because a QMP client relying on the
> > thread-id being the PID would be insane.
> >
> > 2. Make thread-id optional, present iff we can compute a value.
> >
> > This is what we should have done, but we didn't, and now it's a
> > syntactic compatibility break. Matters only if it actually breaks
> > QMP clients. We believe the one we know shouldn't break.
> >
> > Preferences?
>
> I'm in favor of 2, but can easily live with 1 if we decide to be that much
> more conservative. Tooling that can't handle a missing value is not going
> to fare any better with a value that is unusable because it is -1, but the
> important point is that I don't think we have a scenario with such tooling
> depending on the value (the tools that DO depend on the value are built on
> platforms where the value is usable).
I'm fine with (2) too. While technically a backcompat break, it won't
hurt us in the real world, and so is the pragmatic choice that gets us
to a long term better solution.
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] 13+ messages in thread
* [PATCH 00/10] target/arm: Various v8.1M minor features
@ 2020-10-12 15:33 Peter Maydell
2020-10-12 15:33 ` [PATCH] osdep.h: Add doc comment for qemu_get_thread_id() Peter Maydell
0 siblings, 1 reply; 13+ messages in thread
From: Peter Maydell @ 2020-10-12 15:33 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson
This patchseries implements various minor v8.1M new features,
notably the branch-future and low-overhead-loop extensions.
(None of this will get enabled until we have enough to implement
a CPU model which has v8.1M, which will be the Cortex-M55, but
as usual we can get stuff into the tree gradually.)
Patch 1 is a decodetree fix suggested by Richard that is
necessary to avoid wrong-decode of the changes to t32.decode
by later patches.
thanks
-- PMM
Peter Maydell (10):
decodetree: Fix codegen for non-overlapping group inside overlapping
group
target/arm: Implement v8.1M NOCP handling
target/arm: Implement v8.1M conditional-select insns
target/arm: Make the t32 insn[25:23]=111 group non-overlapping
target/arm: Don't allow BLX imm for M-profile
target/arm: Implement v8.1M branch-future insns (as NOPs)
target/arm: Implement v8.1M low-overhead-loop instructions
target/arm: Fix has_vfp/has_neon ID reg squashing for M-profile
target/arm: Implement FPSCR.LTPSIZE for M-profile LOB extension
target/arm: Fix writing to FPSCR.FZ16 on M-profile
target/arm/cpu.h | 7 ++
target/arm/m-nocp.decode | 10 ++-
target/arm/t32.decode | 50 +++++++----
target/arm/cpu.c | 34 ++++---
target/arm/translate.c | 157 +++++++++++++++++++++++++++++++++
target/arm/vfp_helper.c | 30 +++++--
scripts/decodetree.py | 2 +-
target/arm/translate-vfp.c.inc | 17 +++-
8 files changed, 268 insertions(+), 39 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()
2020-10-12 15:33 [PATCH 00/10] target/arm: Various v8.1M minor features Peter Maydell
@ 2020-10-12 15:33 ` Peter Maydell
0 siblings, 0 replies; 13+ messages in thread
From: Peter Maydell @ 2020-10-12 15:33 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson
Add a documentation comment for qemu_get_thread_id(): since this
is rather host-OS-specific it's useful if people writing the
implementation and people thinking of using the function know
what the purpose and limitations are.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Based on conversation with Dan on IRC, and prompted by the recent
patch to add OpenBSD support.
Q: should we document exactly what the thread-id value is for
each host platform in the QMP documentation ? Somebody writing
a management layer app should ideally not have to grovel through
the application to figure out what they should do with the
integer value they get back from query-cpus...
include/qemu/osdep.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 4841b5c6b5f..8279f72e5ed 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -515,6 +515,20 @@ bool qemu_has_ofd_lock(void);
bool qemu_write_pidfile(const char *pidfile, Error **errp);
+/**
+ * qemu_get_thread_id: Return OS-specific ID of current thread
+ *
+ * This function returns an OS-specific identifier of the
+ * current thread. This will be used for the "thread-id" field in
+ * the response to the QMP query-cpus and query-iothreads commands.
+ * The intention is that a VM management layer application can then
+ * use it to tie specific QEMU vCPU and IO threads to specific host
+ * CPUs using whatever the host OS's CPU affinity setting API is.
+ * New implementations of this function for new host OSes should
+ * return the most sensible integer ID that works for that purpose.
+ *
+ * This function should not be used for anything else inside QEMU.
+ */
int qemu_get_thread_id(void);
#ifndef CONFIG_IOVEC
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-10-12 15:58 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-16 15:41 [PATCH] osdep.h: Add doc comment for qemu_get_thread_id() Peter Maydell
2020-07-28 15:02 ` Peter Maydell
2020-07-28 15:16 ` Eric Blake
2020-07-28 15:25 ` Peter Maydell
2020-07-30 15:10 ` Markus Armbruster
2020-07-30 15:52 ` Peter Maydell
2020-07-30 15:59 ` Daniel P. Berrangé
2020-07-30 16:24 ` Eric Blake
2020-07-30 16:40 ` Daniel P. Berrangé
2020-07-31 7:44 ` Markus Armbruster
2020-07-31 13:46 ` Eric Blake
2020-07-31 13:51 ` Daniel P. Berrangé
2020-10-12 15:33 [PATCH 00/10] target/arm: Various v8.1M minor features Peter Maydell
2020-10-12 15:33 ` [PATCH] osdep.h: Add doc comment for qemu_get_thread_id() Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).