[v6,01/33] main-loop.h: introduce qemu_in_main_thread()

When invoked from the main loop, this function is the same
as qemu_mutex_iothread_locked, and returns true if the BQL is held.
When invoked from iothreads or tests, it returns true only
if the current AioContext is the Main Loop.

This essentially just extends qemu_mutex_iothread_locked to work
also in unit tests or other users like storage-daemon, that run
in the Main Loop but end up using the implementation in
stubs/iothread-lock.c.

Using qemu_mutex_iothread_locked in unit tests defaults to false
because they use the implementation in stubs/iothread-lock,
making all assertions added in next patches fail despite the
AioContext is still the main loop.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/qemu/main-loop.h | 13 +++++++++++++
 softmmu/cpus.c           |  5 +++++
 stubs/iothread-lock.c    |  5 +++++
 3 files changed, 23 insertions(+)

Am 21.01.2022 um 18:05 hat Emanuele Giuseppe Esposito geschrieben:
> When invoked from the main loop, this function is the same
> as qemu_mutex_iothread_locked, and returns true if the BQL is held.

So its name is misleading because it doesn't answer the question whether
we're in the main thread, but whethere we're holding the BQL (which is
mostly equivalent to holding an AioContext lock, not being in the home
thread of that AioContext).

> When invoked from iothreads or tests, it returns true only
> if the current AioContext is the Main Loop.
> 
> This essentially just extends qemu_mutex_iothread_locked to work
> also in unit tests or other users like storage-daemon, that run
> in the Main Loop but end up using the implementation in
> stubs/iothread-lock.c.
> 
> Using qemu_mutex_iothread_locked in unit tests defaults to false
> because they use the implementation in stubs/iothread-lock,
> making all assertions added in next patches fail despite the
> AioContext is still the main loop.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

This adds a new function that is almost the same as an existing
function, but the documentation is unclear when I should use one or the
other.

What are the use cases for the existing qemu_mutex_iothread_locked()
stub where we rely on false being returned?

If there aren't any, then maybe we should change the stub for that one
instead of adding a new function that behaves the same in the system
emulator and different only when it's stubbed out?

Kevin

On 27/01/2022 11:56, Kevin Wolf wrote:
> Am 21.01.2022 um 18:05 hat Emanuele Giuseppe Esposito geschrieben:
>> When invoked from the main loop, this function is the same
>> as qemu_mutex_iothread_locked, and returns true if the BQL is held.
> 
> So its name is misleading because it doesn't answer the question whether
> we're in the main thread, but whethere we're holding the BQL (which is
> mostly equivalent to holding an AioContext lock, not being in the home
> thread of that AioContext).
> 
>> When invoked from iothreads or tests, it returns true only
>> if the current AioContext is the Main Loop.
>>
>> This essentially just extends qemu_mutex_iothread_locked to work
>> also in unit tests or other users like storage-daemon, that run
>> in the Main Loop but end up using the implementation in
>> stubs/iothread-lock.c.
>>
>> Using qemu_mutex_iothread_locked in unit tests defaults to false
>> because they use the implementation in stubs/iothread-lock,
>> making all assertions added in next patches fail despite the
>> AioContext is still the main loop.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> This adds a new function that is almost the same as an existing
> function, but the documentation is unclear when I should use one or the
> other.

Why do you think it is unclear? as explained in commit and
documentation, unit tests for example use stubs/iothread-lock, which
returns always false. So we can (and should!) use this function in APIs
invoked by
unit tests, because qemu_mutex_iothread_locked will just return false,
making all tests crash. On the other side, I am pretty sure it won't
cause any failure when running QEMU or qemu-iotests, because there most
of the API use the cpu implementation.

> 
> What are the use cases for the existing qemu_mutex_iothread_locked()
> stub where we rely on false being returned?

I don't think we ever rely on stub being false. There are only 2 places
where I see
!qemu_mutex_iothread_locked(), and are in helper_regs.c and cpus.c

So being cpu related functions they use the cpu implementation.

However, commit 5f50be9b5810293141bb53cfd0cb46e765367d56 changed the
stub to return false for a specific reason.

> 
> If there aren't any, then maybe we should change the stub for that one
> instead of adding a new function that behaves the same in the system
> emulator and different only when it's stubbed out?

