Received-SPF: pass (google.com: domain of linux-kernel+bounces-184154-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240516041948.3546553-1-irogers@google.com> <20240516041948.3546553-2-irogers@google.com>
 <CAM9d7ch51JK8Xu+kOYUdxgM10_gS2=vjfW5sqFwrRS2eC8cYXA@mail.gmail.com> <CAP-5=fXNQ0=8mKKkBrmA9JpNLZEoQxb=AEYjBT-_brFt3Qom-A@mail.gmail.com>
In-Reply-To: <CAP-5=fXNQ0=8mKKkBrmA9JpNLZEoQxb=AEYjBT-_brFt3Qom-A@mail.gmail.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Mon, 20 May 2024 13:27:09 -0700
Message-ID: <CAM9d7cj4c-twjH5TgLiJ-9qoxkdh0v5mmU_dA87BrjsO5q7brA@mail.gmail.com>
Subject: Re: [PATCH v1 1/3] perf bpf filter: Give terms their own enum
To: Ian Rogers <irogers@google.com>
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>, 
	Adrian Hunter <adrian.hunter@intel.com>, Kan Liang <kan.liang@linux.intel.com>, 
	Changbin Du <changbin.du@huawei.com>, John Fastabend <john.fastabend@gmail.com>, 
	Andrii Nakryiko <andrii@kernel.org>, 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

On Fri, May 17, 2024 at 8:30=E2=80=AFPM Ian Rogers <irogers@google.com> wro=
te:
>
> On Fri, May 17, 2024 at 6:36=E2=80=AFPM Namhyung Kim <namhyung@kernel.org=
> wrote:
> >
> > On Wed, May 15, 2024 at 9:20=E2=80=AFPM Ian Rogers <irogers@google.com>=
 wrote:
> > >
> > > Give the term types their own enum so that additional terms can be
> > > added that don't correspond to a PERF_SAMPLE_xx flag. The term values
> > > are numerically ascending rather than bit field positions, this means
> > > they need translating to a PERF_SAMPLE_xx bit field in certain places
> > > and they are more densely encoded.
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > [SNIP]
> > > diff --git a/tools/perf/util/bpf_skel/sample_filter.bpf.c b/tools/per=
f/util/bpf_skel/sample_filter.bpf.c
> > > index fb94f5280626..8666c85e9333 100644
> > > --- a/tools/perf/util/bpf_skel/sample_filter.bpf.c
> > > +++ b/tools/perf/util/bpf_skel/sample_filter.bpf.c
> > > @@ -48,31 +48,50 @@ static inline __u64 perf_get_sample(struct bpf_pe=
rf_event_data_kern *kctx,
> > >  {
> > >         struct perf_sample_data___new *data =3D (void *)kctx->data;
> > >
> > > -       if (!bpf_core_field_exists(data->sample_flags) ||
> > > -           (data->sample_flags & entry->flags) =3D=3D 0)
> > > +       if (!bpf_core_field_exists(data->sample_flags))
> > >                 return 0;
> > >
> > > -       switch (entry->flags) {
> > > -       case PERF_SAMPLE_IP:
> > > +       switch (entry->term) {
> > > +       case PBF_TERM_NONE:
> > > +               return 0;
> > > +       case PBF_TERM_IP:
> > > +               if ((data->sample_flags & PERF_SAMPLE_IP) =3D=3D 0)
> > > +                       return 0;
> >
> > Can we check this in a single place like in the original code
> > instead of doing it in every case?  I think we can keep the
> > entry->flags and check it only if it's non zero.  Then uid and
> > gid will have 0 to skip.
>
> I found the old way confusing. As the flags are a bitmap it looks like
> more than one can be set, if that were the case then the switch
> statement would be broken as the case wouldn't exist. Using an enum

The entry->flags is set by the bpf-filter code and it's guaranteed
to have a single bit.

> like this allows warnings to occur when a term is missing in the
> switch statement - which is good when you are adding new terms. I
> think it more obviously matches the man page. We could arrange for the
> enum values to encode the shift position of the flag. Something like:
>
> if ((entry->term > PBF_TERM_NONE && entry->term <=3D PBF_TERM_DATA_SRC) &=
&
>     (data->sample_flags & (1 << entry->term)) =3D=3D 0)
>    return 0;

Actually I'm ok with enum (and bit shift if needed).  I'm just thinking
if it'd be nicer if we can have a check in a single place.

>
> But the problem there is that not every sample type has an enum value,
> and I'm not sure it makes sense for things like STREAM_ID. We could do
> some macro magic to reduce the verbosity like:
>
> #define SAMPLE_CASE(x) \
>     case PBF_TERM_##x: \
>         if ((data->sample_flags & PERF_SAMPLE_x) =3D=3D 0) \
>             return 0
>
> But I thought that made the code harder to read given the relatively
> small number of sample cases.

I also want to avoid the macro if possible.

Thanks,
Namhyung


> >
> > >                 return kctx->data->ip;
> > > -       case PERF_SAMPLE_ID:
> > > +       case PBF_TERM_ID:
> > > +               if ((data->sample_flags & PERF_SAMPLE_ID) =3D=3D 0)
> > > +                       return 0;
> > >                 return kctx->data->id;
> > > -       case PERF_SAMPLE_TID:
> > > +       case PBF_TERM_TID:
> > > +               if ((data->sample_flags & PERF_SAMPLE_TID) =3D=3D 0)
> > > +                       return 0;
> > >                 if (entry->part)
> > >                         return kctx->data->tid_entry.pid;
> > >                 else
> > >                         return kctx->data->tid_entry.tid;
> > > -       case PERF_SAMPLE_CPU:
> > > +       case PBF_TERM_CPU:
> > > +               if ((data->sample_flags & PERF_SAMPLE_CPU) =3D=3D 0)
> > > +                       return 0;
> > >                 return kctx->data->cpu_entry.cpu;
> > > -       case PERF_SAMPLE_TIME:
> > > +       case PBF_TERM_TIME:
> > > +               if ((data->sample_flags & PERF_SAMPLE_TIME) =3D=3D 0)
> > > +                       return 0;
> > >                 return kctx->data->time;
> > > -       case PERF_SAMPLE_ADDR:
> > > +       case PBF_TERM_ADDR:
> > > +               if ((data->sample_flags & PERF_SAMPLE_ADDR) =3D=3D 0)
> > > +                       return 0;
> > >                 return kctx->data->addr;
> > > -       case PERF_SAMPLE_PERIOD:
> > > +       case PBF_TERM_PERIOD:
> > > +               if ((data->sample_flags & PERF_SAMPLE_PERIOD) =3D=3D =
0)
> > > +                       return 0;
> > >                 return kctx->data->period;
> > > -       case PERF_SAMPLE_TRANSACTION:
> > > +       case PBF_TERM_TRANSACTION:
> > > +               if ((data->sample_flags & PERF_SAMPLE_TRANSACTION) =
=3D=3D 0)
> > > +                       return 0;
> > >                 return kctx->data->txn;
> > > -       case PERF_SAMPLE_WEIGHT_STRUCT:
> > > +       case PBF_TERM_WEIGHT_STRUCT:
> > > +               if ((data->sample_flags & PERF_SAMPLE_WEIGHT_STRUCT) =
=3D=3D 0)
> > > +                       return 0;
> > >                 if (entry->part =3D=3D 1)
> > >                         return kctx->data->weight.var1_dw;
> > >                 if (entry->part =3D=3D 2)
> > > @@ -80,15 +99,25 @@ static inline __u64 perf_get_sample(struct bpf_pe=
rf_event_data_kern *kctx,
> > >                 if (entry->part =3D=3D 3)
> > >                         return kctx->data->weight.var3_w;
> > >                 /* fall through */
> > > -       case PERF_SAMPLE_WEIGHT:
> > > +       case PBF_TERM_WEIGHT:
> > > +               if ((data->sample_flags & PERF_SAMPLE_WEIGHT) =3D=3D =
0)
> > > +                       return 0;
> > >                 return kctx->data->weight.full;
> > > -       case PERF_SAMPLE_PHYS_ADDR:
> > > +       case PBF_TERM_PHYS_ADDR:
> > > +               if ((data->sample_flags & PERF_SAMPLE_PHYS_ADDR) =3D=
=3D 0)
> > > +                       return 0;
> > >                 return kctx->data->phys_addr;
> > > -       case PERF_SAMPLE_CODE_PAGE_SIZE:
> > > +       case PBF_TERM_CODE_PAGE_SIZE:
> > > +               if ((data->sample_flags & PERF_SAMPLE_CODE_PAGE_SIZE)=
 =3D=3D 0)
> > > +                       return 0;
> > >                 return kctx->data->code_page_size;
> > > -       case PERF_SAMPLE_DATA_PAGE_SIZE:
> > > +       case PBF_TERM_DATA_PAGE_SIZE:
> > > +               if ((data->sample_flags & PERF_SAMPLE_DATA_PAGE_SIZE)=
 =3D=3D 0)
> > > +                       return 0;
> > >                 return kctx->data->data_page_size;
> > > -       case PERF_SAMPLE_DATA_SRC:
> > > +       case PBF_TERM_DATA_SRC:
> > > +               if ((data->sample_flags & PERF_SAMPLE_DATA_SRC) =3D=
=3D 0)
> > > +                       return 0;
> > >                 if (entry->part =3D=3D 1)
> > >                         return kctx->data->data_src.mem_op;
> > >                 if (entry->part =3D=3D 2)
> > > --
> > > 2.45.0.rc1.225.g2a3ae87e7f-goog
> > >