On Mon, 20 Apr 2026 07:21:45 +0100,
Akihiko Odaki <[email protected]> wrote:
>
> On 2026/04/19 23:34, Marc Zyngier wrote:
> > On Sat, 18 Apr 2026 09:14:24 +0100,
> > Akihiko Odaki <[email protected]> wrote:
> >>
> >> Convert the list of PMUs to a RCU-protected list that has primitives to
> >> avoid read-side contention.
> >>
> >> Signed-off-by: Akihiko Odaki <[email protected]>
> >> ---
> >> arch/arm64/kvm/pmu-emul.c | 14 ++++++--------
> >> 1 file changed, 6 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> >> index 59ec96e09321..ef5140bbfe28 100644
> >> --- a/arch/arm64/kvm/pmu-emul.c
> >> +++ b/arch/arm64/kvm/pmu-emul.c
> >> @@ -7,9 +7,9 @@
> >> #include <linux/cpu.h>
> >> #include <linux/kvm.h>
> >> #include <linux/kvm_host.h>
> >> -#include <linux/list.h>
> >> #include <linux/perf_event.h>
> >> #include <linux/perf/arm_pmu.h>
> >> +#include <linux/rculist.h>
> >> #include <linux/uaccess.h>
> >> #include <asm/kvm_emulate.h>
> >> #include <kvm/arm_pmu.h>
> >> @@ -26,7 +26,6 @@ static bool kvm_pmu_counter_is_enabled(struct kvm_pmc
> >> *pmc);
> >> bool kvm_supports_guest_pmuv3(void)
> >> {
> >> - guard(mutex)(&arm_pmus_lock);
> >> return !list_empty(&arm_pmus);
> >
> > Please read include/linux/rculist.h and the discussion about the
> > interaction of list_empty() with RCU-protected lists. How about using
> > list_first_or_null_rcu() for peace of mind?
>
> list_first_or_null_rcu() is useful to replace a sequence of
> list_empty() and list_first_entry() that is protected by a lock, but
> this function instead requires the invariant that nobody deletes an
> element from the list, and list_first_or_null_rcu() does not allow
> removing the requirement.
>
> The header file says:
> > Where are list_empty_rcu() and list_first_entry_rcu()?
> >
> > They do not exist because they would lead to subtle race conditions:
> >
> > if (!list_empty_rcu(mylist)) {
> > struct foo *bar = list_first_entry_rcu(mylist, struct foo,
> > list_member);
> > do_something(bar);
> > }
> >
> > The list might be non-empty when list_empty_rcu() checks it, but it
> > might have become empty by the time that list_first_entry_rcu()
> > rereads the ->next pointer, which would result in a SEGV.
> >
> > When not using RCU, it is OK for list_first_entry() to re-read that
> > pointer because both functions should be protected by some lock that
> > blocks writers.
> >
> > When using RCU, list_empty() uses READ_ONCE() to fetch the
> > RCU-protected ->next pointer and then compares it to the address of
> > the list head. However, it neither dereferences this pointer nor
> > provides this pointer to its caller. Thus, READ_ONCE() suffices
> > (that is, rcu_dereference() is not needed), which means that
> > list_empty() can be used anywhere you would want to use
> > list_empty_rcu(). Just don't expect anything useful to happen if you
> > do a subsequent lockless call to list_first_entry_rcu()!!!
> >
> > See list_first_or_null_rcu for an alternative.
>
> However, kvm_supports_guest_pmuv3() locked a mutex when calling
> list_empty() and unlocked it immediately after that, instead of
> re-reading list_first_entry(). This construct inherently had a race
> condition with code that deletes an element; when the caller of
> kvm_supports_guest_pmuv3() decides to enable guest PMUv3, the host PMU
> may have been gone. But it was still safe because no one deletes an
> element.
>
> The same logic also applies when using RCU. As the comment says, we
> can use list_empty() instead of the hypothetical list_empty_rcu()
> macro because we don't expect it to magically enable something like
> list_first_entry_rcu(). This function instead keep relying on the fact
> that no one deletes an element of the list.
And that's exactly the sort of thing I am trying to plan for. *Should*
we introduce a way to remove PMUs from the list, this predicate
becomes unsafe.
So I want at least a comment explaining this to the unsuspecting
reader, as this is rather subtle.
M.
--
Without deviation from the norm, progress is not possible.
