在 2024/8/2 0:49, Andrii Nakryiko 写道: > On Thu, Aug 1, 2024 at 5:23 AM Liao, Chang <liaochang1@xxxxxxxxxx> wrote: >> >> >> >> 在 2024/8/1 5:42, Andrii Nakryiko 写道: >>> To avoid unnecessarily taking a (brief) refcount on uprobe during >>> breakpoint handling in handle_swbp for entry uprobes, make find_uprobe() >>> not take refcount, but protect the lifetime of a uprobe instance with >>> RCU. This improves scalability, as refcount gets quite expensive due to >>> cache line bouncing between multiple CPUs. >>> >>> Specifically, we utilize our own uprobe-specific SRCU instance for this >>> RCU protection. put_uprobe() will delay actual kfree() using call_srcu(). >>> >>> For now, uretprobe and single-stepping handling will still acquire >>> refcount as necessary. We'll address these issues in follow up patches >>> by making them use SRCU with timeout. >>> >>> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx> >>> --- >>> kernel/events/uprobes.c | 93 ++++++++++++++++++++++++----------------- >>> 1 file changed, 55 insertions(+), 38 deletions(-) >>> > > [...] > >>> >>> @@ -2258,12 +2275,12 @@ static void handle_swbp(struct pt_regs *regs) >>> if (arch_uprobe_skip_sstep(&uprobe->arch, regs)) >>> goto out; >>> >>> - if (!pre_ssout(uprobe, regs, bp_vaddr)) >>> - return; >>> + if (pre_ssout(uprobe, regs, bp_vaddr)) >>> + goto out; >>> >> >> Regardless what pre_ssout() returns, it always reach the label 'out', so the >> if block is unnecessary. > > yep, I know, but I felt like > > if (something is wrong) > goto out; > > pattern was important to keep for each possible failing step for consistency. > > so unless this is a big deal, I'd keep it as is, as in the future > there might be some other steps after pre_ssout() before returning, so > this is a bit more "composable" > OK, I would say this conditional-check pattern is likely to be optimized away by modern compiler. Thanks. > >> >> >>> - /* arch_uprobe_skip_sstep() succeeded, or restart if can't singlestep */ >>> out: >>> - put_uprobe(uprobe); >>> + /* arch_uprobe_skip_sstep() succeeded, or restart if can't singlestep */ >>> + srcu_read_unlock(&uprobes_srcu, srcu_idx); >>> } >>> >>> /* >> >> -- >> BR >> Liao, Chang -- BR Liao, Chang
