On Tue, Aug 01, 2023 at 01:43:53PM -0700, Alexei Starovoitov wrote: > On Tue, Aug 1, 2023 at 1:18 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote: > > > > > > > > On 8/1/23 12:44 PM, Yonghong Song wrote: > > > > > > > > > On 8/1/23 4:53 AM, Yafang Shao wrote: > > >> On Tue, Aug 1, 2023 at 3:30 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote: > > >>> > > >>> Adding support for bpf_get_func_ip helper for uprobe program to return > > >>> probed address for both uprobe and return uprobe. > > >>> > > >>> We discussed this in [1] and agreed that uprobe can have special use > > >>> of bpf_get_func_ip helper that differs from kprobe. > > >>> > > >>> The kprobe bpf_get_func_ip returns: > > >>> - address of the function if probe is attach on function entry > > >>> for both kprobe and return kprobe > > >>> - 0 if the probe is not attach on function entry > > >>> > > >>> The uprobe bpf_get_func_ip returns: > > >>> - address of the probe for both uprobe and return uprobe > > >>> > > >>> The reason for this semantic change is that kernel can't really tell > > >>> if the probe user space address is function entry. > > >>> > > >>> The uprobe program is actually kprobe type program attached as uprobe. > > >>> One of the consequences of this design is that uprobes do not have its > > >>> own set of helpers, but share them with kprobes. > > >>> > > >>> As we need different functionality for bpf_get_func_ip helper for > > >>> uprobe, > > >>> I'm adding the bool value to the bpf_trace_run_ctx, so the helper can > > >>> detect that it's executed in uprobe context and call specific code. > > >>> > > >>> The is_uprobe bool is set as true in bpf_prog_run_array_sleepable which > > >>> is currently used only for executing bpf programs in uprobe. > > >> > > >> That is error-prone. If we don't intend to rename > > >> bpf_prog_run_array_sleepable() to bpf_prog_run_array_uprobe(), I think > > >> we'd better introduce a new parameter 'bool is_uprobe' into it. > > > > > > Agree that renaming bpf_prog_run_array_sleepable() to > > > bpf_prog_run_array_uprobe() probably better. This way, it is > > > self-explainable for `run_ctx.is_uprobe = true`. > > > > > > If unlikely case in the future, another sleepable run prog array > > > is needed. They can have their own bpf_prog_run_array_<..> > > > and underlying bpf_prog_run_array_sleepable() can be factored out. > > > > Or if want to avoid unnecessary code churn, at least add > > a comment in bpf_prog_run_array_sleepable() to explain > > that why it is safe to do `run_ctx.is_uprobe = true;`. > > I think renaming to _uprobe() is a good idea. > I would prefer if we can remove the bool is_uprobe run-time check, > but don't see a way to do it cleanly. ok, I'll rename it thanks, jirka
