[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z6zslLa8XM1ubwXj@google.com>
Date: Wed, 12 Feb 2025 10:46:44 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
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 Tue, Feb 11, 2025 at 09:41:04PM -0800, Ian Rogers wrote:
> On Tue, Feb 11, 2025 at 5:51 PM Namhyung Kim <namhyung@...nel.org> wrote:
> > But you removed non-BPF and non-root (w/o pinning BPF) use cases.
>
> I didn't think this was a hard series to understand. It moves the -u
> options of perf top and perf record to using BPF filters. The many
> reasons for this I already explained:
> https://lore.kernel.org/lkml/CAP-5=fUY83gifsMZA0Q45kiQQbAKp2uJxkuwrwGtHK2hiUFRDA@mail.gmail.com/
>
> Your case is a user that isn't exiting and starting processes and
> wants to process themself or some other user they some how have
> permissions for? They need to not be starting and exiting processes as
> new processes are ignored and exiting processes cause the
> perf_event_open to fail. What stops such a user passing the list of
> processes they have that aren't starting and exiting as a -p option?
>
> If you try something like:
> $ perf top -p $(ps --no-headers -u $USER -o pid | awk '{printf "%s%s",
> sep, $1; sep=","}')
> this is exactly what you get. Does it work? No, the ps and awk
> processes terminating but being in the list of processes causes the
> perf_event_open for those pids to fail. This is exactly the same
> problem as the -u option that you want to keep for this case. The
> approach just doesn't work.
>
> Why not make failing to add a -u option fallback on doing the /proc
> scan and pretend the processes are a -p option? So now there's a
> silent fallback to a broken behavior. New processes won't be profiled
> and the data race between the scan and the perf_event_open will cause
> failures with non-obvious causes/solutions - mainly, use sudo to run
> as root. I'd say this isn't ideal.
>
> This series fixes an option on two commands so that it works in the
> sensible use-case, perf running with privileges trying to filter
> samples belonging to a specific user. We can say the patch series
> doesn't work for the case you give, I don't think anybody cares about
> that case, they can get near identical behavior from -p, the behavior
> from -p will be clearer than just having -u doing something similar,
> namely failing to open for a process and terminate.
>
> You may hate and ignore every point I make and still want to keep the
> existing broken behavior. As I've already tried to make clear, you're
> adding to the maintenance burden to everyone working in the code base
> as the notion of target is fundamental and because you are insisting
> on keeping a broken behavior you are also making it untestable. Given
> the -u option doesn't work, I strongly suspect nobody uses it. Do I
> worry about this series causing harm to the people who aren't using
> the option? No I don't as there is a better opportunity in having an
> option that (1) does work and (2) results in a simpler code base.
It's not completely broken and works sometimes. And it seems we have an
issue with BPF sideband events. But it worked when you ran it as root.
$ sudo perf record -u $(id -u) --no-bpf-event -- sleep 1
WARNING: Ignored open failure for pid 404758
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 2.754 MB perf.data (3638 samples) ]
$ sudo perf report -s sym --stdio -q | head
1.43% [k] audit_filter_rules.isra.0
1.33% [.] pthread_mutex_lock@@GLIBC_2.2.5
1.06% [k] __audit_filter_op
1.05% [.] __vdso_clock_gettime
0.94% [.] _dbus_type_reader_get_current_type
0.82% [.] pthread_mutex_trylock@@GLIBC_2.34
0.76% [k] __fdget
0.72% [.] _dbus_first_type_in_signature
0.61% [.] __GI___pthread_mutex_unlock_usercnt
0.56% [k] native_sched_clock
Thanks,
Namhyung
Powered by blists - more mailing lists