[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEf4BzZUVe-dQNcb1VQbEcN4kBFOYrFOB537q4Vhtpm_ebL9aQ@mail.gmail.com>
Date: Fri, 12 Jul 2024 14:06:08 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: mingo@...nel.org, andrii@...nel.org, oleg@...hat.com,
linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
rostedt@...dmis.org, mhiramat@...nel.org, jolsa@...nel.org, clm@...a.com,
paulmck@...nel.org, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH v2 06/11] perf/uprobe: SRCU-ify uprobe->consumer list
+ bpf@...r, please cc bpf ML for the next revision, these changes are
very relevant there as well, thanks
On Thu, Jul 11, 2024 at 4:07 AM Peter Zijlstra <peterz@...radead.org> wrote:
>
> With handle_swbp() hitting concurrently on (all) CPUs the
> uprobe->register_rwsem can get very contended. Add an SRCU instance to
> cover the consumer list and consumer lifetime.
>
> Since the consumer are externally embedded structures, unregister will
> have to suffer a synchronize_srcu().
>
> A notably complication is the UPROBE_HANDLER_REMOVE logic which can
> race against uprobe_register() such that it might want to remove a
> freshly installer handler that didn't get called. In order to close
> this hole, a seqcount is added. With that, the removal path can tell
> if anything changed and bail out of the removal.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
> kernel/events/uprobes.c | 60 ++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 50 insertions(+), 10 deletions(-)
>
[...]
> @@ -800,7 +808,7 @@ static bool consumer_del(struct uprobe *
> down_write(&uprobe->consumer_rwsem);
> for (con = &uprobe->consumers; *con; con = &(*con)->next) {
> if (*con == uc) {
> - *con = uc->next;
> + WRITE_ONCE(*con, uc->next);
I have a dumb and mechanical question.
Above in consumer_add() you are doing WRITE_ONCE() for uc->next
assignment, but rcu_assign_pointer() for uprobe->consumers. Here, you
are doing WRITE_ONCE() for the same operation, if it so happens that
uc == *con == uprobe->consumers. So is rcu_assign_pointer() necessary
in consumer_addr()? If yes, we should have it here as well, no? And if
not, why bother with it in consumer_add()?
> ret = true;
> break;
> }
> @@ -1139,9 +1147,13 @@ void uprobe_unregister(struct inode *ino
> return;
>
> down_write(&uprobe->register_rwsem);
> + raw_write_seqcount_begin(&uprobe->register_seq);
> __uprobe_unregister(uprobe, uc);
> + raw_write_seqcount_end(&uprobe->register_seq);
> up_write(&uprobe->register_rwsem);
> put_uprobe(uprobe);
> +
> + synchronize_srcu(&uprobes_srcu);
> }
> EXPORT_SYMBOL_GPL(uprobe_unregister);
[...]
Powered by blists - more mailing lists