Re: [PATCH v3 04/16] perf: Stop pretending that perf can handle multiple guest callbacks

Paolo Bonzini
2021-09-22T06:25:53Z

On 22/09/21 02:05, Sean Christopherson wrote:
> Drop the 'int' return value from the perf (un)register callbacks helpers
> and stop pretending perf can support multiple callbacks. The 'int'
> returns are not future proofing anything as none of the callers take
> action on an error. It's also not obvious that there will ever be
> co-tenant hypervisors, and if there are, that allowing multiple callbacks
> to be registered is desirable or even correct.
>
> Opportunistically rename callbacks=>cbs in the affected declarations to
> match their definitions.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> arch/arm64/include/asm/kvm_host.h | 4 ++--
> arch/arm64/kvm/perf.c | 8 ++++----
> include/linux/perf_event.h | 12 ++++++------
> kernel/events/core.c | 16 ++++------------
> 4 files changed, 16 insertions(+), 24 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 41911585ae0c..ed940aec89e0 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -670,8 +670,8 @@ unsigned long kvm_mmio_read_buf(const void *buf, unsigned int len);
> int kvm_handle_mmio_return(struct kvm_vcpu *vcpu);
> int io_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa);
>
> -int kvm_perf_init(void);
> -int kvm_perf_teardown(void);
> +void kvm_perf_init(void);
> +void kvm_perf_teardown(void);
>
> long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu);
> gpa_t kvm_init_stolen_time(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/kvm/perf.c b/arch/arm64/kvm/perf.c
> index 151c31fb9860..c37c0cf1bfe9 100644
> --- a/arch/arm64/kvm/perf.c
> +++ b/arch/arm64/kvm/perf.c
> @@ -48,15 +48,15 @@ static struct perf_guest_info_callbacks kvm_guest_cbs = {
> .get_guest_ip = kvm_get_guest_ip,
> };
>
> -int kvm_perf_init(void)
> +void kvm_perf_init(void)
> {
> if (kvm_pmu_probe_pmuver() != 0xf && !is_protected_kvm_enabled())
> static_branch_enable(&kvm_arm_pmu_available);
>
> - return perf_register_guest_info_callbacks(&kvm_guest_cbs);
> + perf_register_guest_info_callbacks(&kvm_guest_cbs);
> }
>
> -int kvm_perf_teardown(void)
> +void kvm_perf_teardown(void)
> {
> - return perf_unregister_guest_info_callbacks(&kvm_guest_cbs);
> + perf_unregister_guest_info_callbacks(&kvm_guest_cbs);
> }
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 6b0405e578c1..317d4658afe9 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -1245,8 +1245,8 @@ static inline struct perf_guest_info_callbacks *perf_get_guest_cbs(void)
> /* Prevent reloading between a !NULL check and dereferences. */
> return READ_ONCE(perf_guest_cbs);
> }
> -extern int perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *callbacks);
> -extern int perf_unregister_guest_info_callbacks(struct perf_guest_info_callbacks *callbacks);
> +extern void perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *cbs);
> +extern void perf_unregister_guest_info_callbacks(struct perf_guest_info_callbacks *cbs);
>
> extern void perf_event_exec(void);
> extern void perf_event_comm(struct task_struct *tsk, bool exec);
> @@ -1489,10 +1489,10 @@ perf_sw_event(u32 event_id, u64 nr, struct pt_regs *regs, u64 addr) { }
> static inline void
> perf_bp_event(struct perf_event *event, void *data) { }
>
> -static inline int perf_register_guest_info_callbacks
> -(struct perf_guest_info_callbacks *callbacks) { return 0; }
> -static inline int perf_unregister_guest_info_callbacks
> -(struct perf_guest_info_callbacks *callbacks) { return 0; }
> +static inline void perf_register_guest_info_callbacks
> +(struct perf_guest_info_callbacks *cbs) { }
> +static inline void perf_unregister_guest_info_callbacks
> +(struct perf_guest_info_callbacks *cbs) { }
>
> static inline void perf_event_mmap(struct vm_area_struct *vma) { }
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 80ff050a7b55..d90a43572400 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6482,31 +6482,23 @@ static void perf_pending_event(struct irq_work *entry)
> perf_swevent_put_recursion_context(rctx);
> }
>
> -/*
> - * We assume there is only KVM supporting the callbacks.
> - * Later on, we might change it to a list if there is
> - * another virtualization implementation supporting the callbacks.
> - */
> struct perf_guest_info_callbacks *perf_guest_cbs;
> -
> -int perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *cbs)
> +void perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *cbs)
> {
> if (WARN_ON_ONCE(perf_guest_cbs))
> - return -EBUSY;
> + return;
>
> WRITE_ONCE(perf_guest_cbs, cbs);

Maybe you want a smp_store_release or rcu_assign_pointer here?

> - return 0;
> }
> EXPORT_SYMBOL_GPL(perf_register_guest_info_callbacks);
>
> -int perf_unregister_guest_info_callbacks(struct perf_guest_info_callbacks *cbs)
> +void perf_unregister_guest_info_callbacks(struct perf_guest_info_callbacks *cbs)
> {
> if (WARN_ON_ONCE(perf_guest_cbs != cbs))
> - return -EINVAL;
> + return;
>
> WRITE_ONCE(perf_guest_cbs, NULL);
> synchronize_rcu();
> - return 0;
> }
> EXPORT_SYMBOL_GPL(perf_unregister_guest_info_callbacks);
>
>

Apart from the above,

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