| Message ID | 20240801045907.4010984-15-mizhang@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Mediated Passthrough vPMU 3.0 for x86 | expand |
On 8/1/2024 10:28 AM, Mingwei Zhang wrote: > From: Kan Liang <kan.liang@linux.intel.com> > > There will be a dedicated interrupt vector for guests on some platforms, > e.g., Intel. Add an interface to switch the interrupt vector while > entering/exiting a guest. > > When PMI switch into a new guest vector, guest_lvtpc value need to be > reflected onto HW, e,g., guest clear PMI mask bit, the HW PMI mask > bit should be cleared also, then PMI can be generated continuously > for guest. So guest_lvtpc parameter is added into perf_guest_enter() > and switch_interrupt(). > > At switch_interrupt(), the target pmu with PASSTHROUGH cap should > be found. Since only one passthrough pmu is supported, we keep the > implementation simply by tracking the pmu as a global variable. > > Signed-off-by: Kan Liang <kan.liang@linux.intel.com> > > [Simplify the commit with removal of srcu lock/unlock since only one pmu is > supported.] > > Signed-off-by: Mingwei Zhang <mizhang@google.com> > --- > include/linux/perf_event.h | 9 +++++++-- > kernel/events/core.c | 36 ++++++++++++++++++++++++++++++++++-- > 2 files changed, 41 insertions(+), 4 deletions(-) > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index 75773f9890cc..aeb08f78f539 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -541,6 +541,11 @@ struct pmu { > * Check period value for PERF_EVENT_IOC_PERIOD ioctl. > */ > int (*check_period) (struct perf_event *event, u64 value); /* optional */ > + > + /* > + * Switch the interrupt vectors, e.g., guest enter/exit. > + */ > + void (*switch_interrupt) (bool enter, u32 guest_lvtpc); /* optional */ > }; > > enum perf_addr_filter_action_t { > @@ -1738,7 +1743,7 @@ extern int perf_event_period(struct perf_event *event, u64 value); > extern u64 perf_event_pause(struct perf_event *event, bool reset); > int perf_get_mediated_pmu(void); > void perf_put_mediated_pmu(void); > -void perf_guest_enter(void); > +void perf_guest_enter(u32 guest_lvtpc); > void perf_guest_exit(void); > #else /* !CONFIG_PERF_EVENTS: */ > static inline void * > @@ -1833,7 +1838,7 @@ static inline int perf_get_mediated_pmu(void) > } > > static inline void perf_put_mediated_pmu(void) { } > -static inline void perf_guest_enter(void) { } > +static inline void perf_guest_enter(u32 guest_lvtpc) { } > static inline void perf_guest_exit(void) { } > #endif > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 57ff737b922b..047ca5748ee2 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -422,6 +422,7 @@ static inline bool is_include_guest_event(struct perf_event *event) > > static LIST_HEAD(pmus); > static DEFINE_MUTEX(pmus_lock); > +static struct pmu *passthru_pmu; > static struct srcu_struct pmus_srcu; > static cpumask_var_t perf_online_mask; > static struct kmem_cache *perf_event_cache; > @@ -5941,8 +5942,21 @@ void perf_put_mediated_pmu(void) > } > EXPORT_SYMBOL_GPL(perf_put_mediated_pmu); > > +static void perf_switch_interrupt(bool enter, u32 guest_lvtpc) > +{ > + /* Mediated passthrough PMU should have PASSTHROUGH_VPMU cap. */ > + if (!passthru_pmu) > + return; > + > + if (passthru_pmu->switch_interrupt && > + try_module_get(passthru_pmu->module)) { > + passthru_pmu->switch_interrupt(enter, guest_lvtpc); > + module_put(passthru_pmu->module); > + } > +} > + > /* When entering a guest, schedule out all exclude_guest events. */ > -void perf_guest_enter(void) > +void perf_guest_enter(u32 guest_lvtpc) > { > struct perf_cpu_context *cpuctx = this_cpu_ptr(&perf_cpu_context); > > @@ -5962,6 +5976,8 @@ void perf_guest_enter(void) > perf_ctx_enable(cpuctx->task_ctx, EVENT_GUEST); > } > > + perf_switch_interrupt(true, guest_lvtpc); > + > __this_cpu_write(perf_in_guest, true); > > unlock: > @@ -5980,6 +5996,8 @@ void perf_guest_exit(void) > if (WARN_ON_ONCE(!__this_cpu_read(perf_in_guest))) > goto unlock; > > + perf_switch_interrupt(false, 0); > + > perf_ctx_disable(&cpuctx->ctx, EVENT_GUEST); > ctx_sched_in(&cpuctx->ctx, EVENT_GUEST); > perf_ctx_enable(&cpuctx->ctx, EVENT_GUEST); > @@ -11842,7 +11860,21 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type) > if (!pmu->event_idx) > pmu->event_idx = perf_event_idx_default; > > - list_add_rcu(&pmu->entry, &pmus); > + /* > + * Initialize passthru_pmu with the core pmu that has > + * PERF_PMU_CAP_PASSTHROUGH_VPMU capability. > + */ > + if (pmu->capabilities & PERF_PMU_CAP_PASSTHROUGH_VPMU) { > + if (!passthru_pmu) > + passthru_pmu = pmu; > + > + if (WARN_ONCE(passthru_pmu != pmu, "Only one passthrough PMU is supported\n")) { > + ret = -EINVAL; > + goto free_dev; > + } > + } Our intention is to virtualize IBS PMUs (Op and Fetch) using the same framework. However, if IBS PMUs are also using the PERF_PMU_CAP_PASSTHROUGH_VPMU capability, IBS PMU registration fails at this point because the Core PMU is already registered with PERF_PMU_CAP_PASSTHROUGH_VPMU. > + > + list_add_tail_rcu(&pmu->entry, &pmus); > atomic_set(&pmu->exclusive_cnt, 0); > ret = 0; > unlock:
On 2024-09-19 2:02 a.m., Manali Shukla wrote: > On 8/1/2024 10:28 AM, Mingwei Zhang wrote: >> From: Kan Liang <kan.liang@linux.intel.com> >> >> There will be a dedicated interrupt vector for guests on some platforms, >> e.g., Intel. Add an interface to switch the interrupt vector while >> entering/exiting a guest. >> >> When PMI switch into a new guest vector, guest_lvtpc value need to be >> reflected onto HW, e,g., guest clear PMI mask bit, the HW PMI mask >> bit should be cleared also, then PMI can be generated continuously >> for guest. So guest_lvtpc parameter is added into perf_guest_enter() >> and switch_interrupt(). >> >> At switch_interrupt(), the target pmu with PASSTHROUGH cap should >> be found. Since only one passthrough pmu is supported, we keep the >> implementation simply by tracking the pmu as a global variable. >> >> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> >> >> [Simplify the commit with removal of srcu lock/unlock since only one pmu is >> supported.] >> >> Signed-off-by: Mingwei Zhang <mizhang@google.com> >> --- >> include/linux/perf_event.h | 9 +++++++-- >> kernel/events/core.c | 36 ++++++++++++++++++++++++++++++++++-- >> 2 files changed, 41 insertions(+), 4 deletions(-) >> >> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h >> index 75773f9890cc..aeb08f78f539 100644 >> --- a/include/linux/perf_event.h >> +++ b/include/linux/perf_event.h >> @@ -541,6 +541,11 @@ struct pmu { >> * Check period value for PERF_EVENT_IOC_PERIOD ioctl. >> */ >> int (*check_period) (struct perf_event *event, u64 value); /* optional */ >> + >> + /* >> + * Switch the interrupt vectors, e.g., guest enter/exit. >> + */ >> + void (*switch_interrupt) (bool enter, u32 guest_lvtpc); /* optional */ >> }; >> >> enum perf_addr_filter_action_t { >> @@ -1738,7 +1743,7 @@ extern int perf_event_period(struct perf_event *event, u64 value); >> extern u64 perf_event_pause(struct perf_event *event, bool reset); >> int perf_get_mediated_pmu(void); >> void perf_put_mediated_pmu(void); >> -void perf_guest_enter(void); >> +void perf_guest_enter(u32 guest_lvtpc); >> void perf_guest_exit(void); >> #else /* !CONFIG_PERF_EVENTS: */ >> static inline void * >> @@ -1833,7 +1838,7 @@ static inline int perf_get_mediated_pmu(void) >> } >> >> static inline void perf_put_mediated_pmu(void) { } >> -static inline void perf_guest_enter(void) { } >> +static inline void perf_guest_enter(u32 guest_lvtpc) { } >> static inline void perf_guest_exit(void) { } >> #endif >> >> diff --git a/kernel/events/core.c b/kernel/events/core.c >> index 57ff737b922b..047ca5748ee2 100644 >> --- a/kernel/events/core.c >> +++ b/kernel/events/core.c >> @@ -422,6 +422,7 @@ static inline bool is_include_guest_event(struct perf_event *event) >> >> static LIST_HEAD(pmus); >> static DEFINE_MUTEX(pmus_lock); >> +static struct pmu *passthru_pmu; >> static struct srcu_struct pmus_srcu; >> static cpumask_var_t perf_online_mask; >> static struct kmem_cache *perf_event_cache; >> @@ -5941,8 +5942,21 @@ void perf_put_mediated_pmu(void) >> } >> EXPORT_SYMBOL_GPL(perf_put_mediated_pmu); >> >> +static void perf_switch_interrupt(bool enter, u32 guest_lvtpc) >> +{ >> + /* Mediated passthrough PMU should have PASSTHROUGH_VPMU cap. */ >> + if (!passthru_pmu) >> + return; >> + >> + if (passthru_pmu->switch_interrupt && >> + try_module_get(passthru_pmu->module)) { >> + passthru_pmu->switch_interrupt(enter, guest_lvtpc); >> + module_put(passthru_pmu->module); >> + } >> +} >> + >> /* When entering a guest, schedule out all exclude_guest events. */ >> -void perf_guest_enter(void) >> +void perf_guest_enter(u32 guest_lvtpc) >> { >> struct perf_cpu_context *cpuctx = this_cpu_ptr(&perf_cpu_context); >> >> @@ -5962,6 +5976,8 @@ void perf_guest_enter(void) >> perf_ctx_enable(cpuctx->task_ctx, EVENT_GUEST); >> } >> >> + perf_switch_interrupt(true, guest_lvtpc); >> + >> __this_cpu_write(perf_in_guest, true); >> >> unlock: >> @@ -5980,6 +5996,8 @@ void perf_guest_exit(void) >> if (WARN_ON_ONCE(!__this_cpu_read(perf_in_guest))) >> goto unlock; >> >> + perf_switch_interrupt(false, 0); >> + >> perf_ctx_disable(&cpuctx->ctx, EVENT_GUEST); >> ctx_sched_in(&cpuctx->ctx, EVENT_GUEST); >> perf_ctx_enable(&cpuctx->ctx, EVENT_GUEST); >> @@ -11842,7 +11860,21 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type) >> if (!pmu->event_idx) >> pmu->event_idx = perf_event_idx_default; >> >> - list_add_rcu(&pmu->entry, &pmus); >> + /* >> + * Initialize passthru_pmu with the core pmu that has >> + * PERF_PMU_CAP_PASSTHROUGH_VPMU capability. >> + */ >> + if (pmu->capabilities & PERF_PMU_CAP_PASSTHROUGH_VPMU) { >> + if (!passthru_pmu) >> + passthru_pmu = pmu; >> + >> + if (WARN_ONCE(passthru_pmu != pmu, "Only one passthrough PMU is supported\n")) { >> + ret = -EINVAL; >> + goto free_dev; >> + } >> + } > > > Our intention is to virtualize IBS PMUs (Op and Fetch) using the same framework. However, > if IBS PMUs are also using the PERF_PMU_CAP_PASSTHROUGH_VPMU capability, IBS PMU registration > fails at this point because the Core PMU is already registered with PERF_PMU_CAP_PASSTHROUGH_VPMU. > The original implementation doesn't limit the number of PMUs with PERF_PMU_CAP_PASSTHROUGH_VPMU. But at that time, we could not find a case of more than one PMU with the flag. After several debates, the patch was simplified only to support one PMU with the flag. It should not be hard to change it back. Thanks, Kan >> + >> + list_add_tail_rcu(&pmu->entry, &pmus); >> atomic_set(&pmu->exclusive_cnt, 0); >> ret = 0; >> unlock: > >
On 9/19/2024 6:30 PM, Liang, Kan wrote: > > > On 2024-09-19 2:02 a.m., Manali Shukla wrote: >> On 8/1/2024 10:28 AM, Mingwei Zhang wrote: >>> From: Kan Liang <kan.liang@linux.intel.com> >>> >>> There will be a dedicated interrupt vector for guests on some platforms, >>> e.g., Intel. Add an interface to switch the interrupt vector while >>> entering/exiting a guest. >>> >>> When PMI switch into a new guest vector, guest_lvtpc value need to be >>> reflected onto HW, e,g., guest clear PMI mask bit, the HW PMI mask >>> bit should be cleared also, then PMI can be generated continuously >>> for guest. So guest_lvtpc parameter is added into perf_guest_enter() >>> and switch_interrupt(). >>> >>> At switch_interrupt(), the target pmu with PASSTHROUGH cap should >>> be found. Since only one passthrough pmu is supported, we keep the >>> implementation simply by tracking the pmu as a global variable. >>> >>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> >>> >>> [Simplify the commit with removal of srcu lock/unlock since only one pmu is >>> supported.] >>> >>> Signed-off-by: Mingwei Zhang <mizhang@google.com> >>> --- >>> include/linux/perf_event.h | 9 +++++++-- >>> kernel/events/core.c | 36 ++++++++++++++++++++++++++++++++++-- >>> 2 files changed, 41 insertions(+), 4 deletions(-) >>> >>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h >>> index 75773f9890cc..aeb08f78f539 100644 >>> --- a/include/linux/perf_event.h >>> +++ b/include/linux/perf_event.h >>> @@ -541,6 +541,11 @@ struct pmu { >>> * Check period value for PERF_EVENT_IOC_PERIOD ioctl. >>> */ >>> int (*check_period) (struct perf_event *event, u64 value); /* optional */ >>> + >>> + /* >>> + * Switch the interrupt vectors, e.g., guest enter/exit. >>> + */ >>> + void (*switch_interrupt) (bool enter, u32 guest_lvtpc); /* optional */ >>> }; >>> >>> enum perf_addr_filter_action_t { >>> @@ -1738,7 +1743,7 @@ extern int perf_event_period(struct perf_event *event, u64 value); >>> extern u64 perf_event_pause(struct perf_event *event, bool reset); >>> int perf_get_mediated_pmu(void); >>> void perf_put_mediated_pmu(void); >>> -void perf_guest_enter(void); >>> +void perf_guest_enter(u32 guest_lvtpc); >>> void perf_guest_exit(void); >>> #else /* !CONFIG_PERF_EVENTS: */ >>> static inline void * >>> @@ -1833,7 +1838,7 @@ static inline int perf_get_mediated_pmu(void) >>> } >>> >>> static inline void perf_put_mediated_pmu(void) { } >>> -static inline void perf_guest_enter(void) { } >>> +static inline void perf_guest_enter(u32 guest_lvtpc) { } >>> static inline void perf_guest_exit(void) { } >>> #endif >>> >>> diff --git a/kernel/events/core.c b/kernel/events/core.c >>> index 57ff737b922b..047ca5748ee2 100644 >>> --- a/kernel/events/core.c >>> +++ b/kernel/events/core.c >>> @@ -422,6 +422,7 @@ static inline bool is_include_guest_event(struct perf_event *event) >>> >>> static LIST_HEAD(pmus); >>> static DEFINE_MUTEX(pmus_lock); >>> +static struct pmu *passthru_pmu; >>> static struct srcu_struct pmus_srcu; >>> static cpumask_var_t perf_online_mask; >>> static struct kmem_cache *perf_event_cache; >>> @@ -5941,8 +5942,21 @@ void perf_put_mediated_pmu(void) >>> } >>> EXPORT_SYMBOL_GPL(perf_put_mediated_pmu); >>> >>> +static void perf_switch_interrupt(bool enter, u32 guest_lvtpc) >>> +{ >>> + /* Mediated passthrough PMU should have PASSTHROUGH_VPMU cap. */ >>> + if (!passthru_pmu) >>> + return; >>> + >>> + if (passthru_pmu->switch_interrupt && >>> + try_module_get(passthru_pmu->module)) { >>> + passthru_pmu->switch_interrupt(enter, guest_lvtpc); >>> + module_put(passthru_pmu->module); >>> + } >>> +} >>> + >>> /* When entering a guest, schedule out all exclude_guest events. */ >>> -void perf_guest_enter(void) >>> +void perf_guest_enter(u32 guest_lvtpc) >>> { >>> struct perf_cpu_context *cpuctx = this_cpu_ptr(&perf_cpu_context); >>> >>> @@ -5962,6 +5976,8 @@ void perf_guest_enter(void) >>> perf_ctx_enable(cpuctx->task_ctx, EVENT_GUEST); >>> } >>> >>> + perf_switch_interrupt(true, guest_lvtpc); >>> + >>> __this_cpu_write(perf_in_guest, true); >>> >>> unlock: >>> @@ -5980,6 +5996,8 @@ void perf_guest_exit(void) >>> if (WARN_ON_ONCE(!__this_cpu_read(perf_in_guest))) >>> goto unlock; >>> >>> + perf_switch_interrupt(false, 0); >>> + >>> perf_ctx_disable(&cpuctx->ctx, EVENT_GUEST); >>> ctx_sched_in(&cpuctx->ctx, EVENT_GUEST); >>> perf_ctx_enable(&cpuctx->ctx, EVENT_GUEST); >>> @@ -11842,7 +11860,21 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type) >>> if (!pmu->event_idx) >>> pmu->event_idx = perf_event_idx_default; >>> >>> - list_add_rcu(&pmu->entry, &pmus); >>> + /* >>> + * Initialize passthru_pmu with the core pmu that has >>> + * PERF_PMU_CAP_PASSTHROUGH_VPMU capability. >>> + */ >>> + if (pmu->capabilities & PERF_PMU_CAP_PASSTHROUGH_VPMU) { >>> + if (!passthru_pmu) >>> + passthru_pmu = pmu; >>> + >>> + if (WARN_ONCE(passthru_pmu != pmu, "Only one passthrough PMU is supported\n")) { >>> + ret = -EINVAL; >>> + goto free_dev; >>> + } >>> + } >> >> >> Our intention is to virtualize IBS PMUs (Op and Fetch) using the same framework. However, >> if IBS PMUs are also using the PERF_PMU_CAP_PASSTHROUGH_VPMU capability, IBS PMU registration >> fails at this point because the Core PMU is already registered with PERF_PMU_CAP_PASSTHROUGH_VPMU. >> > > The original implementation doesn't limit the number of PMUs with > PERF_PMU_CAP_PASSTHROUGH_VPMU. But at that time, we could not find a > case of more than one PMU with the flag. After several debates, the > patch was simplified only to support one PMU with the flag. > It should not be hard to change it back. > > Thanks, > Kan > Yes, we have a use case to use mediated passthrough vPMU framework for IBS virtualization. So, we will need it. - Manali >>> + >>> + list_add_tail_rcu(&pmu->entry, &pmus); >>> atomic_set(&pmu->exclusive_cnt, 0); >>> ret = 0; >>> unlock: >> >>
On Fri, Sep 20, 2024 at 7:09 AM Manali Shukla <manali.shukla@amd.com> wrote: > > On 9/19/2024 6:30 PM, Liang, Kan wrote: > > > > > > On 2024-09-19 2:02 a.m., Manali Shukla wrote: > >> On 8/1/2024 10:28 AM, Mingwei Zhang wrote: > >>> From: Kan Liang <kan.liang@linux.intel.com> > >>> > >>> There will be a dedicated interrupt vector for guests on some platforms, > >>> e.g., Intel. Add an interface to switch the interrupt vector while > >>> entering/exiting a guest. > >>> > >>> When PMI switch into a new guest vector, guest_lvtpc value need to be > >>> reflected onto HW, e,g., guest clear PMI mask bit, the HW PMI mask > >>> bit should be cleared also, then PMI can be generated continuously > >>> for guest. So guest_lvtpc parameter is added into perf_guest_enter() > >>> and switch_interrupt(). > >>> > >>> At switch_interrupt(), the target pmu with PASSTHROUGH cap should > >>> be found. Since only one passthrough pmu is supported, we keep the > >>> implementation simply by tracking the pmu as a global variable. > >>> > >>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> > >>> > >>> [Simplify the commit with removal of srcu lock/unlock since only one pmu is > >>> supported.] > >>> > >>> Signed-off-by: Mingwei Zhang <mizhang@google.com> > >>> --- > >>> include/linux/perf_event.h | 9 +++++++-- > >>> kernel/events/core.c | 36 ++++++++++++++++++++++++++++++++++-- > >>> 2 files changed, 41 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > >>> index 75773f9890cc..aeb08f78f539 100644 > >>> --- a/include/linux/perf_event.h > >>> +++ b/include/linux/perf_event.h > >>> @@ -541,6 +541,11 @@ struct pmu { > >>> * Check period value for PERF_EVENT_IOC_PERIOD ioctl. > >>> */ > >>> int (*check_period) (struct perf_event *event, u64 value); /* optional */ > >>> + > >>> + /* > >>> + * Switch the interrupt vectors, e.g., guest enter/exit. > >>> + */ > >>> + void (*switch_interrupt) (bool enter, u32 guest_lvtpc); /* optional */ > >>> }; > >>> > >>> enum perf_addr_filter_action_t { > >>> @@ -1738,7 +1743,7 @@ extern int perf_event_period(struct perf_event *event, u64 value); > >>> extern u64 perf_event_pause(struct perf_event *event, bool reset); > >>> int perf_get_mediated_pmu(void); > >>> void perf_put_mediated_pmu(void); > >>> -void perf_guest_enter(void); > >>> +void perf_guest_enter(u32 guest_lvtpc); > >>> void perf_guest_exit(void); > >>> #else /* !CONFIG_PERF_EVENTS: */ > >>> static inline void * > >>> @@ -1833,7 +1838,7 @@ static inline int perf_get_mediated_pmu(void) > >>> } > >>> > >>> static inline void perf_put_mediated_pmu(void) { } > >>> -static inline void perf_guest_enter(void) { } > >>> +static inline void perf_guest_enter(u32 guest_lvtpc) { } > >>> static inline void perf_guest_exit(void) { } > >>> #endif > >>> > >>> diff --git a/kernel/events/core.c b/kernel/events/core.c > >>> index 57ff737b922b..047ca5748ee2 100644 > >>> --- a/kernel/events/core.c > >>> +++ b/kernel/events/core.c > >>> @@ -422,6 +422,7 @@ static inline bool is_include_guest_event(struct perf_event *event) > >>> > >>> static LIST_HEAD(pmus); > >>> static DEFINE_MUTEX(pmus_lock); > >>> +static struct pmu *passthru_pmu; > >>> static struct srcu_struct pmus_srcu; > >>> static cpumask_var_t perf_online_mask; > >>> static struct kmem_cache *perf_event_cache; > >>> @@ -5941,8 +5942,21 @@ void perf_put_mediated_pmu(void) > >>> } > >>> EXPORT_SYMBOL_GPL(perf_put_mediated_pmu); > >>> > >>> +static void perf_switch_interrupt(bool enter, u32 guest_lvtpc) > >>> +{ > >>> + /* Mediated passthrough PMU should have PASSTHROUGH_VPMU cap. */ > >>> + if (!passthru_pmu) > >>> + return; > >>> + > >>> + if (passthru_pmu->switch_interrupt && > >>> + try_module_get(passthru_pmu->module)) { > >>> + passthru_pmu->switch_interrupt(enter, guest_lvtpc); > >>> + module_put(passthru_pmu->module); > >>> + } > >>> +} > >>> + > >>> /* When entering a guest, schedule out all exclude_guest events. */ > >>> -void perf_guest_enter(void) > >>> +void perf_guest_enter(u32 guest_lvtpc) > >>> { > >>> struct perf_cpu_context *cpuctx = this_cpu_ptr(&perf_cpu_context); > >>> > >>> @@ -5962,6 +5976,8 @@ void perf_guest_enter(void) > >>> perf_ctx_enable(cpuctx->task_ctx, EVENT_GUEST); > >>> } > >>> > >>> + perf_switch_interrupt(true, guest_lvtpc); > >>> + > >>> __this_cpu_write(perf_in_guest, true); > >>> > >>> unlock: > >>> @@ -5980,6 +5996,8 @@ void perf_guest_exit(void) > >>> if (WARN_ON_ONCE(!__this_cpu_read(perf_in_guest))) > >>> goto unlock; > >>> > >>> + perf_switch_interrupt(false, 0); > >>> + > >>> perf_ctx_disable(&cpuctx->ctx, EVENT_GUEST); > >>> ctx_sched_in(&cpuctx->ctx, EVENT_GUEST); > >>> perf_ctx_enable(&cpuctx->ctx, EVENT_GUEST); > >>> @@ -11842,7 +11860,21 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type) > >>> if (!pmu->event_idx) > >>> pmu->event_idx = perf_event_idx_default; > >>> > >>> - list_add_rcu(&pmu->entry, &pmus); > >>> + /* > >>> + * Initialize passthru_pmu with the core pmu that has > >>> + * PERF_PMU_CAP_PASSTHROUGH_VPMU capability. > >>> + */ > >>> + if (pmu->capabilities & PERF_PMU_CAP_PASSTHROUGH_VPMU) { > >>> + if (!passthru_pmu) > >>> + passthru_pmu = pmu; > >>> + > >>> + if (WARN_ONCE(passthru_pmu != pmu, "Only one passthrough PMU is supported\n")) { > >>> + ret = -EINVAL; > >>> + goto free_dev; > >>> + } > >>> + } > >> > >> > >> Our intention is to virtualize IBS PMUs (Op and Fetch) using the same framework. However, > >> if IBS PMUs are also using the PERF_PMU_CAP_PASSTHROUGH_VPMU capability, IBS PMU registration > >> fails at this point because the Core PMU is already registered with PERF_PMU_CAP_PASSTHROUGH_VPMU. > >> > > > > The original implementation doesn't limit the number of PMUs with > > PERF_PMU_CAP_PASSTHROUGH_VPMU. But at that time, we could not find a > > case of more than one PMU with the flag. After several debates, the > > patch was simplified only to support one PMU with the flag. > > It should not be hard to change it back. The original implementation is by design having a terrible performance overhead, ie., every PMU context switch at runtime requires a SRCU lock pair and pmu list traversal. To reduce the overhead, we put "passthrough" pmus in the front of the list and quickly exit the pmu traversal when we just pass the last "passthrough" pmu. I honestly do not like the idea because it is simply a hacky solution with high maintenance cost, but I am not the right person to make the call. So, if perf (kernel) subsystem maintainers are happy, I can withdraw from this one. My second point is: if you look at the details, the only reason why we traverse the pmu list is to check if a pmu has implemented the "switch_interrupt()" API. If so, invoke it, which will change the PMI vector from NMI to a maskable interrupt for KVM. In AMD case, I bet even if there are multiple passthrough pmus, only one requires switching the interrupt vector. Let me know if this is wrong. Thanks. -Mingwei > > > > Thanks, > > Kan > > > > Yes, we have a use case to use mediated passthrough vPMU framework for IBS virtualization. > So, we will need it. > > - Manali > > >>> + > >>> + list_add_tail_rcu(&pmu->entry, &pmus); > >>> atomic_set(&pmu->exclusive_cnt, 0); > >>> ret = 0; > >>> unlock: > >> > >> >
On 9/24/2024 12:19 AM, Mingwei Zhang wrote: > On Fri, Sep 20, 2024 at 7:09 AM Manali Shukla <manali.shukla@amd.com> wrote: >> >> On 9/19/2024 6:30 PM, Liang, Kan wrote: >>> >>> >>> On 2024-09-19 2:02 a.m., Manali Shukla wrote: >>>> On 8/1/2024 10:28 AM, Mingwei Zhang wrote: >>>>> From: Kan Liang <kan.liang@linux.intel.com> >>>>> >>>>> There will be a dedicated interrupt vector for guests on some platforms, >>>>> e.g., Intel. Add an interface to switch the interrupt vector while >>>>> entering/exiting a guest. >>>>> >>>>> When PMI switch into a new guest vector, guest_lvtpc value need to be >>>>> reflected onto HW, e,g., guest clear PMI mask bit, the HW PMI mask >>>>> bit should be cleared also, then PMI can be generated continuously >>>>> for guest. So guest_lvtpc parameter is added into perf_guest_enter() >>>>> and switch_interrupt(). >>>>> >>>>> At switch_interrupt(), the target pmu with PASSTHROUGH cap should >>>>> be found. Since only one passthrough pmu is supported, we keep the >>>>> implementation simply by tracking the pmu as a global variable. >>>>> >>>>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> >>>>> >>>>> [Simplify the commit with removal of srcu lock/unlock since only one pmu is >>>>> supported.] >>>>> >>>>> Signed-off-by: Mingwei Zhang <mizhang@google.com> >>>>> --- >>>>> include/linux/perf_event.h | 9 +++++++-- >>>>> kernel/events/core.c | 36 ++++++++++++++++++++++++++++++++++-- >>>>> 2 files changed, 41 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h >>>>> index 75773f9890cc..aeb08f78f539 100644 >>>>> --- a/include/linux/perf_event.h >>>>> +++ b/include/linux/perf_event.h >>>>> @@ -541,6 +541,11 @@ struct pmu { >>>>> * Check period value for PERF_EVENT_IOC_PERIOD ioctl. >>>>> */ >>>>> int (*check_period) (struct perf_event *event, u64 value); /* optional */ >>>>> + >>>>> + /* >>>>> + * Switch the interrupt vectors, e.g., guest enter/exit. >>>>> + */ >>>>> + void (*switch_interrupt) (bool enter, u32 guest_lvtpc); /* optional */ >>>>> }; >>>>> >>>>> enum perf_addr_filter_action_t { >>>>> @@ -1738,7 +1743,7 @@ extern int perf_event_period(struct perf_event *event, u64 value); >>>>> extern u64 perf_event_pause(struct perf_event *event, bool reset); >>>>> int perf_get_mediated_pmu(void); >>>>> void perf_put_mediated_pmu(void); >>>>> -void perf_guest_enter(void); >>>>> +void perf_guest_enter(u32 guest_lvtpc); >>>>> void perf_guest_exit(void); >>>>> #else /* !CONFIG_PERF_EVENTS: */ >>>>> static inline void * >>>>> @@ -1833,7 +1838,7 @@ static inline int perf_get_mediated_pmu(void) >>>>> } >>>>> >>>>> static inline void perf_put_mediated_pmu(void) { } >>>>> -static inline void perf_guest_enter(void) { } >>>>> +static inline void perf_guest_enter(u32 guest_lvtpc) { } >>>>> static inline void perf_guest_exit(void) { } >>>>> #endif >>>>> >>>>> diff --git a/kernel/events/core.c b/kernel/events/core.c >>>>> index 57ff737b922b..047ca5748ee2 100644 >>>>> --- a/kernel/events/core.c >>>>> +++ b/kernel/events/core.c >>>>> @@ -422,6 +422,7 @@ static inline bool is_include_guest_event(struct perf_event *event) >>>>> >>>>> static LIST_HEAD(pmus); >>>>> static DEFINE_MUTEX(pmus_lock); >>>>> +static struct pmu *passthru_pmu; >>>>> static struct srcu_struct pmus_srcu; >>>>> static cpumask_var_t perf_online_mask; >>>>> static struct kmem_cache *perf_event_cache; >>>>> @@ -5941,8 +5942,21 @@ void perf_put_mediated_pmu(void) >>>>> } >>>>> EXPORT_SYMBOL_GPL(perf_put_mediated_pmu); >>>>> >>>>> +static void perf_switch_interrupt(bool enter, u32 guest_lvtpc) >>>>> +{ >>>>> + /* Mediated passthrough PMU should have PASSTHROUGH_VPMU cap. */ >>>>> + if (!passthru_pmu) >>>>> + return; >>>>> + >>>>> + if (passthru_pmu->switch_interrupt && >>>>> + try_module_get(passthru_pmu->module)) { >>>>> + passthru_pmu->switch_interrupt(enter, guest_lvtpc); >>>>> + module_put(passthru_pmu->module); >>>>> + } >>>>> +} >>>>> + >>>>> /* When entering a guest, schedule out all exclude_guest events. */ >>>>> -void perf_guest_enter(void) >>>>> +void perf_guest_enter(u32 guest_lvtpc) >>>>> { >>>>> struct perf_cpu_context *cpuctx = this_cpu_ptr(&perf_cpu_context); >>>>> >>>>> @@ -5962,6 +5976,8 @@ void perf_guest_enter(void) >>>>> perf_ctx_enable(cpuctx->task_ctx, EVENT_GUEST); >>>>> } >>>>> >>>>> + perf_switch_interrupt(true, guest_lvtpc); >>>>> + >>>>> __this_cpu_write(perf_in_guest, true); >>>>> >>>>> unlock: >>>>> @@ -5980,6 +5996,8 @@ void perf_guest_exit(void) >>>>> if (WARN_ON_ONCE(!__this_cpu_read(perf_in_guest))) >>>>> goto unlock; >>>>> >>>>> + perf_switch_interrupt(false, 0); >>>>> + >>>>> perf_ctx_disable(&cpuctx->ctx, EVENT_GUEST); >>>>> ctx_sched_in(&cpuctx->ctx, EVENT_GUEST); >>>>> perf_ctx_enable(&cpuctx->ctx, EVENT_GUEST); >>>>> @@ -11842,7 +11860,21 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type) >>>>> if (!pmu->event_idx) >>>>> pmu->event_idx = perf_event_idx_default; >>>>> >>>>> - list_add_rcu(&pmu->entry, &pmus); >>>>> + /* >>>>> + * Initialize passthru_pmu with the core pmu that has >>>>> + * PERF_PMU_CAP_PASSTHROUGH_VPMU capability. >>>>> + */ >>>>> + if (pmu->capabilities & PERF_PMU_CAP_PASSTHROUGH_VPMU) { >>>>> + if (!passthru_pmu) >>>>> + passthru_pmu = pmu; >>>>> + >>>>> + if (WARN_ONCE(passthru_pmu != pmu, "Only one passthrough PMU is supported\n")) { >>>>> + ret = -EINVAL; >>>>> + goto free_dev; >>>>> + } >>>>> + } >>>> >>>> >>>> Our intention is to virtualize IBS PMUs (Op and Fetch) using the same framework. However, >>>> if IBS PMUs are also using the PERF_PMU_CAP_PASSTHROUGH_VPMU capability, IBS PMU registration >>>> fails at this point because the Core PMU is already registered with PERF_PMU_CAP_PASSTHROUGH_VPMU. >>>> >>> >>> The original implementation doesn't limit the number of PMUs with >>> PERF_PMU_CAP_PASSTHROUGH_VPMU. But at that time, we could not find a >>> case of more than one PMU with the flag. After several debates, the >>> patch was simplified only to support one PMU with the flag. >>> It should not be hard to change it back. > > The original implementation is by design having a terrible performance > overhead, ie., every PMU context switch at runtime requires a SRCU > lock pair and pmu list traversal. To reduce the overhead, we put > "passthrough" pmus in the front of the list and quickly exit the pmu > traversal when we just pass the last "passthrough" pmu. > > I honestly do not like the idea because it is simply a hacky solution > with high maintenance cost, but I am not the right person to make the > call. So, if perf (kernel) subsystem maintainers are happy, I can > withdraw from this one. > > My second point is: if you look at the details, the only reason why we > traverse the pmu list is to check if a pmu has implemented the > "switch_interrupt()" API. If so, invoke it, which will change the PMI > vector from NMI to a maskable interrupt for KVM. In AMD case, I bet > even if there are multiple passthrough pmus, only one requires > switching the interrupt vector. Let me know if this is wrong. > > Thanks. > -Mingwei > Yeah, That is correct. Currently, switching of the interrupt vector is needed only by one passthrough PMU for AMD. It is not required for IBS virtualization and PMC virtualization because VNMI is used to deliver interrupt in both cases. With the mediated passthrough VPMU enabled, all exclude_guest events for PMUs with PERF_PMU_CAP_PASSTHROUGH_VPMU capability are scheduled out when entering a guest and scheduled back in upon exit. We need this functionality for IBS virtualization and PMC virtualization. However, the current design allows only one passthrough PMU, which prevents the IBS driver from loading with the PERF_PMU_CAP_PASSTHROUGH_VPMU capability. As a result, we are unable to utilize functionality of scheduling out and scheduling in exclude_guest events within the mediated passthrough VPMU framework for IBS virtualization. - Manali >>> >>> Thanks, >>> Kan >>> >> >> Yes, we have a use case to use mediated passthrough vPMU framework for IBS virtualization. >> So, we will need it. >> >> - Manali >> >>>>> + >>>>> + list_add_tail_rcu(&pmu->entry, &pmus); >>>>> atomic_set(&pmu->exclusive_cnt, 0); >>>>> ret = 0; >>>>> unlock: >>>> >>>> >>
On Thu, Aug 01, 2024 at 04:58:23AM +0000, Mingwei Zhang wrote: > @@ -5941,8 +5942,21 @@ void perf_put_mediated_pmu(void) > } > EXPORT_SYMBOL_GPL(perf_put_mediated_pmu); > > +static void perf_switch_interrupt(bool enter, u32 guest_lvtpc) > +{ > + /* Mediated passthrough PMU should have PASSTHROUGH_VPMU cap. */ > + if (!passthru_pmu) > + return; > + > + if (passthru_pmu->switch_interrupt && > + try_module_get(passthru_pmu->module)) { > + passthru_pmu->switch_interrupt(enter, guest_lvtpc); > + module_put(passthru_pmu->module); > + } > +} Should we move the whole module reference to perf_pmu_(,un}register() ? > @@ -11842,7 +11860,21 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type) > if (!pmu->event_idx) > pmu->event_idx = perf_event_idx_default; > > - list_add_rcu(&pmu->entry, &pmus); > + /* > + * Initialize passthru_pmu with the core pmu that has > + * PERF_PMU_CAP_PASSTHROUGH_VPMU capability. > + */ > + if (pmu->capabilities & PERF_PMU_CAP_PASSTHROUGH_VPMU) { > + if (!passthru_pmu) > + passthru_pmu = pmu; > + > + if (WARN_ONCE(passthru_pmu != pmu, "Only one passthrough PMU is supported\n")) { > + ret = -EINVAL; > + goto free_dev; Why impose this limit? Changelog also fails to explain this. > + } > + } > + > + list_add_tail_rcu(&pmu->entry, &pmus); > atomic_set(&pmu->exclusive_cnt, 0); > ret = 0; > unlock: > -- > 2.46.0.rc1.232.g9752f9e123-goog >
On Mon, Sep 23, 2024 at 08:49:17PM +0200, Mingwei Zhang wrote: > The original implementation is by design having a terrible performance > overhead, ie., every PMU context switch at runtime requires a SRCU > lock pair and pmu list traversal. To reduce the overhead, we put > "passthrough" pmus in the front of the list and quickly exit the pmu > traversal when we just pass the last "passthrough" pmu. What was the expensive bit? The SRCU memory barrier or the list iteration? How long is that list really?
On Thu, Aug 01, 2024 at 04:58:23AM +0000, Mingwei Zhang wrote: > From: Kan Liang <kan.liang@linux.intel.com> > > There will be a dedicated interrupt vector for guests on some platforms, > e.g., Intel. Add an interface to switch the interrupt vector while > entering/exiting a guest. > > When PMI switch into a new guest vector, guest_lvtpc value need to be > reflected onto HW, e,g., guest clear PMI mask bit, the HW PMI mask > bit should be cleared also, then PMI can be generated continuously > for guest. So guest_lvtpc parameter is added into perf_guest_enter() > and switch_interrupt(). > > At switch_interrupt(), the target pmu with PASSTHROUGH cap should > be found. Since only one passthrough pmu is supported, we keep the > implementation simply by tracking the pmu as a global variable. > > Signed-off-by: Kan Liang <kan.liang@linux.intel.com> > > [Simplify the commit with removal of srcu lock/unlock since only one pmu is > supported.] > > Signed-off-by: Mingwei Zhang <mizhang@google.com> > --- > include/linux/perf_event.h | 9 +++++++-- > kernel/events/core.c | 36 ++++++++++++++++++++++++++++++++++-- > 2 files changed, 41 insertions(+), 4 deletions(-) > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index 75773f9890cc..aeb08f78f539 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -541,6 +541,11 @@ struct pmu { > * Check period value for PERF_EVENT_IOC_PERIOD ioctl. > */ > int (*check_period) (struct perf_event *event, u64 value); /* optional */ > + > + /* > + * Switch the interrupt vectors, e.g., guest enter/exit. > + */ > + void (*switch_interrupt) (bool enter, u32 guest_lvtpc); /* optional */ > }; I'm thinking the guets_lvtpc argument shouldn't be part of the interface. That should be PMU implementation data and accessed by the method implementation.
On Thu, Aug 01, 2024 at 04:58:23AM +0000, Mingwei Zhang wrote: > @@ -5962,6 +5976,8 @@ void perf_guest_enter(void) > perf_ctx_enable(cpuctx->task_ctx, EVENT_GUEST); > } > > + perf_switch_interrupt(true, guest_lvtpc); > + > __this_cpu_write(perf_in_guest, true); > > unlock: > @@ -5980,6 +5996,8 @@ void perf_guest_exit(void) > if (WARN_ON_ONCE(!__this_cpu_read(perf_in_guest))) > goto unlock; > > + perf_switch_interrupt(false, 0); > + > perf_ctx_disable(&cpuctx->ctx, EVENT_GUEST); > ctx_sched_in(&cpuctx->ctx, EVENT_GUEST); > perf_ctx_enable(&cpuctx->ctx, EVENT_GUEST); This seems to suggest the method is named wrong, it should probably be guest_enter() or somsuch.
On 2024-10-14 7:56 a.m., Peter Zijlstra wrote: > On Thu, Aug 01, 2024 at 04:58:23AM +0000, Mingwei Zhang wrote: > >> @@ -5941,8 +5942,21 @@ void perf_put_mediated_pmu(void) >> } >> EXPORT_SYMBOL_GPL(perf_put_mediated_pmu); >> >> +static void perf_switch_interrupt(bool enter, u32 guest_lvtpc) >> +{ >> + /* Mediated passthrough PMU should have PASSTHROUGH_VPMU cap. */ >> + if (!passthru_pmu) >> + return; >> + >> + if (passthru_pmu->switch_interrupt && >> + try_module_get(passthru_pmu->module)) { >> + passthru_pmu->switch_interrupt(enter, guest_lvtpc); >> + module_put(passthru_pmu->module); >> + } >> +} > > Should we move the whole module reference to perf_pmu_(,un}register() ? A PMU module can be load/unload anytime. How should we know if the PMU module is available when the reference check is moved to perf_pmu_(,un}register()? > >> @@ -11842,7 +11860,21 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type) >> if (!pmu->event_idx) >> pmu->event_idx = perf_event_idx_default; >> >> - list_add_rcu(&pmu->entry, &pmus); >> + /* >> + * Initialize passthru_pmu with the core pmu that has >> + * PERF_PMU_CAP_PASSTHROUGH_VPMU capability. >> + */ >> + if (pmu->capabilities & PERF_PMU_CAP_PASSTHROUGH_VPMU) { >> + if (!passthru_pmu) >> + passthru_pmu = pmu; >> + >> + if (WARN_ONCE(passthru_pmu != pmu, "Only one passthrough PMU is supported\n")) { >> + ret = -EINVAL; >> + goto free_dev; > > Why impose this limit? Changelog also fails to explain this. Because the passthru_pmu is global variable. If there are two or more PMUs with the PERF_PMU_CAP_PASSTHROUGH_VPMU, the former one will be implicitly overwritten if without the check. Thanks Kan > >> + } >> + } >> + >> + list_add_tail_rcu(&pmu->entry, &pmus); >> atomic_set(&pmu->exclusive_cnt, 0); >> ret = 0; >> unlock: >> -- >> 2.46.0.rc1.232.g9752f9e123-goog >> >
On 2024-10-14 8:03 a.m., Peter Zijlstra wrote: > On Thu, Aug 01, 2024 at 04:58:23AM +0000, Mingwei Zhang wrote: >> From: Kan Liang <kan.liang@linux.intel.com> >> >> There will be a dedicated interrupt vector for guests on some platforms, >> e.g., Intel. Add an interface to switch the interrupt vector while >> entering/exiting a guest. >> >> When PMI switch into a new guest vector, guest_lvtpc value need to be >> reflected onto HW, e,g., guest clear PMI mask bit, the HW PMI mask >> bit should be cleared also, then PMI can be generated continuously >> for guest. So guest_lvtpc parameter is added into perf_guest_enter() >> and switch_interrupt(). >> >> At switch_interrupt(), the target pmu with PASSTHROUGH cap should >> be found. Since only one passthrough pmu is supported, we keep the >> implementation simply by tracking the pmu as a global variable. >> >> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> >> >> [Simplify the commit with removal of srcu lock/unlock since only one pmu is >> supported.] >> >> Signed-off-by: Mingwei Zhang <mizhang@google.com> >> --- >> include/linux/perf_event.h | 9 +++++++-- >> kernel/events/core.c | 36 ++++++++++++++++++++++++++++++++++-- >> 2 files changed, 41 insertions(+), 4 deletions(-) >> >> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h >> index 75773f9890cc..aeb08f78f539 100644 >> --- a/include/linux/perf_event.h >> +++ b/include/linux/perf_event.h >> @@ -541,6 +541,11 @@ struct pmu { >> * Check period value for PERF_EVENT_IOC_PERIOD ioctl. >> */ >> int (*check_period) (struct perf_event *event, u64 value); /* optional */ >> + >> + /* >> + * Switch the interrupt vectors, e.g., guest enter/exit. >> + */ >> + void (*switch_interrupt) (bool enter, u32 guest_lvtpc); /* optional */ >> }; > > I'm thinking the guets_lvtpc argument shouldn't be part of the > interface. That should be PMU implementation data and accessed by the > method implementation. I think the name of the perf_switch_interrupt() is too specific. Here should be to switch the guest context. The interrupt should be just part of the context. Maybe a interface as below void (*switch_guest_ctx) (bool enter, void *data); /* optional */ Thanks, Kan
On 2024-10-14 9:52 a.m., Peter Zijlstra wrote: > On Thu, Aug 01, 2024 at 04:58:23AM +0000, Mingwei Zhang wrote: >> @@ -5962,6 +5976,8 @@ void perf_guest_enter(void) >> perf_ctx_enable(cpuctx->task_ctx, EVENT_GUEST); >> } >> >> + perf_switch_interrupt(true, guest_lvtpc); >> + >> __this_cpu_write(perf_in_guest, true); >> >> unlock: >> @@ -5980,6 +5996,8 @@ void perf_guest_exit(void) >> if (WARN_ON_ONCE(!__this_cpu_read(perf_in_guest))) >> goto unlock; >> >> + perf_switch_interrupt(false, 0); >> + >> perf_ctx_disable(&cpuctx->ctx, EVENT_GUEST); >> ctx_sched_in(&cpuctx->ctx, EVENT_GUEST); >> perf_ctx_enable(&cpuctx->ctx, EVENT_GUEST); > > This seems to suggest the method is named wrong, it should probably be > guest_enter() or somsuch. > The ctx_sched_in() is to schedule in the host context after the guest_exit(). The EVENT_GUEST is to indicate the guest ctx switch. The name may brings some confusion. Maybe I can add a wrap function perf_host_enter() to include the above codes. Thanks, Kan
On 2024-10-14 7:59 a.m., Peter Zijlstra wrote: > On Mon, Sep 23, 2024 at 08:49:17PM +0200, Mingwei Zhang wrote: > >> The original implementation is by design having a terrible performance >> overhead, ie., every PMU context switch at runtime requires a SRCU >> lock pair and pmu list traversal. To reduce the overhead, we put >> "passthrough" pmus in the front of the list and quickly exit the pmu >> traversal when we just pass the last "passthrough" pmu. > > What was the expensive bit? The SRCU memory barrier or the list > iteration? How long is that list really? Both. But I don't think there is any performance data. The length of the list could vary on different platforms. For a modern server, there could be hundreds of PMUs from uncore PMUs, CXL PMUs, IOMMU PMUs, PMUs of accelerator devices and PMUs from all kinds of devices. The number could keep increasing with more and more devices supporting the PMU capability. Two methods were considered. - One is to add a global variable to track the "passthrough" pmu. The idea assumes that there is only one "passthrough" pmu that requires the switch, and the situation will not be changed in the near feature. So the SRCU memory barrier and the list iteration can be avoided. It's implemented in the patch - The other one is always put the "passthrough" pmus in the front of the list. So the unnecessary list iteration can be avoided. It does nothing for the SRCU lock pair. Thanks, Kan
On Mon, Oct 14, 2024 at 12:15:11PM -0400, Liang, Kan wrote: > > > On 2024-10-14 7:59 a.m., Peter Zijlstra wrote: > > On Mon, Sep 23, 2024 at 08:49:17PM +0200, Mingwei Zhang wrote: > > > >> The original implementation is by design having a terrible performance > >> overhead, ie., every PMU context switch at runtime requires a SRCU > >> lock pair and pmu list traversal. To reduce the overhead, we put > >> "passthrough" pmus in the front of the list and quickly exit the pmu > >> traversal when we just pass the last "passthrough" pmu. > > > > What was the expensive bit? The SRCU memory barrier or the list > > iteration? How long is that list really? > > Both. But I don't think there is any performance data. > > The length of the list could vary on different platforms. For a modern > server, there could be hundreds of PMUs from uncore PMUs, CXL PMUs, > IOMMU PMUs, PMUs of accelerator devices and PMUs from all kinds of > devices. The number could keep increasing with more and more devices > supporting the PMU capability. > > Two methods were considered. > - One is to add a global variable to track the "passthrough" pmu. The > idea assumes that there is only one "passthrough" pmu that requires the > switch, and the situation will not be changed in the near feature. > So the SRCU memory barrier and the list iteration can be avoided. > It's implemented in the patch > > - The other one is always put the "passthrough" pmus in the front of the > list. So the unnecessary list iteration can be avoided. It does nothing > for the SRCU lock pair. PaulMck has patches that introduce srcu_read_lock_lite(), which would avoid the smp_mb() in most cases. https://lkml.kernel.org/r/20241011173931.2050422-6-paulmck@kernel.org We can also keep a second list, just for the passthrough pmus. A bit like sched_cb_list.
On Mon, Oct 14, 2024 at 11:40:21AM -0400, Liang, Kan wrote: > >> @@ -11842,7 +11860,21 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type) > >> if (!pmu->event_idx) > >> pmu->event_idx = perf_event_idx_default; > >> > >> - list_add_rcu(&pmu->entry, &pmus); > >> + /* > >> + * Initialize passthru_pmu with the core pmu that has > >> + * PERF_PMU_CAP_PASSTHROUGH_VPMU capability. > >> + */ > >> + if (pmu->capabilities & PERF_PMU_CAP_PASSTHROUGH_VPMU) { > >> + if (!passthru_pmu) > >> + passthru_pmu = pmu; > >> + > >> + if (WARN_ONCE(passthru_pmu != pmu, "Only one passthrough PMU is supported\n")) { > >> + ret = -EINVAL; > >> + goto free_dev; > > > > Why impose this limit? Changelog also fails to explain this. > > Because the passthru_pmu is global variable. If there are two or more > PMUs with the PERF_PMU_CAP_PASSTHROUGH_VPMU, the former one will be > implicitly overwritten if without the check. That is not the question; but is has been answered elsewhere. For some reason you thought the srcu+list thing was expensive.
On Mon, Oct 14, 2024 at 11:51:06AM -0400, Liang, Kan wrote: > On 2024-10-14 8:03 a.m., Peter Zijlstra wrote: > > On Thu, Aug 01, 2024 at 04:58:23AM +0000, Mingwei Zhang wrote: > >> From: Kan Liang <kan.liang@linux.intel.com> > >> > >> There will be a dedicated interrupt vector for guests on some platforms, > >> e.g., Intel. Add an interface to switch the interrupt vector while > >> entering/exiting a guest. > >> > >> When PMI switch into a new guest vector, guest_lvtpc value need to be > >> reflected onto HW, e,g., guest clear PMI mask bit, the HW PMI mask > >> bit should be cleared also, then PMI can be generated continuously > >> for guest. So guest_lvtpc parameter is added into perf_guest_enter() > >> and switch_interrupt(). > >> > >> At switch_interrupt(), the target pmu with PASSTHROUGH cap should > >> be found. Since only one passthrough pmu is supported, we keep the > >> implementation simply by tracking the pmu as a global variable. > >> > >> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> > >> > >> [Simplify the commit with removal of srcu lock/unlock since only one pmu is > >> supported.] > >> > >> Signed-off-by: Mingwei Zhang <mizhang@google.com> > >> --- > >> include/linux/perf_event.h | 9 +++++++-- > >> kernel/events/core.c | 36 ++++++++++++++++++++++++++++++++++-- > >> 2 files changed, 41 insertions(+), 4 deletions(-) > >> > >> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > >> index 75773f9890cc..aeb08f78f539 100644 > >> --- a/include/linux/perf_event.h > >> +++ b/include/linux/perf_event.h > >> @@ -541,6 +541,11 @@ struct pmu { > >> * Check period value for PERF_EVENT_IOC_PERIOD ioctl. > >> */ > >> int (*check_period) (struct perf_event *event, u64 value); /* optional */ > >> + > >> + /* > >> + * Switch the interrupt vectors, e.g., guest enter/exit. > >> + */ > >> + void (*switch_interrupt) (bool enter, u32 guest_lvtpc); /* optional */ > >> }; > > > > I'm thinking the guets_lvtpc argument shouldn't be part of the > > interface. That should be PMU implementation data and accessed by the > > method implementation. > > I think the name of the perf_switch_interrupt() is too specific. > Here should be to switch the guest context. The interrupt should be just > part of the context. Maybe a interface as below > > void (*switch_guest_ctx) (bool enter, void *data); /* optional */ I don't think you even need the data thing. For example, the x86/intel implementation can just look at a x86_pmu data field to find the magic value.
On Mon, Oct 14, 2024 at 11:40:21AM -0400, Liang, Kan wrote: > > > On 2024-10-14 7:56 a.m., Peter Zijlstra wrote: > > On Thu, Aug 01, 2024 at 04:58:23AM +0000, Mingwei Zhang wrote: > > > >> @@ -5941,8 +5942,21 @@ void perf_put_mediated_pmu(void) > >> } > >> EXPORT_SYMBOL_GPL(perf_put_mediated_pmu); > >> > >> +static void perf_switch_interrupt(bool enter, u32 guest_lvtpc) > >> +{ > >> + /* Mediated passthrough PMU should have PASSTHROUGH_VPMU cap. */ > >> + if (!passthru_pmu) > >> + return; > >> + > >> + if (passthru_pmu->switch_interrupt && > >> + try_module_get(passthru_pmu->module)) { > >> + passthru_pmu->switch_interrupt(enter, guest_lvtpc); > >> + module_put(passthru_pmu->module); > >> + } > >> +} > > > > Should we move the whole module reference to perf_pmu_(,un}register() ? > > A PMU module can be load/unload anytime. How should we know if the PMU > module is available when the reference check is moved to > perf_pmu_(,un}register()? Feh, dunno. I never really use modules. I just think the above is naf -- doubly so because you're saying the SRCU smp_mb() are expensive; but this module reference crap is more expensive than that. (IOW, the SRCU smp_mb() cannot have been the problem)
On 2024-10-14 1:49 p.m., Peter Zijlstra wrote: > On Mon, Oct 14, 2024 at 11:51:06AM -0400, Liang, Kan wrote: >> On 2024-10-14 8:03 a.m., Peter Zijlstra wrote: >>> On Thu, Aug 01, 2024 at 04:58:23AM +0000, Mingwei Zhang wrote: >>>> From: Kan Liang <kan.liang@linux.intel.com> >>>> >>>> There will be a dedicated interrupt vector for guests on some platforms, >>>> e.g., Intel. Add an interface to switch the interrupt vector while >>>> entering/exiting a guest. >>>> >>>> When PMI switch into a new guest vector, guest_lvtpc value need to be >>>> reflected onto HW, e,g., guest clear PMI mask bit, the HW PMI mask >>>> bit should be cleared also, then PMI can be generated continuously >>>> for guest. So guest_lvtpc parameter is added into perf_guest_enter() >>>> and switch_interrupt(). >>>> >>>> At switch_interrupt(), the target pmu with PASSTHROUGH cap should >>>> be found. Since only one passthrough pmu is supported, we keep the >>>> implementation simply by tracking the pmu as a global variable. >>>> >>>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> >>>> >>>> [Simplify the commit with removal of srcu lock/unlock since only one pmu is >>>> supported.] >>>> >>>> Signed-off-by: Mingwei Zhang <mizhang@google.com> >>>> --- >>>> include/linux/perf_event.h | 9 +++++++-- >>>> kernel/events/core.c | 36 ++++++++++++++++++++++++++++++++++-- >>>> 2 files changed, 41 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h >>>> index 75773f9890cc..aeb08f78f539 100644 >>>> --- a/include/linux/perf_event.h >>>> +++ b/include/linux/perf_event.h >>>> @@ -541,6 +541,11 @@ struct pmu { >>>> * Check period value for PERF_EVENT_IOC_PERIOD ioctl. >>>> */ >>>> int (*check_period) (struct perf_event *event, u64 value); /* optional */ >>>> + >>>> + /* >>>> + * Switch the interrupt vectors, e.g., guest enter/exit. >>>> + */ >>>> + void (*switch_interrupt) (bool enter, u32 guest_lvtpc); /* optional */ >>>> }; >>> >>> I'm thinking the guets_lvtpc argument shouldn't be part of the >>> interface. That should be PMU implementation data and accessed by the >>> method implementation. >> >> I think the name of the perf_switch_interrupt() is too specific. >> Here should be to switch the guest context. The interrupt should be just >> part of the context. Maybe a interface as below >> >> void (*switch_guest_ctx) (bool enter, void *data); /* optional */ > > I don't think you even need the data thing. For example, the x86/intel > implementation can just look at a x86_pmu data field to find the magic > value. The new vector is created by KVM, not perf. So it cannot be found in the x86_pmu data field. Perf needs it to update the interrupt vector so the guest PMI can be handled by KVM directly. Thanks, Kan
On 2024-10-14 1:45 p.m., Peter Zijlstra wrote: > On Mon, Oct 14, 2024 at 12:15:11PM -0400, Liang, Kan wrote: >> >> >> On 2024-10-14 7:59 a.m., Peter Zijlstra wrote: >>> On Mon, Sep 23, 2024 at 08:49:17PM +0200, Mingwei Zhang wrote: >>> >>>> The original implementation is by design having a terrible performance >>>> overhead, ie., every PMU context switch at runtime requires a SRCU >>>> lock pair and pmu list traversal. To reduce the overhead, we put >>>> "passthrough" pmus in the front of the list and quickly exit the pmu >>>> traversal when we just pass the last "passthrough" pmu. >>> >>> What was the expensive bit? The SRCU memory barrier or the list >>> iteration? How long is that list really? >> >> Both. But I don't think there is any performance data. >> >> The length of the list could vary on different platforms. For a modern >> server, there could be hundreds of PMUs from uncore PMUs, CXL PMUs, >> IOMMU PMUs, PMUs of accelerator devices and PMUs from all kinds of >> devices. The number could keep increasing with more and more devices >> supporting the PMU capability. >> >> Two methods were considered. >> - One is to add a global variable to track the "passthrough" pmu. The >> idea assumes that there is only one "passthrough" pmu that requires the >> switch, and the situation will not be changed in the near feature. >> So the SRCU memory barrier and the list iteration can be avoided. >> It's implemented in the patch >> >> - The other one is always put the "passthrough" pmus in the front of the >> list. So the unnecessary list iteration can be avoided. It does nothing >> for the SRCU lock pair. > > PaulMck has patches that introduce srcu_read_lock_lite(), which would > avoid the smp_mb() in most cases. > > https://lkml.kernel.org/r/20241011173931.2050422-6-paulmck@kernel.org > > We can also keep a second list, just for the passthrough pmus. A bit > like sched_cb_list. Maybe we can do something like pmu_event_list, which has its own lock. So we should not need the srcu and the module reference. Thanks, Kan
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index 75773f9890cc..aeb08f78f539 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -541,6 +541,11 @@ struct pmu { * Check period value for PERF_EVENT_IOC_PERIOD ioctl. */ int (*check_period) (struct perf_event *event, u64 value); /* optional */ + + /* + * Switch the interrupt vectors, e.g., guest enter/exit. + */ + void (*switch_interrupt) (bool enter, u32 guest_lvtpc); /* optional */ }; enum perf_addr_filter_action_t { @@ -1738,7 +1743,7 @@ extern int perf_event_period(struct perf_event *event, u64 value); extern u64 perf_event_pause(struct perf_event *event, bool reset); int perf_get_mediated_pmu(void); void perf_put_mediated_pmu(void); -void perf_guest_enter(void); +void perf_guest_enter(u32 guest_lvtpc); void perf_guest_exit(void); #else /* !CONFIG_PERF_EVENTS: */ static inline void * @@ -1833,7 +1838,7 @@ static inline int perf_get_mediated_pmu(void) } static inline void perf_put_mediated_pmu(void) { } -static inline void perf_guest_enter(void) { } +static inline void perf_guest_enter(u32 guest_lvtpc) { } static inline void perf_guest_exit(void) { } #endif diff --git a/kernel/events/core.c b/kernel/events/core.c index 57ff737b922b..047ca5748ee2 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -422,6 +422,7 @@ static inline bool is_include_guest_event(struct perf_event *event) static LIST_HEAD(pmus); static DEFINE_MUTEX(pmus_lock); +static struct pmu *passthru_pmu; static struct srcu_struct pmus_srcu; static cpumask_var_t perf_online_mask; static struct kmem_cache *perf_event_cache; @@ -5941,8 +5942,21 @@ void perf_put_mediated_pmu(void) } EXPORT_SYMBOL_GPL(perf_put_mediated_pmu); +static void perf_switch_interrupt(bool enter, u32 guest_lvtpc) +{ + /* Mediated passthrough PMU should have PASSTHROUGH_VPMU cap. */ + if (!passthru_pmu) + return; + + if (passthru_pmu->switch_interrupt && + try_module_get(passthru_pmu->module)) { + passthru_pmu->switch_interrupt(enter, guest_lvtpc); + module_put(passthru_pmu->module); + } +} + /* When entering a guest, schedule out all exclude_guest events. */ -void perf_guest_enter(void) +void perf_guest_enter(u32 guest_lvtpc) { struct perf_cpu_context *cpuctx = this_cpu_ptr(&perf_cpu_context); @@ -5962,6 +5976,8 @@ void perf_guest_enter(void) perf_ctx_enable(cpuctx->task_ctx, EVENT_GUEST); } + perf_switch_interrupt(true, guest_lvtpc); + __this_cpu_write(perf_in_guest, true); unlock: @@ -5980,6 +5996,8 @@ void perf_guest_exit(void) if (WARN_ON_ONCE(!__this_cpu_read(perf_in_guest))) goto unlock; + perf_switch_interrupt(false, 0); + perf_ctx_disable(&cpuctx->ctx, EVENT_GUEST); ctx_sched_in(&cpuctx->ctx, EVENT_GUEST); perf_ctx_enable(&cpuctx->ctx, EVENT_GUEST); @@ -11842,7 +11860,21 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type) if (!pmu->event_idx) pmu->event_idx = perf_event_idx_default; - list_add_rcu(&pmu->entry, &pmus); + /* + * Initialize passthru_pmu with the core pmu that has + * PERF_PMU_CAP_PASSTHROUGH_VPMU capability. + */ + if (pmu->capabilities & PERF_PMU_CAP_PASSTHROUGH_VPMU) { + if (!passthru_pmu) + passthru_pmu = pmu; + + if (WARN_ONCE(passthru_pmu != pmu, "Only one passthrough PMU is supported\n")) { + ret = -EINVAL; + goto free_dev; + } + } + + list_add_tail_rcu(&pmu->entry, &pmus); atomic_set(&pmu->exclusive_cnt, 0); ret = 0; unlock: