QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [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; 12+ 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	[flat|nested] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ 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é

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git