I don't think we can replace it, bcause stubs/qemu_in_main_thread()
calls qemu_get_current_aio_context, that in turn calls
qemu_mutex_iothread_locked. So we implicitly rely on that "false".

Thank you,
Emanuele

Am 31.01.2022 um 12:25 hat Emanuele Giuseppe Esposito geschrieben:
> 
> 
> On 27/01/2022 11:56, Kevin Wolf wrote:
> > Am 21.01.2022 um 18:05 hat Emanuele Giuseppe Esposito geschrieben:
> >> When invoked from the main loop, this function is the same
> >> as qemu_mutex_iothread_locked, and returns true if the BQL is held.
> > 
> > So its name is misleading because it doesn't answer the question whether
> > we're in the main thread, but whethere we're holding the BQL (which is
> > mostly equivalent to holding an AioContext lock, not being in the home
> > thread of that AioContext).
> > 
> >> When invoked from iothreads or tests, it returns true only
> >> if the current AioContext is the Main Loop.
> >>
> >> This essentially just extends qemu_mutex_iothread_locked to work
> >> also in unit tests or other users like storage-daemon, that run
> >> in the Main Loop but end up using the implementation in
> >> stubs/iothread-lock.c.
> >>
> >> Using qemu_mutex_iothread_locked in unit tests defaults to false
> >> because they use the implementation in stubs/iothread-lock,
> >> making all assertions added in next patches fail despite the
> >> AioContext is still the main loop.
> >>
> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > This adds a new function that is almost the same as an existing
> > function, but the documentation is unclear when I should use one or the
> > other.
> 
> Why do you think it is unclear? as explained in commit and
> documentation, unit tests for example use stubs/iothread-lock, which
> returns always false. So we can (and should!) use this function in
> APIs invoked by unit tests, because qemu_mutex_iothread_locked will
> just return false, making all tests crash. On the other side, I am
> pretty sure it won't cause any failure when running QEMU or
> qemu-iotests, because there most of the API use the cpu
> implementation.

I feel "use this function if your code is going to be used by unit
tests, but use qemu_mutex_iothread_locked() otherwise" is a very strange
interface. I'm relatively sure that qemu_mutex_iothread_locked() isn't
primarily used to make unit tests crash.

Documentation and the definition of the interface of a function
shouldn't be about the caller, but about the semantics of the function
itself. So, which question does qemu_mutex_iothread_locked() answer, and
which question does qemu_in_main_thread() answer?

The meaning of their result must be different, otherwise there wouldn't
be two different functions that must be used in different contexts.

> > What are the use cases for the existing qemu_mutex_iothread_locked()
> > stub where we rely on false being returned?
> 
> I don't think we ever rely on stub being false. There are only 2
> places where I see !qemu_mutex_iothread_locked(), and are in
> helper_regs.c and cpus.c
> 
> So being cpu related functions they use the cpu implementation.
> 
> However, commit 5f50be9b5810293141bb53cfd0cb46e765367d56 changed the
> stub to return false for a specific reason.

I must admit I don't really understand the reasoning there:

    With this change, the stub qemu_mutex_iothread_locked() must be changed
    from true to false.  The previous value of true was needed because the
    main thread did not have an AioContext in the thread-local variable,
    but now it does have one.

This explains why it _can_ be changed to false for this caller, but not
why it _must_ be changed.

> > If there aren't any, then maybe we should change the stub for that one
> > instead of adding a new function that behaves the same in the system
> > emulator and different only when it's stubbed out?
> 
> I don't think we can replace it, bcause stubs/qemu_in_main_thread()
> calls qemu_get_current_aio_context, that in turn calls
> qemu_mutex_iothread_locked. So we implicitly rely on that "false".

qemu_get_current_aio_context() will just return my_aiocontext, which is
qemu_aio_context because all tools call qemu_init_main_loop(). I don't
think it will ever call qemu_mutex_iothread_locked() in tools, except
for worker threads.

Ooh, and that's why we return false. Because the only kind of non-main
threads accessing global state in tools are worker threads which never
hold the BQL in the system emulator.

So is the problem with the unit tests really just that they never call
qemu_init_main_loop() and therefore never set the AioContext for the
main thread?

Kevin

