[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEf4BzYzqw7zO1dBXSgh1sQoFtdg2sa5avOch8jJW=_iRJuquQ@mail.gmail.com>
Date: Thu, 1 Aug 2024 09:49:45 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: "Liao, Chang" <liaochang1@...wei.com>
Cc: Andrii Nakryiko <andrii@...nel.org>, linux-trace-kernel@...r.kernel.org,
peterz@...radead.org, oleg@...hat.com, rostedt@...dmis.org,
mhiramat@...nel.org, bpf@...r.kernel.org, linux-kernel@...r.kernel.org,
jolsa@...nel.org, paulmck@...nel.org
Subject: Re: [PATCH 3/8] uprobes: protected uprobe lifetime with SRCU
On Thu, Aug 1, 2024 at 5:23 AM Liao, Chang <liaochang1@...wei.com> 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@...nel.org>
> > ---
> > 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"
>
>
> > - /* 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
Powered by blists - more mailing lists