On Fri, Nov 14, 2025 at 10:24:40AM +0900, Akihiko Odaki wrote:
> However it creates another problem. Think of the following sequence:
>
> call_rcu_thread() |
> | call_rcu1()
> | qatomic_fetch_inc(&rcu_call_count)
> | qemu_sem_post(&sync_event)
> |
> qatomic_read(&rcu_call_count) |
> enter_qs(false) |
> wait_for_readers(false) |
> qemu_sem_timedwait( |
> &sync_event, 10) |
>
> qemu_sem_timedwait() incorrectly interrupts the RCU thread and enters the
> force quiescent state.
First thing to mention is, IIUC above can't happen because if
call_rcu_thread() is already waked up and reaching enter_qs(), then there
should have been, for example, a prior call_rcu1() that incremented
rcu_call_count and posted to sync_event, hence rcu_call_count cannot be 0
anymore in the call_rcu1() above, because the sub happens later:
call_rcu_thread:
... sees rcu_call_count > 0, quit loop ...
...
enter_qs(sleep);
qatomic_sub(&rcu_call_count, n); <-------------------------
...
That means the concurrent call_rcu1() above will not post sem anymore
because it will only post it if rcu_call_count==0.
Besides, IMHO replacing the event with sem shouldn't change similar
behavior comparing to when using events. Because any spot that can post()
concurrently can also does qemu_event_set() concurrently... after all, we
only have a few spots resettting the event in the original patch, after the
reset a concurrent qemu_event_set() will re-activate it.
Sem does introduce possible false positives on events, but as I was trying
to justify, a generic VM boots and shutdown should need less than 1000 rcu
calls, so worst case is we loop read rcu_call_count 1000 times... I also
didn't measure how many of such call_rcu1() will see N>0 so they'll not
post() to sem at all, I believe it'll be common. So the perf overhead
should really be rare, IMHO.
Note that I was not requesting you to replace this event to sem before
merging. Frankly, I think your work is good enough to land.
However I still want to discuss a sem replacement, not only because this is
the best place to discuss it (after all, force rcu starts to introduce a
genuine second user of the event besides call_rcu1, hence the "being able
to remember more than 1" will start to make more sense than avoid resets),
it looks also cleaner to remove some new code force rcu adds.
When reviewing I found it also hard to justify qemu_event_reset() are
proper all over the places. Sem will remove that problem. So at least to
me, if you agree with using sem is better, it'll at least make me more
confident to ack the series (not that merging needs my ack.. but I do feel
more uncertain when the event needs explicit resets). OTOH, false positive
causes some loops in rcu thread which looks much safer even if happens
(however in reality, I think sem shouldn't be more than 2).
Thanks,
--
Peter Xu