On 1/31/22 15:33, Kevin Wolf wrote:
> I feel "use this function if your code is going to be used by unit
> tests, but use qemu_mutex_iothread_locked() otherwise" is a very strange
> interface. I'm relatively sure that qemu_mutex_iothread_locked() isn't
> primarily used to make unit tests crash.

qemu_mutex_iothread_locked() should never be used in the block layer, 
because programs that use the block layer might not call an iothread 
lock, and indeed only the emulator have an iothread lock.

Making it always true would be okay when those programs were 
single-threaded, but really they all had worker threads so it was 
changed to false by commit 5f50be9b58 ("async: the main AioContext is 
only "current" if under the BQL", 2021-06-18).

> Documentation and the definition of the interface of a function
> shouldn't be about the caller, but about the semantics of the function
> itself. So, which question does qemu_mutex_iothread_locked() answer, and
> which question does qemu_in_main_thread() answer?

qemu_mutex_iothread_locked() -> Does the program have exclusive access 
to the emulator's globals.

qemu_in_main_thread() -> Does the program have access to the block 
layer's globals.  In emulators this is governed by the iothread lock, 
elsewhere they are accessible only from the home thread of the initial 
AioContext.

So, in emulators it is the same question, but in the block layer one of 
them is actually meaningless.

Really the "bug" is that qemu_mutex_iothread_locked() should really not 
be used outside emulators.  There are just two uses, but it's hard to 
remove them.

So why are two functions needed?  Two reasons:

- stubs/iothread-lock.c cannot define qemu_mutex_iothread_locked() as 
"return qemu_get_current_aio_context() == qemu_get_aio_context();" 
because it would cause infinite recursion with 
qemu_get_current_aio_context()

- even if it could, frankly qemu_mutex_iothread_locked() is a horrible 
name, and in the context of the block layer it conflicts especially 
badly with the "iothread" concept.

Maybe we should embrace the BQL name and rename the functions that refer 
to the "iothread lock".  But then I don't want to have code in the block 
layer that refers to a "big lock".

>>> What are the use cases for the existing qemu_mutex_iothread_locked()
>>> stub where we rely on false being returned?
>>
>> I don't think we ever rely on stub being false. There are only 2
>> places where I see !qemu_mutex_iothread_locked(), and are in
>> helper_regs.c and cpus.c

We rely on it indirectly, here:

     AioContext *qemu_get_current_aio_context(void)
     {
         if (my_aiocontext) {
             return my_aiocontext;
         }
         if (qemu_mutex_iothread_locked()) {
             /* Possibly in a vCPU thread.  */
             return qemu_get_aio_context();
         }
         return NULL;
     }

which affects the behavior of aio_co_enter.  Before that patch, worker 
threads computed qemu_get_aio_context(), while afterwards they compute NULL.

This is not just for unit tests but also for qemu-storage-daemon and 
other block layer programs.

>> However, commit 5f50be9b5810293141bb53cfd0cb46e765367d56 changed the
>> stub to return false for a specific reason.
> 
> I must admit I don't really understand the reasoning there:
> 
>      With this change, the stub qemu_mutex_iothread_locked() must be changed
>      from true to false.  The previous value of true was needed because the
>      main thread did not have an AioContext in the thread-local variable,
>      but now it does have one.
> 
> This explains why it _can_ be changed to false for this caller, but not
> why it _must_ be changed.

See above: because it returns the wrong value for all threads except 
one, and there are better ways to do a meaningful check in that one 
thread: using qemu_get_current_aio_context(), which is what aio_co_enter 
did at the time and qemu_in_main_thread() does with Emanuele's change.

> So is the problem with the unit tests really just that they never call
> qemu_init_main_loop() and therefore never set the AioContext for the
> main thread?

No, most of them do (and if some are missing it we can add it).

Paolo

On 31/01/2022 16:49, Paolo Bonzini wrote:
> On 1/31/22 15:33, Kevin Wolf wrote:
>> I feel "use this function if your code is going to be used by unit
>> tests, but use qemu_mutex_iothread_locked() otherwise" is a very strange
>> interface. I'm relatively sure that qemu_mutex_iothread_locked() isn't
>> primarily used to make unit tests crash.
> 
> qemu_mutex_iothread_locked() should never be used in the block layer,
> because programs that use the block layer might not call an iothread
> lock, and indeed only the emulator have an iothread lock.
> 
> Making it always true would be okay when those programs were
> single-threaded, but really they all had worker threads so it was
> changed to false by commit 5f50be9b58 ("async: the main AioContext is
> only "current" if under the BQL", 2021-06-18).
> 
>> Documentation and the definition of the interface of a function
>> shouldn't be about the caller, but about the semantics of the function
>> itself. So, which question does qemu_mutex_iothread_locked() answer, and
>> which question does qemu_in_main_thread() answer?
> 
> qemu_mutex_iothread_locked() -> Does the program have exclusive access
> to the emulator's globals.
> 
> qemu_in_main_thread() -> Does the program have access to the block
> layer's globals.  In emulators this is governed by the iothread lock,
> elsewhere they are accessible only from the home thread of the initial
> AioContext.
> 
> So, in emulators it is the same question, but in the block layer one of
> them is actually meaningless.
> 
> Really the "bug" is that qemu_mutex_iothread_locked() should really not
> be used outside emulators.  There are just two uses, but it's hard to
> remove them.
> 
> So why are two functions needed?  Two reasons:
> 
> - stubs/iothread-lock.c cannot define qemu_mutex_iothread_locked() as
> "return qemu_get_current_aio_context() == qemu_get_aio_context();"
> because it would cause infinite recursion with
> qemu_get_current_aio_context()
> 
> - even if it could, frankly qemu_mutex_iothread_locked() is a horrible
> name, and in the context of the block layer it conflicts especially
> badly with the "iothread" concept.
> 
> Maybe we should embrace the BQL name and rename the functions that refer
> to the "iothread lock".  But then I don't want to have code in the block
> layer that refers to a "big lock".

So based on Paolo's reply, I would modify the function comment in this way:

@@ -242,6 +242,9 @@ AioContext *iohandler_get_aio_context(void);
  * must always be taken outside other locks.  This function helps
  * functions take different paths depending on whether the current
  * thread is running within the main loop mutex.
+ *
+ * This function should never be used in the block layer, please
+ * instead refer to qemu_in_main_thread().
  */
 bool qemu_mutex_iothread_locked(void);

+
+/**
+ * qemu_in_main_thread: same as qemu_mutex_iothread_locked when
+ * softmmu/cpus.c implementation is linked. Otherwise this function
+ * checks that the current AioContext is the global AioContext
+ * (main loop).
+ *
+ * This is useful when checking that the BQL is held as a
+ * replacement of qemu_mutex_iothread_locked() usege in the
+ * block layer, since the former returns false when invoked by
+ * unit tests or other users like qemu-storage-daemon that end
+ * up using the stubs/iothread-lock.c implementation.
+ *
+ * This function should only be used in the block layer.
+ * Use this function to determine whether it is possible to safely
+ * access the block layer's globals.
+ */
+bool qemu_in_main_thread(void);

What do you think?

Emanuele

On 2/1/22 10:08, Emanuele Giuseppe Esposito wrote:
> 
> + *
> + * This function should never be used in the block layer, please
> + * instead refer to qemu_in_main_thread().


This function should never be used in the block layer, because unit 
tests, block layer tools and qemu-storage-daemon do not have a BQL.
Please instead refer to qemu_in_main_thread().

> +
> +/**
> + * qemu_in_main_thread: same as qemu_mutex_iothread_locked when
> + * softmmu/cpus.c implementation is linked. Otherwise this function
> + * checks that the current AioContext is the global AioContext
> + * (main loop).
> + *
> + * This is useful when checking that the BQL is held as a
> + * replacement of qemu_mutex_iothread_locked() usege in the
> + * block layer, since the former returns false when invoked by
> + * unit tests or other users like qemu-storage-daemon that end
> + * up using the stubs/iothread-lock.c implementation.
> + *
> + * This function should only be used in the block layer.
> + * Use this function to determine whether it is possible to safely
> + * access the block layer's globals.
> + */
> +bool qemu_in_main_thread(void);

I think the reference to qemu_mutex_iothread_locked() complicates 
things.  It's enough to explain the different policies in my opinion:

/**
  * qemu_in_main_thread: return whether it's possible to safely access
  * the global state of the block layer.
  *
  * Global state of the block layer is not accessible from I/O threads
  * or worker threads; only from threads that "own" the default
  * AioContext that qemu_get_aio_context() returns.  For tests, block
  * layer tools and qemu-storage-daemon there is a designated thread that
  * runs the event loop for qemu_get_aio_context(), and that is the
  * main thread.
  *
  * For emulators, however, any thread that holds the BQL can act
  * as the block layer main thread; this will be any of the actual
  * main thread, the vCPU threads or the RCU thread.
  *
  * For clarity, do not use this function outside the block layer.
  */

Paolo

Am 31.01.2022 um 16:49 hat Paolo Bonzini geschrieben:
> > > However, commit 5f50be9b5810293141bb53cfd0cb46e765367d56 changed the
> > > stub to return false for a specific reason.
> > 
> > I must admit I don't really understand the reasoning there:
> > 
> >      With this change, the stub qemu_mutex_iothread_locked() must be changed
> >      from true to false.  The previous value of true was needed because the
> >      main thread did not have an AioContext in the thread-local variable,
> >      but now it does have one.
> > 
> > This explains why it _can_ be changed to false for this caller, but not
> > why it _must_ be changed.
> 
> See above: because it returns the wrong value for all threads except one,
> and there are better ways to do a meaningful check in that one thread: using
> qemu_get_current_aio_context(), which is what aio_co_enter did at the time
> and qemu_in_main_thread() does with Emanuele's change.
> 
> > So is the problem with the unit tests really just that they never call
> > qemu_init_main_loop() and therefore never set the AioContext for the
> > main thread?
> 
> No, most of them do (and if some are missing it we can add it).

But if they do, why doesn't qemu_get_current_aio_context() already
return the right result? In this case, my_aiocontext is set and it
should never even call qemu_mutex_iothread_locked() - at least not in
any case where qemu_in_main_thread() would return true.

So provided that qemu_init_main_loop() is called, both functions should
be equivalent and we wouldn't need a second one.

Kevin

Am 01.02.2022 um 12:55 hat Kevin Wolf geschrieben:
> Am 31.01.2022 um 16:49 hat Paolo Bonzini geschrieben:
> > > > However, commit 5f50be9b5810293141bb53cfd0cb46e765367d56 changed the
> > > > stub to return false for a specific reason.
> > > 
> > > I must admit I don't really understand the reasoning there:
> > > 
> > >      With this change, the stub qemu_mutex_iothread_locked() must be changed
> > >      from true to false.  The previous value of true was needed because the
> > >      main thread did not have an AioContext in the thread-local variable,
> > >      but now it does have one.
> > > 
> > > This explains why it _can_ be changed to false for this caller, but not
> > > why it _must_ be changed.
> > 
> > See above: because it returns the wrong value for all threads except one,
> > and there are better ways to do a meaningful check in that one thread: using
> > qemu_get_current_aio_context(), which is what aio_co_enter did at the time
> > and qemu_in_main_thread() does with Emanuele's change.
> > 
> > > So is the problem with the unit tests really just that they never call
> > > qemu_init_main_loop() and therefore never set the AioContext for the
> > > main thread?
> > 
> > No, most of them do (and if some are missing it we can add it).
> 
> But if they do, why doesn't qemu_get_current_aio_context() already
> return the right result? In this case, my_aiocontext is set and it
> should never even call qemu_mutex_iothread_locked() - at least not in
> any case where qemu_in_main_thread() would return true.
> 
> So provided that qemu_init_main_loop() is called, both functions should
> be equivalent and we wouldn't need a second one.

Sorry, I was confused and comparing the wrong two functions.
qemu_get_current_aio_context() does return the right result and it's
exactly what qemu_in_main_thread() uses. So yes, it's right and it's
different from qemu_mutex_iothread_locked().

It would be less confusing if qemu_get_current_aio_context() used
qemu_in_main_thread() (with two different implementations of
qemu_in_main_thread() for the system emulator and tools) instead of the
other way around, but I guess that's harder to implement because we
would need a different way to figure out whether we're in the main
thread, at least as long as my_aiocontext is static and can't be
accessed in stubs. We could probably make it public, though.

Kevin