On Mon, Aug 5, 2024 at 7:52 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote: > > LGTM, just a few notes... > > On 07/31, Andrii Nakryiko wrote: > > > > @@ -59,6 +61,7 @@ struct uprobe { > > struct list_head pending_list; > > struct uprobe_consumer *consumers; > > struct inode *inode; /* Also hold a ref to inode */ > > + struct rcu_head rcu; > > you can probably put the new member into the union with, say, rb_node. yep, good point, will do > > > @@ -1945,9 +1950,14 @@ pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr) > > if (!utask) > > return -ENOMEM; > > > > + if (!try_get_uprobe(uprobe)) > > + return -EINVAL; > > + > > a bit off-topic right now, but it seems that we can simply kill > utask->active_uprobe. We can turn into into "bool has_active_uprobe" > and copy uprobe->arch into uprobe_task. Lets discuss this later. I'm going to change this active_uprobe thing to be either refcounted or SRCU-protected (but with timeout), so I'll need a bit more structure around this. Let's see how that lands and if we still can get rid of it, we can discuss. > > > @@ -2201,13 +2215,15 @@ static void handle_swbp(struct pt_regs *regs) > > { > > struct uprobe *uprobe; > > unsigned long bp_vaddr; > > - int is_swbp; > > + int is_swbp, srcu_idx; > > > > bp_vaddr = uprobe_get_swbp_addr(regs); > > if (bp_vaddr == uprobe_get_trampoline_vaddr()) > > return uprobe_handle_trampoline(regs); > > > > - uprobe = find_active_uprobe(bp_vaddr, &is_swbp); > > + srcu_idx = srcu_read_lock(&uprobes_srcu); > > + > > + uprobe = find_active_uprobe_rcu(bp_vaddr, &is_swbp); > > if (!uprobe) { > > if (is_swbp > 0) { > > /* No matching uprobe; signal SIGTRAP. */ > > @@ -2223,6 +2239,7 @@ static void handle_swbp(struct pt_regs *regs) > > */ > > instruction_pointer_set(regs, bp_vaddr); > > } > > + srcu_read_unlock(&uprobes_srcu, srcu_idx); > > return; > > Why not > goto out; > > ? > Good point, can be goto out, will change. > Oleg. >