[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240805155931.GC11049@redhat.com>
Date: Mon, 5 Aug 2024 17:59:32 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Andrii Nakryiko <andrii@...nel.org>
Cc: linux-trace-kernel@...r.kernel.org, peterz@...radead.org,
rostedt@...dmis.org, mhiramat@...nel.org, bpf@...r.kernel.org,
linux-kernel@...r.kernel.org, jolsa@...nel.org, paulmck@...nel.org
Subject: Re: [PATCH 5/8] uprobes: travers uprobe's consumer list locklessly
under SRCU protection
On 07/31, Andrii Nakryiko wrote:
>
> @@ -1120,17 +1098,19 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
> int err;
>
> down_write(&uprobe->register_rwsem);
> - if (WARN_ON(!consumer_del(uprobe, uc))) {
> - err = -ENOENT;
OK, I agree, this should never happen.
But if you remove this check, then
> int uprobe_apply(struct uprobe *uprobe, struct uprobe_consumer *uc, bool add)
> {
> struct uprobe_consumer *con;
> - int ret = -ENOENT;
> + int ret = -ENOENT, srcu_idx;
>
> down_write(&uprobe->register_rwsem);
> - for (con = uprobe->consumers; con && con != uc ; con = con->next)
> - ;
> - if (con)
> - ret = register_for_each_vma(uprobe, add ? uc : NULL);
> +
> + srcu_idx = srcu_read_lock(&uprobes_srcu);
> + list_for_each_entry_srcu(con, &uprobe->consumers, cons_node,
> + srcu_read_lock_held(&uprobes_srcu)) {
> + if (con == uc) {
> + ret = register_for_each_vma(uprobe, add ? uc : NULL);
> + break;
> + }
> + }
we can probably remove the similar check above?
I mean, why do we need the list_for_each_entry_srcu() above? Is it possible
that uprobe_apply(uprobe, uc) is called when "uc" is not on the ->consumers
list?
At first glance I see no problems in this patch... but you know, my eyes are
already blurring, I'll continue tomorrow and read this patch again.
Oleg.
Powered by blists - more mailing lists