Received-SPF: pass (google.com: domain of linux-kernel+bounces-182740-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240516041948.3546553-1-irogers@google.com> <20240516041948.3546553-2-irogers@google.com>
In-Reply-To: <20240516041948.3546553-2-irogers@google.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Fri, 17 May 2024 18:36:22 -0700
Message-ID: <CAM9d7ch51JK8Xu+kOYUdxgM10_gS2=vjfW5sqFwrRS2eC8cYXA@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 Wed, May 15, 2024 at 9:20=E2=80=AFPM Ian Rogers <irogers@google.com> wro=
te:
>
> 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.h b/tools/perf/util/b=
pf_skel/sample-filter.h
> index 2e96e1ab084a..161d5ff49cb6 100644
> --- a/tools/perf/util/bpf_skel/sample-filter.h
> +++ b/tools/perf/util/bpf_skel/sample-filter.h
> @@ -16,12 +16,32 @@ enum perf_bpf_filter_op {
>         PBF_OP_GROUP_END,
>  };
>
> +enum perf_bpf_filter_term {
> +       /* No term is in use. */
> +       PBF_TERM_NONE,
> +       /* Terms that correspond to PERF_SAMPLE_xx values. */
> +       PBF_TERM_IP,
> +       PBF_TERM_ID,
> +       PBF_TERM_TID,
> +       PBF_TERM_CPU,
> +       PBF_TERM_TIME,
> +       PBF_TERM_ADDR,
> +       PBF_TERM_PERIOD,
> +       PBF_TERM_TRANSACTION,
> +       PBF_TERM_WEIGHT,
> +       PBF_TERM_PHYS_ADDR,
> +       PBF_TERM_CODE_PAGE_SIZE,
> +       PBF_TERM_DATA_PAGE_SIZE,
> +       PBF_TERM_WEIGHT_STRUCT,
> +       PBF_TERM_DATA_SRC,
> +};
> +
>  /* BPF map entry for filtering */
>  struct perf_bpf_filter_entry {
>         enum perf_bpf_filter_op op;
>         __u32 part; /* sub-sample type info when it has multiple values *=
/
> -       __u64 flags; /* perf sample type flags */
> +       enum perf_bpf_filter_term term;
>         __u64 value;
>  };
>
> -#endif /* PERF_UTIL_BPF_SKEL_SAMPLE_FILTER_H */
> \ No newline at end of file
> +#endif /* PERF_UTIL_BPF_SKEL_SAMPLE_FILTER_H */
> diff --git a/tools/perf/util/bpf_skel/sample_filter.bpf.c b/tools/perf/ut=
il/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_perf_e=
vent_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.

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_perf_e=
vent_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
>