[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAP-5=fXTLBb1+PghPCraJQcXY518Jtt3AwPLpoXvjXErW79U5w@mail.gmail.com>
Date: Wed, 12 Feb 2025 23:27:38 -0800
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>, Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>,
Hao Ge <gehao@...inos.cn>, James Clark <james.clark@...aro.org>,
Howard Chu <howardchu95@...il.com>, Dominique Martinet <asmadeus@...ewreck.org>,
Levi Yun <yeoreum.yun@....com>, Xu Yang <xu.yang_2@....com>,
Tengda Wu <wutengda@...weicloud.com>, Yang Jihong <yangjihong1@...wei.com>,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 00/10] Move uid filtering to BPF filters
On Wed, Feb 12, 2025 at 5:44 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Wed, Feb 12, 2025 at 03:17:35PM -0800, Ian Rogers wrote:
> > On Wed, Feb 12, 2025 at 2:56 PM Namhyung Kim <namhyung@...nel.org> wrote:
> > >
> > > On Wed, Feb 12, 2025 at 12:00:42PM -0800, Ian Rogers wrote:
> > > > On Wed, Feb 12, 2025 at 10:46 AM Namhyung Kim <namhyung@...nel.org> wrote:
> > > > > It's not completely broken and works sometimes.
> > > >
> > > > No this is the definition of completely broken. If it only works
> > > > sometimes then you can't use it, we can't put a test on it, there is
> > > > no point in it. Even when it doesn't fail on perf_event_open, does it
> > > > work for processes that start after /proc is scanned? No, it is
> > > > completely broken.
> > >
> > > Ok, we have a different definition for it. Let's ignore the imaginary
> > > users of the broken features. Have you added a test for this change?
> > >
> > > Anyway I've tested your change and found some issues:
> > >
> > > 1. It silently exited when BPF-skel is not built. Better to put some
> > > messages at least.
> > >
> > > $ sudo ./perf record -u $(id -u) -- sleep 1
> > >
> > > 2. Even with BPF-skel, perf record doesn't work well. It did something
> > > but failed to get sample data for some reason.
> > >
> > > $ sudo ./perf record -u $(id -u) -- sleep 1
> > > [ perf record: Woken up 1 times to write data ]
> > > [ perf record: Captured and wrote 0.045 MB perf.data ]
> > >
> > > Oh, I think you now need to pass -a because it now works in
> > > system-wide mode and drops samples for other users.
> >
> > This is a pre-existing problem, no?
>
> No, it worked without -a in the past. Please see my previous reply.
> I think -u/--uid is one of the supported target in the perf tool (not
> for the system call) and it used to disable system-wide mode if -u is
> used at the same time.
As I keep repeating the '-u' option has never worked in the past, it
either early exited or missed recording new processes.
The documentation for `perf record` is:
```
...
-a, --all-cpus
System-wide collection from all CPUs (default if no target
is specified).
...
-u, --uid=
Record events in threads owned by uid. Name or number.
...
```
So `-a` is implied without a target/workload but you are specifying a
workload and `-u`. So do you want samples from the workload or from
the user's processes? I can tell from your command line being sleep
that you want it to imply `-a` but would that be true if it were a
benchmark? For the benchmark case you probably don't want `-a`
implied. If you specify `-p` and `-u` should the processes that don't
match -u be sampled or are you expecting implicit system wide
profiling now?
I agree the behavior in the patch series doesn't match the existing
behavior, but I'm not sure the existing behavior agrees with the
documentation nor working as expected. Having the `-u` not select
system wide, as in the patch, seems to agree with the documentation
better. You have specified a target workload/process/eventual pid and
you want samples owned by the uid, why should you now start getting
samples from other processes? With `-p` you wouldn't expect `-a` to
suddenly get implied, I'm not sure it makes any more sense with any
target/workload.
> > > 3. With BPF-skel, non-root users will see this.
> > >
> > > $ ./perf record -u $(id -u) -- sleep 1
> > > cannot get fd for 'filters' map
> > > failed to set filter "BPF" on event cycles:P with 13 (Permission denied)
> > >
> > > I think it's confusing and better to tell user that you need to run
> > > 'perf record --setup-filter pin' as root first. But maybe due to the
> > > issue #2, you still need to run it as root.
I think that is an okay addition to BPF filters in general. I'm wary
of having patch series feature crept into requiring the entire tool
being reimplemented.
Thanks,
Ian
Powered by blists - more mailing lists