[PATCH] osdep.h: Add doc comment for qemu_get_thread_id()

[PATCH] osdep.h: Add doc comment for qemu_get_thread_id()

[Peter Maydell]

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

Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()

[Peter Maydell]

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

Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()

[Eric Blake]

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

Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()

[Peter Maydell]

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

Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()

[Markus Armbruster]

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?

Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()

[Peter Maydell]

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

Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()

[Daniel P. Berrangé]

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 :|

Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()

[Eric Blake]

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

Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()

[Daniel P. Berrangé]

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 :|

Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()

[Markus Armbruster]

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?

Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()

[Eric Blake]

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

Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()

[Daniel P. Berrangé]

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 :|

[PATCH] osdep.h: Add doc comment for qemu_get_thread_id()

[Peter Maydell]

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