diff mbox series

[v4,3/3] perf/core: Check sample_type in perf_sample_save_brstack

Message ID 20240510191423.2297538-4-yabinc@google.com (mailing list archive)
State Superseded
Headers show
Series perf/core: Check sample_type in sample data saving helper functions | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization

Commit Message

Yabin Cui May 10, 2024, 7:14 p.m. UTC 
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(-)

Comments

Namhyung Kim May 10, 2024, 9:29 p.m. UTC | #1 
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
>
Yabin Cui May 13, 2024, 6:31 p.m. UTC | #2 
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
> >
Namhyung Kim May 13, 2024, 8:39 p.m. UTC | #3 
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
Peter Zijlstra May 15, 2024, 8:58 a.m. UTC | #4 
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?
Yabin Cui May 15, 2024, 5:47 p.m. UTC | #5 
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().
diff mbox series

Patch

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;