On Fri, Nov 07, 2025 at 10:47:35AM +0900, Akihiko Odaki wrote:
> On 2025/11/07 6:52, Peter Xu wrote:
> > On Thu, Nov 06, 2025 at 10:40:52AM +0900, Akihiko Odaki wrote:
> > > > > > > + /*
> > > > > > > + * Ensure that the forced variable has not been set
> > > > > > > after fetching
> > > > > > > + * rcu_call_count; otherwise we may get confused by a
> > > > > > > force quiescent
> > > > > > > + * state request for an element later than n.
> > > > > > > + */
> > > > > > > + while (qatomic_xchg(&forced, false)) {
> > > > > > > + sleep = false;
> > > > > > > + n = qatomic_read(&rcu_call_count);
> > > > > > > }
> > > > > >
> > > > > > This is pretty tricky, and I wonder if it will make the code easier
> > > > > > to read
> > > > > > if we convert the sync_event to be a semaphore instead. When as a
> > > > > > sem, it
> > > > > > will take account of whatever kick to it, either a call_rcu1() or an
> > > > > > enforced rcu flush, so that we don't need to reset it. Meanwhile,
> > > > > > we don't
> > > > > > worry on an slightly outdated "n" read because the 2nd round of
> > > > > > sem_wait()
> > > > > > will catch that new "n".
> > > > > >
> > > > > > Instead, worst case is rcu thread runs one more round without seeing
> > > > > > callbacks on the queue.
> > > > > >
> > > > > > I'm not sure if that could help simplying code, maybe also make it
> > > > > > less
> > > > > > error-prone.
> > > > >
> > > > > Semaphore is not applicable here because it will not de-duplicate
> > > > > concurrent
> > > > > kicks of RCU threads.
> > > >
> > > > Why concurrent kicks of rcu threads is a problem? QemuSemaphore is
> > > > thread-safe itself, meanwhile IIUC it only still causes
> > > > call_rcu_thread()
> > > > loops some more rounds reading "n", which looks all safe. No?
> > >
> > > It is safe but incurs overheads and confusing. QemuEvent represents the
> > > boolean semantics better.
> > >
> > > I also have difficulty to understand how converting sync_event to a
> > > semaphore simplifies the code. Perhaps some (pseudo)code to show how the
> > > code will look like may be useful.
> >
> > I prepared a patch on top of your current patchset to show what I meant. I
> > also added comments and some test results showing why I think it might be
> > fine that the sem overhead should be small.
> >
> > In short, I tested a VM with 8 vCPUs and 4G mem, booting Linux and properly
> > poweroff, I only saw <1000 rcu_call1 users in total. That should be the
> > max-bound of sem overhead on looping in rcu thread.
> >
> > It's in patch format but still treat it as a comment instead to discuss
> > with you. Attaching it is just easier for me.
> >
> > ===8<===
> > From 71f15ed19050a973088352a8d71b6cc6b7b5f7cf Mon Sep 17 00:00:00 2001
> > From: Peter Xu <[email protected]>
> > Date: Thu, 6 Nov 2025 16:03:00 -0500
> > Subject: [PATCH] rcu: Make sync_event a semaphore
> >
> > It could simply all reset logic, especially after enforced rcu is
> > introduced we'll also need a tweak to re-read "n", which can be avoided too
> > when with a sem.
> >
> > However, the sem can introduce an overhead in high frequecy rcu frees.
> > This patch is drafted with the assumption that rcu free is at least very
> > rare in QEMU, hence it's not a problem.
> >
> > When I tested with this command:
> >
> > qemu-system-x86_64 -M q35,kernel-irqchip=split,suppress-vmdesc=on -smp 8 \
> > -m 4G -msg timestamp=on -name peter-vm,debug-threads=on -cpu Nehalem \
> > -accel kvm -qmp unix:/tmp/peter.sock,server,nowait -nographic \
> > -monitor telnet::6666,server,nowait -netdev
> > user,id=net0,hostfwd=tcp::5555-:22
> > -device e1000,netdev=net0 -device virtio-balloon $DISK
> >
> > I booted a pre-installed Linux, login and poweroff, wait until VM
> > completely shutdowns. I captured less than 1000 rcu_free1() calls in
> > summary. It means for the whole lifetime of such VM the max overhead of
> > the call_rcu_thread() loop reading rcu_call_count will be 1000 loops.
> >
> > Signed-off-by: Peter Xu <[email protected]>
> > ---
> > util/rcu.c | 36 ++++++++----------------------------
> > 1 file changed, 8 insertions(+), 28 deletions(-)
> >
> > diff --git a/util/rcu.c b/util/rcu.c
> > index 85f9333f5d..dfe031a5c9 100644
> > --- a/util/rcu.c
> > +++ b/util/rcu.c
> > @@ -54,7 +54,7 @@ static int rcu_call_count;
> > static QemuMutex rcu_registry_lock;
> > /* Set when the forced variable is set or rcu_call_count becomes
> > non-zero. */
> > -static QemuEvent sync_event;
> > +static QemuSemaphore sync_event;
> > /*
> > * Check whether a quiescent state was crossed between the beginning of
> > @@ -80,7 +80,7 @@ static ThreadList registry =
> > QLIST_HEAD_INITIALIZER(registry);
> > void force_rcu(void)
> > {
> > qatomic_set(&forced, true);
> > - qemu_event_set(&sync_event);
> > + qemu_sem_post(&sync_event);
> > }
> > /* Wait for previous parity/grace period to be empty of readers. */
> > @@ -148,7 +148,7 @@ static void wait_for_readers(bool sleep)
> > */
> > qemu_event_reset(&rcu_gp_event);
> > } else if (qatomic_read(&rcu_call_count) >= RCU_CALL_MIN_SIZE ||
> > - !sleeps || qemu_event_timedwait(&sync_event, 10)) {
> > + !sleeps || qemu_sem_timedwait(&sync_event, 10)) {
> > /*
> > * Now one of the following heuristical conditions is
> > satisfied:
> > * - A decent number of callbacks piled up.
> > @@ -286,7 +286,6 @@ static void *call_rcu_thread(void *opaque)
> > rcu_register_thread();
> > for (;;) {
> > - bool sleep = true;
> > int n;
> > /*
> > @@ -294,7 +293,6 @@ static void *call_rcu_thread(void *opaque)
> > * added before enter_qs() starts.
> > */
> > for (;;) {
> > - qemu_event_reset(&sync_event);
> > n = qatomic_read(&rcu_call_count);
> > if (n) {
> > break;
> > @@ -303,36 +301,19 @@ static void *call_rcu_thread(void *opaque)
> > #if defined(CONFIG_MALLOC_TRIM)
> > malloc_trim(4 * 1024 * 1024);
> > #endif
> > - qemu_event_wait(&sync_event);
> > + qemu_sem_wait(&sync_event);
> > }
> > - /*
> > - * Ensure that an event for a rcu_call_count change will not
> > interrupt
> > - * wait_for_readers().
> > - */
> > - qemu_event_reset(&sync_event);
> > -
> > - /*
> > - * Ensure that the forced variable has not been set after fetching
> > - * rcu_call_count; otherwise we may get confused by a force
> > quiescent
> > - * state request for an element later than n.
> > - */
> > - while (qatomic_xchg(&forced, false)) {
> > - sleep = false;
> > - n = qatomic_read(&rcu_call_count);
> > - }
> > -
> > - enter_qs(sleep);
> > + enter_qs(!qatomic_xchg(&forced, false));
>
> This is not OK; the forced variable may be set after rcu_call_count is
> fetched. In that case, we should avoid unsetting the force quiescent state
> request for the elements later than "n" or refetch "n".
Indeed I missed that part, but it should be trivial to fix, on top of my
previous patch:
===8<===
diff --git a/util/rcu.c b/util/rcu.c
index dfe031a5c9..aff98d9ee2 100644
--- a/util/rcu.c
+++ b/util/rcu.c
@@ -286,6 +286,7 @@ static void *call_rcu_thread(void *opaque)
rcu_register_thread();
for (;;) {
+ bool sleep;
int n;
/*
@@ -293,6 +294,7 @@ static void *call_rcu_thread(void *opaque)
* added before enter_qs() starts.
*/
for (;;) {
+ sleep = !qatomic_xchg(&forced, false);
n = qatomic_read(&rcu_call_count);
if (n) {
break;
@@ -304,7 +306,7 @@ static void *call_rcu_thread(void *opaque)
qemu_sem_wait(&sync_event);
}
- enter_qs(!qatomic_xchg(&forced, false));
+ enter_qs(sleep);
qatomic_sub(&rcu_call_count, n);
bql_lock();
while (n > 0) {
===8<===
The idea is still the same, using semaphore can avoid explicit resets and a
lot of other ordering constraints like reading call_count, etc.
E.g. even before this series, we still need to properly reset at explicit
time to make sure we can capture a set() correct. When with sem, all these
issues are gone simply because we won't miss post() when it's a counter not
boolean.
Also, would you please also have a look at other comments I left in the
same email (after the patch I attached)?
https://lore.kernel.org/qemu-devel/[email protected]/
Can search "When I was having a closer look, I found some other issues".
Thanks,
--
Peter Xu
