Re: [PATCH 6/7] include: Move QemuLockCnt APIs to their own header

[Paolo Bonzini <pbonzini@redhat.com>]

On 8/16/24 15:22, Peter Maydell wrote:
> Currently the QemuLockCnt data structure and associated functions are
> in the include/qemu/thread.h header.  Move them to their own
> qemu/lockcnt.h.  The main reason for doing this is that it means we
> can autogenerate the documentation comments into the docs/devel
> documentation.
> 
> The copyright/author in the new header is drawn from lockcnt.c,
> since the header changes were added in the same commit as
> lockcnt.c; since neither thread.h nor lockcnt.c state an explicit
> license, the standard default of GPL-2-or-later applies.
> 
> We include the new header (and the .c file, which was accidentally
> omitted previously) in the "RCU" part of MAINTAINERS, since that
> is where the lockcnt.rst documentation is categorized.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Yeah, GPLv2+ is intended for both these declarations and lockcnt.c.

Thanks,

Paolo

> ---
> Paolo: could you confirm that you meant GPL2+ for this code?
> ---
>   MAINTAINERS            |   2 +
>   docs/devel/lockcnt.rst |   2 +-
>   include/block/aio.h    |   1 +
>   include/hw/core/cpu.h  |   1 +
>   include/qemu/lockcnt.h | 130 +++++++++++++++++++++++++++++++++++++++++
>   include/qemu/thread.h  | 111 -----------------------------------
>   accel/accel-blocker.c  |   1 +
>   hw/core/cpu-common.c   |   1 +
>   util/aio-posix.c       |   1 +
>   util/aio-win32.c       |   1 +
>   util/async.c           |   1 +
>   util/fdmon-epoll.c     |   1 +
>   util/lockcnt.c         |   1 +
>   13 files changed, 142 insertions(+), 112 deletions(-)
>   create mode 100644 include/qemu/lockcnt.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f8f4df44460..2da11411ff3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3115,8 +3115,10 @@ S: Maintained
>   F: docs/devel/lockcnt.rst
>   F: docs/devel/rcu.rst
>   F: include/qemu/rcu*.h
> +F: include/qemu/lockcnt.h
>   F: tests/unit/rcutorture.c
>   F: tests/unit/test-rcu-*.c
> +F: util/lockcnt.c
>   F: util/rcu.c
>   
>   Human Monitor (HMP)
> diff --git a/docs/devel/lockcnt.rst b/docs/devel/lockcnt.rst
> index 994aeb57151..728594bcea3 100644
> --- a/docs/devel/lockcnt.rst
> +++ b/docs/devel/lockcnt.rst
> @@ -175,7 +175,7 @@ three instructions in the critical path, two assignments and a ``smp_wmb()``.
>   ``QemuLockCnt`` API
>   -------------------
>   
> -The ``QemuLockCnt`` API is described in ``include/qemu/thread.h``.
> +The ``QemuLockCnt`` API is described in ``include/qemu/lockcnt.h``.
>   
>   
>   ``QemuLockCnt`` usage
> diff --git a/include/block/aio.h b/include/block/aio.h
> index 4ee81936ed5..43883a8a33a 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -20,6 +20,7 @@
>   #include "qemu/coroutine-core.h"
>   #include "qemu/queue.h"
>   #include "qemu/event_notifier.h"
> +#include "qemu/lockcnt.h"
>   #include "qemu/thread.h"
>   #include "qemu/timer.h"
>   #include "block/graph-lock.h"
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 1c9c775df65..ecbeeb1c0dd 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -33,6 +33,7 @@
>   #include "qemu/bitmap.h"
>   #include "qemu/rcu_queue.h"
>   #include "qemu/queue.h"
> +#include "qemu/lockcnt.h"
>   #include "qemu/thread.h"
>   #include "qom/object.h"
>   
> diff --git a/include/qemu/lockcnt.h b/include/qemu/lockcnt.h
> new file mode 100644
> index 00000000000..2c92ae17c9e
> --- /dev/null
> +++ b/include/qemu/lockcnt.h
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * QemuLockCnt implementation
> + *
> + * Copyright Red Hat, Inc. 2017
> + *
> + * Author:
> + *   Paolo Bonzini <pbonzini@redhat.com>
> + *
> + */
> +
> +#ifndef QEMU_LOCKCNT_H
> +#define QEMU_LOCKCNT_H
> +
> +#include "qemu/thread.h"
> +
> +typedef struct QemuLockCnt QemuLockCnt;
> +
> +struct QemuLockCnt {
> +#ifndef CONFIG_LINUX
> +    QemuMutex mutex;
> +#endif
> +    unsigned count;
> +};
> +
> +/**
> + * qemu_lockcnt_init: initialize a QemuLockcnt
> + * @lockcnt: the lockcnt to initialize
> + *
> + * Initialize lockcnt's counter to zero and prepare its mutex
> + * for usage.
> + */
> +void qemu_lockcnt_init(QemuLockCnt *lockcnt);
> +
> +/**
> + * qemu_lockcnt_destroy: destroy a QemuLockcnt
> + * @lockcnt: the lockcnt to destruct
> + *
> + * Destroy lockcnt's mutex.
> + */
> +void qemu_lockcnt_destroy(QemuLockCnt *lockcnt);
> +
> +/**
> + * qemu_lockcnt_inc: increment a QemuLockCnt's counter
> + * @lockcnt: the lockcnt to operate on
> + *
> + * If the lockcnt's count is zero, wait for critical sections
> + * to finish and increment lockcnt's count to 1.  If the count
> + * is not zero, just increment it.
> + *
> + * Because this function can wait on the mutex, it must not be
> + * called while the lockcnt's mutex is held by the current thread.
> + * For the same reason, qemu_lockcnt_inc can also contribute to
> + * AB-BA deadlocks.  This is a sample deadlock scenario:
> + *
> + *            thread 1                      thread 2
> + *            -------------------------------------------------------
> + *            qemu_lockcnt_lock(&lc1);
> + *                                          qemu_lockcnt_lock(&lc2);
> + *            qemu_lockcnt_inc(&lc2);
> + *                                          qemu_lockcnt_inc(&lc1);
> + */
> +void qemu_lockcnt_inc(QemuLockCnt *lockcnt);
> +
> +/**
> + * qemu_lockcnt_dec: decrement a QemuLockCnt's counter
> + * @lockcnt: the lockcnt to operate on
> + */
> +void qemu_lockcnt_dec(QemuLockCnt *lockcnt);
> +
> +/**
> + * qemu_lockcnt_dec_and_lock: decrement a QemuLockCnt's counter and
> + * possibly lock it.
> + * @lockcnt: the lockcnt to operate on
> + *
> + * Decrement lockcnt's count.  If the new count is zero, lock
> + * the mutex and return true.  Otherwise, return false.
> + */
> +bool qemu_lockcnt_dec_and_lock(QemuLockCnt *lockcnt);
> +
> +/**
> + * qemu_lockcnt_dec_if_lock: possibly decrement a QemuLockCnt's counter and
> + * lock it.
> + * @lockcnt: the lockcnt to operate on
> + *
> + * If the count is 1, decrement the count to zero, lock
> + * the mutex and return true.  Otherwise, return false.
> + */
> +bool qemu_lockcnt_dec_if_lock(QemuLockCnt *lockcnt);
> +
> +/**
> + * qemu_lockcnt_lock: lock a QemuLockCnt's mutex.
> + * @lockcnt: the lockcnt to operate on
> + *
> + * Remember that concurrent visits are not blocked unless the count is
> + * also zero.  You can use qemu_lockcnt_count to check for this inside a
> + * critical section.
> + */
> +void qemu_lockcnt_lock(QemuLockCnt *lockcnt);
> +
> +/**
> + * qemu_lockcnt_unlock: release a QemuLockCnt's mutex.
> + * @lockcnt: the lockcnt to operate on.
> + */
> +void qemu_lockcnt_unlock(QemuLockCnt *lockcnt);
> +
> +/**
> + * qemu_lockcnt_inc_and_unlock: combined unlock/increment on a QemuLockCnt.
> + * @lockcnt: the lockcnt to operate on.
> + *
> + * This is the same as
> + *
> + *     qemu_lockcnt_unlock(lockcnt);
> + *     qemu_lockcnt_inc(lockcnt);
> + *
> + * but more efficient.
> + */
> +void qemu_lockcnt_inc_and_unlock(QemuLockCnt *lockcnt);
> +
> +/**
> + * qemu_lockcnt_count: query a LockCnt's count.
> + * @lockcnt: the lockcnt to query.
> + *
> + * Note that the count can change at any time.  Still, while the
> + * lockcnt is locked, one can usefully check whether the count
> + * is non-zero.
> + */
> +unsigned qemu_lockcnt_count(QemuLockCnt *lockcnt);
> +
> +#endif
> diff --git a/include/qemu/thread.h b/include/qemu/thread.h
> index fb74e21c08a..7eba27a7049 100644
> --- a/include/qemu/thread.h
> +++ b/include/qemu/thread.h
> @@ -293,115 +293,4 @@ static inline void qemu_spin_unlock(QemuSpin *spin)
>   #endif
>   }
>   
> -struct QemuLockCnt {
> -#ifndef CONFIG_LINUX
> -    QemuMutex mutex;
> -#endif
> -    unsigned count;
> -};
> -
> -/**
> - * qemu_lockcnt_init: initialize a QemuLockcnt
> - * @lockcnt: the lockcnt to initialize
> - *
> - * Initialize lockcnt's counter to zero and prepare its mutex
> - * for usage.
> - */
> -void qemu_lockcnt_init(QemuLockCnt *lockcnt);
> -
> -/**
> - * qemu_lockcnt_destroy: destroy a QemuLockcnt
> - * @lockcnt: the lockcnt to destruct
> - *
> - * Destroy lockcnt's mutex.
> - */
> -void qemu_lockcnt_destroy(QemuLockCnt *lockcnt);
> -
> -/**
> - * qemu_lockcnt_inc: increment a QemuLockCnt's counter
> - * @lockcnt: the lockcnt to operate on
> - *
> - * If the lockcnt's count is zero, wait for critical sections
> - * to finish and increment lockcnt's count to 1.  If the count
> - * is not zero, just increment it.
> - *
> - * Because this function can wait on the mutex, it must not be
> - * called while the lockcnt's mutex is held by the current thread.
> - * For the same reason, qemu_lockcnt_inc can also contribute to
> - * AB-BA deadlocks.  This is a sample deadlock scenario:
> - *
> - *            thread 1                      thread 2
> - *            -------------------------------------------------------
> - *            qemu_lockcnt_lock(&lc1);
> - *                                          qemu_lockcnt_lock(&lc2);
> - *            qemu_lockcnt_inc(&lc2);
> - *                                          qemu_lockcnt_inc(&lc1);
> - */
> -void qemu_lockcnt_inc(QemuLockCnt *lockcnt);
> -
> -/**
> - * qemu_lockcnt_dec: decrement a QemuLockCnt's counter
> - * @lockcnt: the lockcnt to operate on
> - */
> -void qemu_lockcnt_dec(QemuLockCnt *lockcnt);
> -
> -/**
> - * qemu_lockcnt_dec_and_lock: decrement a QemuLockCnt's counter and
> - * possibly lock it.
> - * @lockcnt: the lockcnt to operate on
> - *
> - * Decrement lockcnt's count.  If the new count is zero, lock
> - * the mutex and return true.  Otherwise, return false.
> - */
> -bool qemu_lockcnt_dec_and_lock(QemuLockCnt *lockcnt);
> -
> -/**
> - * qemu_lockcnt_dec_if_lock: possibly decrement a QemuLockCnt's counter and
> - * lock it.
> - * @lockcnt: the lockcnt to operate on
> - *
> - * If the count is 1, decrement the count to zero, lock
> - * the mutex and return true.  Otherwise, return false.
> - */
> -bool qemu_lockcnt_dec_if_lock(QemuLockCnt *lockcnt);
> -
> -/**
> - * qemu_lockcnt_lock: lock a QemuLockCnt's mutex.
> - * @lockcnt: the lockcnt to operate on
> - *
> - * Remember that concurrent visits are not blocked unless the count is
> - * also zero.  You can use qemu_lockcnt_count to check for this inside a
> - * critical section.
> - */
> -void qemu_lockcnt_lock(QemuLockCnt *lockcnt);
> -
> -/**
> - * qemu_lockcnt_unlock: release a QemuLockCnt's mutex.
> - * @lockcnt: the lockcnt to operate on.
> - */
> -void qemu_lockcnt_unlock(QemuLockCnt *lockcnt);
> -
> -/**
> - * qemu_lockcnt_inc_and_unlock: combined unlock/increment on a QemuLockCnt.
> - * @lockcnt: the lockcnt to operate on.
> - *
> - * This is the same as
> - *
> - *     qemu_lockcnt_unlock(lockcnt);
> - *     qemu_lockcnt_inc(lockcnt);
> - *
> - * but more efficient.
> - */
> -void qemu_lockcnt_inc_and_unlock(QemuLockCnt *lockcnt);
> -
> -/**
> - * qemu_lockcnt_count: query a LockCnt's count.
> - * @lockcnt: the lockcnt to query.
> - *
> - * Note that the count can change at any time.  Still, while the
> - * lockcnt is locked, one can usefully check whether the count
> - * is non-zero.
> - */
> -unsigned qemu_lockcnt_count(QemuLockCnt *lockcnt);
> -
>   #endif
> diff --git a/accel/accel-blocker.c b/accel/accel-blocker.c
> index e083f24aa80..75daaa29113 100644
> --- a/accel/accel-blocker.c
> +++ b/accel/accel-blocker.c
> @@ -25,6 +25,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/lockcnt.h"
>   #include "qemu/thread.h"
>   #include "qemu/main-loop.h"
>   #include "hw/core/cpu.h"
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index 7982ecd39a5..09c79035949 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -24,6 +24,7 @@
>   #include "sysemu/hw_accel.h"
>   #include "qemu/log.h"
>   #include "qemu/main-loop.h"
> +#include "qemu/lockcnt.h"
>   #include "exec/log.h"
>   #include "exec/gdbstub.h"
>   #include "sysemu/tcg.h"
> diff --git a/util/aio-posix.c b/util/aio-posix.c
> index 266c9dd35fa..06bf9f456cf 100644
> --- a/util/aio-posix.c
> +++ b/util/aio-posix.c
> @@ -17,6 +17,7 @@
>   #include "block/block.h"
>   #include "block/thread-pool.h"
>   #include "qemu/main-loop.h"
> +#include "qemu/lockcnt.h"
>   #include "qemu/rcu.h"
>   #include "qemu/rcu_queue.h"
>   #include "qemu/sockets.h"
> diff --git a/util/aio-win32.c b/util/aio-win32.c
> index d144f9391fb..6583d5c5f31 100644
> --- a/util/aio-win32.c
> +++ b/util/aio-win32.c
> @@ -18,6 +18,7 @@
>   #include "qemu/osdep.h"
>   #include "block/block.h"
>   #include "qemu/main-loop.h"
> +#include "qemu/lockcnt.h"
>   #include "qemu/queue.h"
>   #include "qemu/sockets.h"
>   #include "qapi/error.h"
> diff --git a/util/async.c b/util/async.c
> index 3e3e4fc7126..99db28389f6 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -30,6 +30,7 @@
>   #include "block/graph-lock.h"
>   #include "qemu/main-loop.h"
>   #include "qemu/atomic.h"
> +#include "qemu/lockcnt.h"
>   #include "qemu/rcu_queue.h"
>   #include "block/raw-aio.h"
>   #include "qemu/coroutine_int.h"
> diff --git a/util/fdmon-epoll.c b/util/fdmon-epoll.c
> index c6413cb18fe..9fb8800dde8 100644
> --- a/util/fdmon-epoll.c
> +++ b/util/fdmon-epoll.c
> @@ -5,6 +5,7 @@
>   
>   #include "qemu/osdep.h"
>   #include <sys/epoll.h>
> +#include "qemu/lockcnt.h"
>   #include "qemu/rcu_queue.h"
>   #include "aio-posix.h"
>   
> diff --git a/util/lockcnt.c b/util/lockcnt.c
> index 5da36946b1b..d07c6cc5cee 100644
> --- a/util/lockcnt.c
> +++ b/util/lockcnt.c
> @@ -7,6 +7,7 @@
>    *   Paolo Bonzini <pbonzini@redhat.com>
>    */
>   #include "qemu/osdep.h"
> +#include "qemu/lockcnt.h"
>   #include "qemu/thread.h"
>   #include "qemu/atomic.h"
>   #include "trace.h"