Check sample_type in perf_sample_save_brstack() to prevent
saving branch stack data when it isn't required.
Suggested-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Yabin Cui <yabinc@google.com>
---
arch/x86/events/amd/core.c | 3 +--
arch/x86/events/core.c | 3 +--
arch/x86/events/intel/ds.c | 3 +--
include/linux/perf_event.h | 13 ++++++++-----
4 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
index 985ef3b47919..fb9bf3aa1b42 100644
--- a/arch/x86/events/amd/core.c
+++ b/arch/x86/events/amd/core.c
@@ -967,8 +967,7 @@ static int amd_pmu_v2_handle_irq(struct pt_regs *regs)
if (!x86_perf_event_set_period(event))
continue;
- if (has_branch_stack(event))
- perf_sample_save_brstack(&data, event, &cpuc->lbr_stack, NULL);
+ perf_sample_save_brstack(&data, event, &cpuc->lbr_stack, NULL);
if (perf_event_overflow(event, &data, regs))
x86_pmu_stop(event, 0);
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 5b0dd07b1ef1..ff5577315938 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1702,8 +1702,7 @@ int x86_pmu_handle_irq(struct pt_regs *regs)
perf_sample_data_init(&data, 0, event->hw.last_period);
- if (has_branch_stack(event))
- perf_sample_save_brstack(&data, event, &cpuc->lbr_stack, NULL);
+ perf_sample_save_brstack(&data, event, &cpuc->lbr_stack, NULL);
if (perf_event_overflow(event, &data, regs))
x86_pmu_stop(event, 0);
diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
index c2b5585aa6d1..f25236ffa28f 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -1754,8 +1754,7 @@ static void setup_pebs_fixed_sample_data(struct perf_event *event,
if (x86_pmu.intel_cap.pebs_format >= 3)
setup_pebs_time(event, data, pebs->tsc);
- if (has_branch_stack(event))
- perf_sample_save_brstack(data, event, &cpuc->lbr_stack, NULL);
+ perf_sample_save_brstack(data, event, &cpuc->lbr_stack, NULL);
}
static void adaptive_pebs_save_regs(struct pt_regs *regs,
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 8617815456b0..ecfbe22ff299 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1269,6 +1269,11 @@ static inline void perf_sample_save_raw_data(struct perf_sample_data *data,
data->sample_flags |= PERF_SAMPLE_RAW;
}
+static inline bool has_branch_stack(struct perf_event *event)
+{
+ return event->attr.sample_type & PERF_SAMPLE_BRANCH_STACK;
+}
+
static inline void perf_sample_save_brstack(struct perf_sample_data *data,
struct perf_event *event,
struct perf_branch_stack *brs,
@@ -1276,6 +1281,9 @@ static inline void perf_sample_save_brstack(struct perf_sample_data *data,
{
int size = sizeof(u64); /* nr */
+ if (!has_branch_stack(event))
+ return;
+
if (branch_sample_hw_index(event))
size += sizeof(u64);
size += brs->nr * sizeof(struct perf_branch_entry);
@@ -1665,11 +1673,6 @@ extern void perf_bp_event(struct perf_event *event, void *data);
# define perf_arch_bpf_user_pt_regs(regs) regs
#endif
-static inline bool has_branch_stack(struct perf_event *event)
-{
- return event->attr.sample_type & PERF_SAMPLE_BRANCH_STACK;
-}
-
static inline bool needs_branch_stack(struct perf_event *event)
{
return event->attr.branch_sample_type != 0;
--
2.45.0.118.g7fe29c98d7-googOn Fri, May 10, 2024 at 12:14 PM Yabin Cui <yabinc@google.com> wrote:
>
> Check sample_type in perf_sample_save_brstack() to prevent
> saving branch stack data when it isn't required.
>
> Suggested-by: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Yabin Cui <yabinc@google.com>
It seems powerpc has the similar bug, then you need this:
Fixes: eb55b455ef9c ("perf/core: Add perf_sample_save_brstack() helper")
Thanks,
Namhyung
> ---
> arch/x86/events/amd/core.c | 3 +--
> arch/x86/events/core.c | 3 +--
> arch/x86/events/intel/ds.c | 3 +--
> include/linux/perf_event.h | 13 ++++++++-----
> 4 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
> index 985ef3b47919..fb9bf3aa1b42 100644
> --- a/arch/x86/events/amd/core.c
> +++ b/arch/x86/events/amd/core.c
> @@ -967,8 +967,7 @@ static int amd_pmu_v2_handle_irq(struct pt_regs *regs)
> if (!x86_perf_event_set_period(event))
> continue;
>
> - if (has_branch_stack(event))
> - perf_sample_save_brstack(&data, event, &cpuc->lbr_stack, NULL);
> + perf_sample_save_brstack(&data, event, &cpuc->lbr_stack, NULL);
>
> if (perf_event_overflow(event, &data, regs))
> x86_pmu_stop(event, 0);
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 5b0dd07b1ef1..ff5577315938 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -1702,8 +1702,7 @@ int x86_pmu_handle_irq(struct pt_regs *regs)
>
> perf_sample_data_init(&data, 0, event->hw.last_period);
>
> - if (has_branch_stack(event))
> - perf_sample_save_brstack(&data, event, &cpuc->lbr_stack, NULL);
> + perf_sample_save_brstack(&data, event, &cpuc->lbr_stack, NULL);
>
> if (perf_event_overflow(event, &data, regs))
> x86_pmu_stop(event, 0);
> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index c2b5585aa6d1..f25236ffa28f 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -1754,8 +1754,7 @@ static void setup_pebs_fixed_sample_data(struct perf_event *event,
> if (x86_pmu.intel_cap.pebs_format >= 3)
> setup_pebs_time(event, data, pebs->tsc);
>
> - if (has_branch_stack(event))
> - perf_sample_save_brstack(data, event, &cpuc->lbr_stack, NULL);
> + perf_sample_save_brstack(data, event, &cpuc->lbr_stack, NULL);
> }
>
> static void adaptive_pebs_save_regs(struct pt_regs *regs,
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 8617815456b0..ecfbe22ff299 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -1269,6 +1269,11 @@ static inline void perf_sample_save_raw_data(struct perf_sample_data *data,
> data->sample_flags |= PERF_SAMPLE_RAW;
> }
>
> +static inline bool has_branch_stack(struct perf_event *event)
> +{
> + return event->attr.sample_type & PERF_SAMPLE_BRANCH_STACK;
> +}
> +
> static inline void perf_sample_save_brstack(struct perf_sample_data *data,
> struct perf_event *event,
> struct perf_branch_stack *brs,
> @@ -1276,6 +1281,9 @@ static inline void perf_sample_save_brstack(struct perf_sample_data *data,
> {
> int size = sizeof(u64); /* nr */
>
> + if (!has_branch_stack(event))
> + return;
> +
> if (branch_sample_hw_index(event))
> size += sizeof(u64);
> size += brs->nr * sizeof(struct perf_branch_entry);
> @@ -1665,11 +1673,6 @@ extern void perf_bp_event(struct perf_event *event, void *data);
> # define perf_arch_bpf_user_pt_regs(regs) regs
> #endif
>
> -static inline bool has_branch_stack(struct perf_event *event)
> -{
> - return event->attr.sample_type & PERF_SAMPLE_BRANCH_STACK;
> -}
> -
> static inline bool needs_branch_stack(struct perf_event *event)
> {
> return event->attr.branch_sample_type != 0;
> --
> 2.45.0.118.g7fe29c98d7-goog
>
On Fri, May 10, 2024 at 02:29:58PM -0700, Namhyung Kim wrote:
> On Fri, May 10, 2024 at 12:14 PM Yabin Cui <yabinc@google.com> wrote:
> >
> > Check sample_type in perf_sample_save_brstack() to prevent
> > saving branch stack data when it isn't required.
> >
> > Suggested-by: Namhyung Kim <namhyung@kernel.org>
> > Signed-off-by: Yabin Cui <yabinc@google.com>
>
> It seems powerpc has the similar bug, then you need this:
>
> Fixes: eb55b455ef9c ("perf/core: Add perf_sample_save_brstack() helper")
Is this really a bug? AFAICT it just does unneeded work, no?
On Wed, May 15, 2024 at 1:58 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, May 10, 2024 at 02:29:58PM -0700, Namhyung Kim wrote:
> > On Fri, May 10, 2024 at 12:14 PM Yabin Cui <yabinc@google.com> wrote:
> > >
> > > Check sample_type in perf_sample_save_brstack() to prevent
> > > saving branch stack data when it isn't required.
> > >
> > > Suggested-by: Namhyung Kim <namhyung@kernel.org>
> > > Signed-off-by: Yabin Cui <yabinc@google.com>
> >
> > It seems powerpc has the similar bug, then you need this:
> >
> > Fixes: eb55b455ef9c ("perf/core: Add perf_sample_save_brstack() helper")
>
> Is this really a bug? AFAICT it just does unneeded work, no?
It's not a bug. As I replied to Namhyuang, the powerpc code checks
sample_type before calling perf_sample_save_brstack().
arch/powerpc/perf/core-book3s.c checks sample_type, see
if (event->attr.sample_type & PERF_SAMPLE_BRANCH_STACK) {
...
perf_sample_save_brstack(&data, event, &cpuhw->bhrb_stack, NULL);
}
So I think we don't need the "fixes:" line.
On Fri, May 10, 2024 at 2:30 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Fri, May 10, 2024 at 12:14 PM Yabin Cui <yabinc@google.com> wrote:
> >
> > Check sample_type in perf_sample_save_brstack() to prevent
> > saving branch stack data when it isn't required.
> >
> > Suggested-by: Namhyung Kim <namhyung@kernel.org>
> > Signed-off-by: Yabin Cui <yabinc@google.com>
>
> It seems powerpc has the similar bug, then you need this:
>
> Fixes: eb55b455ef9c ("perf/core: Add perf_sample_save_brstack() helper")
>
> Thanks,
> Namhyung
>
> > ---
> > arch/x86/events/amd/core.c | 3 +--
> > arch/x86/events/core.c | 3 +--
> > arch/x86/events/intel/ds.c | 3 +--
> > include/linux/perf_event.h | 13 ++++++++-----
> > 4 files changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
> > index 985ef3b47919..fb9bf3aa1b42 100644
> > --- a/arch/x86/events/amd/core.c
> > +++ b/arch/x86/events/amd/core.c
> > @@ -967,8 +967,7 @@ static int amd_pmu_v2_handle_irq(struct pt_regs *regs)
> > if (!x86_perf_event_set_period(event))
> > continue;
> >
> > - if (has_branch_stack(event))
> > - perf_sample_save_brstack(&data, event, &cpuc->lbr_stack, NULL);
> > + perf_sample_save_brstack(&data, event, &cpuc->lbr_stack, NULL);
> >
> > if (perf_event_overflow(event, &data, regs))
> > x86_pmu_stop(event, 0);
> > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> > index 5b0dd07b1ef1..ff5577315938 100644
> > --- a/arch/x86/events/core.c
> > +++ b/arch/x86/events/core.c
> > @@ -1702,8 +1702,7 @@ int x86_pmu_handle_irq(struct pt_regs *regs)
> >
> > perf_sample_data_init(&data, 0, event->hw.last_period);
> >
> > - if (has_branch_stack(event))
> > - perf_sample_save_brstack(&data, event, &cpuc->lbr_stack, NULL);
> > + perf_sample_save_brstack(&data, event, &cpuc->lbr_stack, NULL);
> >
> > if (perf_event_overflow(event, &data, regs))
> > x86_pmu_stop(event, 0);
> > diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> > index c2b5585aa6d1..f25236ffa28f 100644
> > --- a/arch/x86/events/intel/ds.c
> > +++ b/arch/x86/events/intel/ds.c
> > @@ -1754,8 +1754,7 @@ static void setup_pebs_fixed_sample_data(struct perf_event *event,
> > if (x86_pmu.intel_cap.pebs_format >= 3)
> > setup_pebs_time(event, data, pebs->tsc);
> >
> > - if (has_branch_stack(event))
> > - perf_sample_save_brstack(data, event, &cpuc->lbr_stack, NULL);
> > + perf_sample_save_brstack(data, event, &cpuc->lbr_stack, NULL);
> > }
> >
> > static void adaptive_pebs_save_regs(struct pt_regs *regs,
> > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> > index 8617815456b0..ecfbe22ff299 100644
> > --- a/include/linux/perf_event.h
> > +++ b/include/linux/perf_event.h
> > @@ -1269,6 +1269,11 @@ static inline void perf_sample_save_raw_data(struct perf_sample_data *data,
> > data->sample_flags |= PERF_SAMPLE_RAW;
> > }
> >
> > +static inline bool has_branch_stack(struct perf_event *event)
> > +{
> > + return event->attr.sample_type & PERF_SAMPLE_BRANCH_STACK;
> > +}
> > +
> > static inline void perf_sample_save_brstack(struct perf_sample_data *data,
> > struct perf_event *event,
> > struct perf_branch_stack *brs,
> > @@ -1276,6 +1281,9 @@ static inline void perf_sample_save_brstack(struct perf_sample_data *data,
> > {
> > int size = sizeof(u64); /* nr */
> >
> > + if (!has_branch_stack(event))
> > + return;
> > +
> > if (branch_sample_hw_index(event))
> > size += sizeof(u64);
> > size += brs->nr * sizeof(struct perf_branch_entry);
> > @@ -1665,11 +1673,6 @@ extern void perf_bp_event(struct perf_event *event, void *data);
> > # define perf_arch_bpf_user_pt_regs(regs) regs
> > #endif
> >
> > -static inline bool has_branch_stack(struct perf_event *event)
> > -{
> > - return event->attr.sample_type & PERF_SAMPLE_BRANCH_STACK;
> > -}
> > -
> > static inline bool needs_branch_stack(struct perf_event *event)
> > {
> > return event->attr.branch_sample_type != 0;
> > --
> > 2.45.0.118.g7fe29c98d7-goog
> >
On Mon, May 13, 2024 at 11:31 AM Yabin Cui <yabinc@google.com> wrote:
>
> arch/powerpc/perf/core-book3s.c checks sample_type, see
> if (event->attr.sample_type & PERF_SAMPLE_BRANCH_STACK) {
> ...
> perf_sample_save_brstack(&data, event, &cpuhw->bhrb_stack, NULL);
> }
> So I think we don't need the "fixes:" line.
Oh, ok. Thanks for the correction!
Namhyung
© 2016 - 2026 Red Hat, Inc.