| Message ID | 20221118091858.242569-2-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | main-loop: Introduce QEMU_IOTHREAD_LOCK_GUARD | expand |
Richard Henderson <richard.henderson@linaro.org> writes: > Create a wrapper for locking/unlocking the iothread lock. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > Cc: Paolo Bonzini <pbonzini@redhat.com> (maintainer:Main loop) You might want to review Paolo's comments from: Subject: [RFC PATCH] main-loop: introduce WITH_QEMU_IOTHREAD_LOCK Date: Mon, 24 Oct 2022 18:19:09 +0100 Message-Id: <20221024171909.434818-1-alex.bennee@linaro.org> So it would be worth having the WITH_QEMU_IOTHREAD_LOCK() and MAYBE_WITH_QEMU_IOTHREAD_LOCK() helpers for completeness. And of course the name cleanup. > --- > include/qemu/main-loop.h | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h > index 3c9a9a982d..c25f390696 100644 > --- a/include/qemu/main-loop.h > +++ b/include/qemu/main-loop.h > @@ -343,6 +343,35 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line); > */ > void qemu_mutex_unlock_iothread(void); > > +/** > + * QEMU_IOTHREAD_LOCK_GUARD > + * > + * Wrap a block of code in a conditional qemu_mutex_{lock,unlock}_iothread. > + */ > +typedef struct IOThreadLockAuto IOThreadLockAuto; > + > +static inline IOThreadLockAuto *qemu_iothread_auto_lock(const char *file, > + int line) > +{ > + if (qemu_mutex_iothread_locked()) { > + return NULL; > + } > + qemu_mutex_lock_iothread_impl(file, line); > + /* Anything non-NULL causes the cleanup function to be called */ > + return (IOThreadLockAuto *)(uintptr_t)1; > +} > + > +static inline void qemu_iothread_auto_unlock(IOThreadLockAuto *l) > +{ > + qemu_mutex_unlock_iothread(); > +} > + > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(IOThreadLockAuto, qemu_iothread_auto_unlock) > + > +#define QEMU_IOTHREAD_LOCK_GUARD() \ > + g_autoptr(IOThreadLockAuto) _iothread_lock_auto __attribute__((unused)) \ > + = qemu_iothread_auto_lock(__FILE__, __LINE__) > + > /* > * qemu_cond_wait_iothread: Wait on condition for the main loop mutex > *
Richard Henderson <richard.henderson@linaro.org> writes: > Create a wrapper for locking/unlocking the iothread lock. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > Cc: Paolo Bonzini <pbonzini@redhat.com> (maintainer:Main loop) > --- > include/qemu/main-loop.h | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h > index 3c9a9a982d..c25f390696 100644 > --- a/include/qemu/main-loop.h > +++ b/include/qemu/main-loop.h > @@ -343,6 +343,35 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line); > */ > void qemu_mutex_unlock_iothread(void); > > +/** > + * QEMU_IOTHREAD_LOCK_GUARD > + * > + * Wrap a block of code in a conditional qemu_mutex_{lock,unlock}_iothread. > + */ > +typedef struct IOThreadLockAuto IOThreadLockAuto; > + > +static inline IOThreadLockAuto *qemu_iothread_auto_lock(const char *file, > + int line) > +{ > + if (qemu_mutex_iothread_locked()) { > + return NULL; > + } > + qemu_mutex_lock_iothread_impl(file, line); > + /* Anything non-NULL causes the cleanup function to be called */ > + return (IOThreadLockAuto *)(uintptr_t)1; Oh hang on, what black magic is this. Does the compiler do a NULL check before calling the cleanup? > +} > + > +static inline void qemu_iothread_auto_unlock(IOThreadLockAuto *l) > +{ > + qemu_mutex_unlock_iothread(); > +} > + > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(IOThreadLockAuto, qemu_iothread_auto_unlock) > + > +#define QEMU_IOTHREAD_LOCK_GUARD() \ > + g_autoptr(IOThreadLockAuto) _iothread_lock_auto __attribute__((unused)) \ > + = qemu_iothread_auto_lock(__FILE__, __LINE__) > + > /* > * qemu_cond_wait_iothread: Wait on condition for the main loop mutex > *
On 11/18/22 05:38, Alex Bennée wrote: > > Richard Henderson <richard.henderson@linaro.org> writes: > >> Create a wrapper for locking/unlocking the iothread lock. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> Cc: Paolo Bonzini <pbonzini@redhat.com> (maintainer:Main loop) >> --- >> include/qemu/main-loop.h | 29 +++++++++++++++++++++++++++++ >> 1 file changed, 29 insertions(+) >> >> diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h >> index 3c9a9a982d..c25f390696 100644 >> --- a/include/qemu/main-loop.h >> +++ b/include/qemu/main-loop.h >> @@ -343,6 +343,35 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line); >> */ >> void qemu_mutex_unlock_iothread(void); >> >> +/** >> + * QEMU_IOTHREAD_LOCK_GUARD >> + * >> + * Wrap a block of code in a conditional qemu_mutex_{lock,unlock}_iothread. >> + */ >> +typedef struct IOThreadLockAuto IOThreadLockAuto; >> + >> +static inline IOThreadLockAuto *qemu_iothread_auto_lock(const char *file, >> + int line) >> +{ >> + if (qemu_mutex_iothread_locked()) { >> + return NULL; >> + } >> + qemu_mutex_lock_iothread_impl(file, line); >> + /* Anything non-NULL causes the cleanup function to be called */ >> + return (IOThreadLockAuto *)(uintptr_t)1; > > Oh hang on, what black magic is this. Does the compiler do a NULL check > before calling the cleanup? Not the compiler, but... >> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(IOThreadLockAuto, qemu_iothread_auto_unlock) ... this does. Follow the macros down and you get > static G_GNUC_UNUSED inline void _GLIB_AUTOPTR_CLEAR_FUNC_NAME(TypeName) (TypeName *_ptr) > { if (_ptr) (cleanup) ((ParentName *) _ptr); } r~
On 11/18/22 05:30, Alex Bennée wrote: > > Richard Henderson <richard.henderson@linaro.org> writes: > >> Create a wrapper for locking/unlocking the iothread lock. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> Cc: Paolo Bonzini <pbonzini@redhat.com> (maintainer:Main loop) > > You might want to review Paolo's comments from: > > Subject: [RFC PATCH] main-loop: introduce WITH_QEMU_IOTHREAD_LOCK > Date: Mon, 24 Oct 2022 18:19:09 +0100 > Message-Id: <20221024171909.434818-1-alex.bennee@linaro.org> > > So it would be worth having the WITH_QEMU_IOTHREAD_LOCK() and > MAYBE_WITH_QEMU_IOTHREAD_LOCK() helpers for completeness. I don't see that (MAYBE_)WITH_QEMU_IOTHREAD_LOCK is particularly useful in any of the cases that I converted. > And of course the name cleanup. What name cleanup? r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 11/18/22 05:30, Alex Bennée wrote: >> Richard Henderson <richard.henderson@linaro.org> writes: >> >>> Create a wrapper for locking/unlocking the iothread lock. >>> >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>> --- >>> Cc: Paolo Bonzini <pbonzini@redhat.com> (maintainer:Main loop) >> You might want to review Paolo's comments from: >> Subject: [RFC PATCH] main-loop: introduce WITH_QEMU_IOTHREAD_LOCK >> Date: Mon, 24 Oct 2022 18:19:09 +0100 >> Message-Id: <20221024171909.434818-1-alex.bennee@linaro.org> >> So it would be worth having the WITH_QEMU_IOTHREAD_LOCK() and >> MAYBE_WITH_QEMU_IOTHREAD_LOCK() helpers for completeness. > > I don't see that (MAYBE_)WITH_QEMU_IOTHREAD_LOCK is particularly > useful in any of the cases that I converted. Fair enough - as long as they are easy enough to add later. The WITH_ forms do work nicely to wrap a particular area under lock and make things visually clear vs the LOCK_GUARD which basically holds the lock to the end of function or exit. > >> And of course the name cleanup. > > What name cleanup? Also lots of bonus points for finally renaming these functions to "*_main_thread" rather than "*_iothread" since, confusingly, iothreads (plural) are the only ones that do not and cannot take the "iothread lock". > > > r~
On 21/11/22 10:55, Alex Bennée wrote: > > Richard Henderson <richard.henderson@linaro.org> writes: > >> On 11/18/22 05:30, Alex Bennée wrote: >>> Richard Henderson <richard.henderson@linaro.org> writes: >>> >>>> Create a wrapper for locking/unlocking the iothread lock. >>>> >>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>>> --- >>>> Cc: Paolo Bonzini <pbonzini@redhat.com> (maintainer:Main loop) >>> You might want to review Paolo's comments from: >>> Subject: [RFC PATCH] main-loop: introduce WITH_QEMU_IOTHREAD_LOCK >>> Date: Mon, 24 Oct 2022 18:19:09 +0100 >>> Message-Id: <20221024171909.434818-1-alex.bennee@linaro.org> >>> So it would be worth having the WITH_QEMU_IOTHREAD_LOCK() and >>> MAYBE_WITH_QEMU_IOTHREAD_LOCK() helpers for completeness. >> >> I don't see that (MAYBE_)WITH_QEMU_IOTHREAD_LOCK is particularly >> useful in any of the cases that I converted. > > Fair enough - as long as they are easy enough to add later. The WITH_ > forms do work nicely to wrap a particular area under lock and make > things visually clear vs the LOCK_GUARD which basically holds the lock > to the end of function or exit. I concur for WITH_QEMU_IOTHREAD_LOCK(), it is a no-brainer.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h index 3c9a9a982d..c25f390696 100644 --- a/include/qemu/main-loop.h +++ b/include/qemu/main-loop.h @@ -343,6 +343,35 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line); */ void qemu_mutex_unlock_iothread(void); +/** + * QEMU_IOTHREAD_LOCK_GUARD + * + * Wrap a block of code in a conditional qemu_mutex_{lock,unlock}_iothread. + */ +typedef struct IOThreadLockAuto IOThreadLockAuto; + +static inline IOThreadLockAuto *qemu_iothread_auto_lock(const char *file, + int line) +{ + if (qemu_mutex_iothread_locked()) { + return NULL; + } + qemu_mutex_lock_iothread_impl(file, line); + /* Anything non-NULL causes the cleanup function to be called */ + return (IOThreadLockAuto *)(uintptr_t)1; +} + +static inline void qemu_iothread_auto_unlock(IOThreadLockAuto *l) +{ + qemu_mutex_unlock_iothread(); +} + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(IOThreadLockAuto, qemu_iothread_auto_unlock) + +#define QEMU_IOTHREAD_LOCK_GUARD() \ + g_autoptr(IOThreadLockAuto) _iothread_lock_auto __attribute__((unused)) \ + = qemu_iothread_auto_lock(__FILE__, __LINE__) + /* * qemu_cond_wait_iothread: Wait on condition for the main loop mutex *
Create a wrapper for locking/unlocking the iothread lock. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- Cc: Paolo Bonzini <pbonzini@redhat.com> (maintainer:Main loop) --- include/qemu/main-loop.h | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)