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.
