Received-SPF: pass (google.com: domain of linux-kernel+bounces-177964-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240510191423.2297538-1-yabinc@google.com> <20240510191423.2297538-4-yabinc@google.com>
 <CAM9d7chNz8-84m28q5qSLjUjZ=Ni1CA_JzbB_P+YJooLQd85YA@mail.gmail.com>
In-Reply-To: <CAM9d7chNz8-84m28q5qSLjUjZ=Ni1CA_JzbB_P+YJooLQd85YA@mail.gmail.com>
From: Yabin Cui <yabinc@google.com>
Date: Mon, 13 May 2024 11:31:34 -0700
Message-ID: <CALJ9ZPP_2=X7XNQrLCV1pQUVH-pnHbW=Kz75ugSy+kda9Xwmpg@mail.gmail.com>
Subject: Re: [PATCH v4 3/3] perf/core: Check sample_type in perf_sample_save_brstack
To: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, 
	Arnaldo Carvalho de Melo <acme@kernel.org>, Mark Rutland <mark.rutland@arm.com>, 
	Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, 
	Ian Rogers <irogers@google.com>, Adrian Hunter <adrian.hunter@intel.com>, 
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org, 
	bpf@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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=E2=80=AFPM Namhyung Kim <namhyung@kernel.org> =
wrote:
>
> On Fri, May 10, 2024 at 12:14=E2=80=AFPM Yabin Cui <yabinc@google.com> wr=
ote:
> >
> > 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 *re=
gs)
> >                 if (!x86_perf_event_set_period(event))
> >                         continue;
> >
> > -               if (has_branch_stack(event))
> > -                       perf_sample_save_brstack(&data, event, &cpuc->l=
br_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->l=
br_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 p=
erf_event *event,
> >         if (x86_pmu.intel_cap.pebs_format >=3D 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(str=
uct perf_sample_data *data,
> >         data->sample_flags |=3D 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 *d=
ata,
> >                                             struct perf_event *event,
> >                                             struct perf_branch_stack *b=
rs,
> > @@ -1276,6 +1281,9 @@ static inline void perf_sample_save_brstack(struc=
t perf_sample_data *data,
> >  {
> >         int size =3D sizeof(u64); /* nr */
> >
> > +       if (!has_branch_stack(event))
> > +               return;
> > +
> >         if (branch_sample_hw_index(event))
> >                 size +=3D sizeof(u64);
> >         size +=3D brs->nr * sizeof(struct perf_branch_entry);
> > @@ -1665,11 +1673,6 @@ extern void perf_bp_event(struct perf_event *eve=
nt, 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 !=3D 0;
> > --
> > 2.45.0.118.g7fe29c98d7-goog
> >