On Thu, Nov 04, 2021, Like Xu wrote:
> On 22/9/2021 8:05 am, Sean Christopherson wrote:
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 464917096e73..80ff050a7b55 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -6491,14 +6491,21 @@ struct perf_guest_info_callbacks *perf_guest_cbs;
> > int perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *cbs)
> > {
> > - perf_guest_cbs = cbs;
> > + if (WARN_ON_ONCE(perf_guest_cbs))
> > + return -EBUSY;
> > +
> > + WRITE_ONCE(perf_guest_cbs, cbs);
>
> So per Paolo's comment [1], does it help to use
> smp_store_release(perf_guest_cbs, cbs)
> or
> rcu_assign_pointer(perf_guest_cbs, cbs)
> here?
Heh, if by "help" you mean "required to prevent bad things on weakly ordered
architectures", then yes, it helps :-) If I'm interpeting Paolo's suggestion
correctly, he's pointing out that oustanding stores to the function pointers in
@cbs need to complete before assigning a non-NULL pointer to perf_guest_cbs,
otherwise a perf event handler may see a valid pointer with half-baked callbacks.
I think smp_store_release() with a comment would be appropriate, assuming my
above interpretation is correct.
> [1] https://lore.kernel.org/kvm/37afc465-c12f-01b9-f3b6-c2573e112d76@xxxxxxxxxx/
>
> > return 0;
> > }
> > EXPORT_SYMBOL_GPL(perf_register_guest_info_callbacks);
> > int perf_unregister_guest_info_callbacks(struct perf_guest_info_callbacks *cbs)
> > {
> > - perf_guest_cbs = NULL;
> > + if (WARN_ON_ONCE(perf_guest_cbs != cbs))
> > + return -EINVAL;
> > +
> > + WRITE_ONCE(perf_guest_cbs, NULL);
> > + synchronize_rcu();
> > return 0;
> > }
> > EXPORT_SYMBOL_GPL(perf_unregister_guest_info_callbacks);
> >
