MIME-Version: 1.0
References: <20230222230141.1729048-1-namhyung@kernel.org> <20230222230141.1729048-3-namhyung@kernel.org>
 <0c4afcfb-fe9a-89ac-16aa-57ce0d2a70bc@intel.com>
In-Reply-To: <0c4afcfb-fe9a-89ac-16aa-57ce0d2a70bc@intel.com>
From:   Namhyung Kim <namhyung@kernel.org>
Date:   Tue, 7 Mar 2023 12:57:26 -0800
Message-ID: <CAM9d7chU4pPVTT1rFvr4zP27c5DOVuX6MJi6qBw9SgN1B-Ezww@mail.gmail.com>
Subject: Re: [PATCH 2/8] perf bpf filter: Implement event sample filtering
To:     Adrian Hunter <adrian.hunter@intel.com>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Jiri Olsa <jolsa@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>,
        Ian Rogers <irogers@google.com>,
        Andi Kleen <ak@linux.intel.com>,
        Kan Liang <kan.liang@linux.intel.com>,
        Song Liu <song@kernel.org>,
        Stephane Eranian <eranian@google.com>,
        Ravi Bangoria <ravi.bangoria@amd.com>,
        Leo Yan <leo.yan@linaro.org>,
        James Clark <james.clark@arm.com>, Hao Luo <haoluo@google.com>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-perf-users@vger.kernel.org, bpf@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Adrian,

Thanks for your feedback!

On Tue, Mar 7, 2023 at 5:04 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 23/02/23 01:01, Namhyung Kim wrote:
> > The BPF program will be attached to a perf_event and be triggered when
> > it overflows.  It'd iterate the filters map and compare the sample
> > value according to the expression.  If any of them fails, the sample
> > would be dropped.
> >
> > Also it needs to have the corresponding sample data for the expression
> > so it compares data->sample_flags with the given value.  To access the
> > sample data, it uses the bpf_cast_to_kern_ctx() kfunc which was added
> > in v6.2 kernel.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>

[SNIP]
> > diff --git a/tools/perf/util/bpf-filter.h b/tools/perf/util/bpf-filter.h
> > index fd5b1164a322..6077930073f9 100644
> > --- a/tools/perf/util/bpf-filter.h
> > +++ b/tools/perf/util/bpf-filter.h
> > @@ -4,15 +4,7 @@
> >
> >  #include <linux/list.h>
> >
> > -enum perf_bpf_filter_op {
> > -     PBF_OP_EQ,
> > -     PBF_OP_NEQ,
> > -     PBF_OP_GT,
> > -     PBF_OP_GE,
> > -     PBF_OP_LT,
> > -     PBF_OP_LE,
> > -     PBF_OP_AND,
> > -};
> > +#include "bpf_skel/sample-filter.h"
> >
> >  struct perf_bpf_filter_expr {
> >       struct list_head list;
> > @@ -21,16 +13,30 @@ struct perf_bpf_filter_expr {
> >       unsigned long val;
> >  };
> >
> > +struct evsel;
> > +
> >  #ifdef HAVE_BPF_SKEL
> >  struct perf_bpf_filter_expr *perf_bpf_filter_expr__new(unsigned long sample_flags,
> >                                                      enum perf_bpf_filter_op op,
> >                                                      unsigned long val);
> >  int perf_bpf_filter__parse(struct list_head *expr_head, const char *str);
> > +int perf_bpf_filter__prepare(struct evsel *evsel);
> > +int perf_bpf_filter__destroy(struct evsel *evsel);
> > +
> >  #else /* !HAVE_BPF_SKEL */
> > +
> >  static inline int perf_bpf_filter__parse(struct list_head *expr_head __maybe_unused,
> >                                        const char *str __maybe_unused)
> >  {
> >       return -ENOSYS;
>
> Any reason for ENOSYS instead of say EOPNOTSUPP?

No specific reason.  I can change it to EOPNOTSUPP.

>
> >  }
> > +static inline int perf_bpf_filter__prepare(struct evsel *evsel)
>
> Needs  __maybe_unused on the parameters

Sure.

>
> > +{
> > +     return -ENOSYS;
> > +}
> > +static inline int perf_bpf_filter__destroy(struct evsel *evsel)
>
> Needs  __maybe_unused on the parameters

Will do.

Thanks,
Namhyung