MIME-Version: 1.0
References: <20230219061329.1001079-1-namhyung@kernel.org> <Y/d8juzJIRHdtKlQ@kernel.org>
In-Reply-To: <Y/d8juzJIRHdtKlQ@kernel.org>
From:   Namhyung Kim <namhyung@kernel.org>
Date:   Thu, 23 Feb 2023 13:49:49 -0800
Message-ID: <CAM9d7cgxJEMioUAqVN7g+GVfmEYtJ3S1tGyNhdQH_qv7wYJAFg@mail.gmail.com>
Subject: Re: [RFC/PATCHSET 0/8] perf record: Implement BPF sample filter (v2)
To:     Arnaldo Carvalho de Melo <acme@kernel.org>
Cc:     Jiri Olsa <jolsa@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>,
        Ian Rogers <irogers@google.com>,
        Adrian Hunter <adrian.hunter@intel.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

On Thu, Feb 23, 2023 at 6:47 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Sat, Feb 18, 2023 at 10:13:21PM -0800, Namhyung Kim escreveu:
> > Hello,
> >
> > There have been requests for more sophisticated perf event sample
> > filtering based on the sample data.  Recently the kernel added BPF
> > programs can access perf sample data and this is the userspace part
> > to enable such a filtering.
> >
> > This still has some rough edges and needs more improvements.  But
> > I'd like to share the current work and get some feedback for the
> > directions and idea for further improvements.
> >
> > v2 changes)
> >  * fix build error with the misc field  (Jiri)
> >  * add a destructor for filter expr  (Ian)
> >  * remove 'bpf:' prefix  (Arnaldo)
> >  * add '||' operator
> >
> > The kernel changes are in the tip.git tree (perf/core branch) for now.
> > perf record has --filter option to set filters on the last specified
> > event in the command line.  It worked only for tracepoints and Intel
> > PT events so far.  This patchset extends it to have 'bpf:' prefix in
> > order to enable the general sample filters using BPF for any events.
> >
> > A new filter expression parser was added (using flex/bison) to process
> > the filter string.  Right now, it only accepts very simple expressions
> > separated by comma.  I'd like to keep the filter expression as simple
> > as possible.
> >
> > It requires samples satisfy all the filter expressions otherwise it'd
> > drop the sample.  IOW filter expressions are connected with logical AND
> > operations implicitly.
> >
> > Essentially the BPF filter expression is:
> >
> >   <term> <operator> <value> (("," | "||") <term> <operator> <value>)*
>
> So "," means "&&" ?

Right. :)

>
> I understand that its less characters, but can't we use the well
> established '&&' instead? :-)

It's doable but I was a bit afraid of the complexity or confusion
it might bring up.  It should handle expressions like 'A && B || C'
then we need to be clear on the precedence of them.  Currently
it would behave like 'A && (B || C)' but users might expect
'(A && B) || C'.

Then we might want to add the parentheses explicitly to adjust
the operator priority.  That could result in complex expressions
possibly with nested parentheses.  I'm not sure if we really want
this.  Considering the restrictions in BPF, I think we should keep
the expressions as simple as possible.

Thanks,
Namhyung