On Wed, Jul 10, 2024 at 9:32 AM Oleg Nesterov <o...@redhat.com> wrote:
>
> It doesn't make any sense to have 2 versions of _register(). Note that
> trace_uprobe_enable(), the only user of uprobe_register(), doesn't need
> to check tu->ref_ctr_offset to decide which one should be used, it could
> safely pass ref_ctr_offset == 0 to uprobe_register_refctr().
>
> Add this argument to uprobe_register(), update the callers, and kill
> uprobe_register_refctr().
>
> Signed-off-by: Oleg Nesterov <o...@redhat.com>
> ---
> include/linux/uprobes.h | 9 ++-------
> kernel/events/uprobes.c | 23 +++++------------------
> kernel/trace/bpf_trace.c | 2 +-
> kernel/trace/trace_uprobe.c | 8 ++------
> 4 files changed, 10 insertions(+), 32 deletions(-)
>
LGTM with few nits below.
Acked-by: Andrii Nakryiko <and...@kernel.org>
> /*
> * uprobe_apply - unregister an already registered probe.
> * @inode: the file in which the probe has to be removed.
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index d1daeab1bbc1..467f358c8ce7 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3477,7 +3477,7 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr
> *attr, struct bpf_prog *pr
> &bpf_uprobe_multi_link_lops, prog);
>
> for (i = 0; i < cnt; i++) {
> - err = uprobe_register_refctr(d_real_inode(link->path.dentry),
> + err = uprobe_register(d_real_inode(link->path.dentry),
> uprobes[i].offset,
> uprobes[i].ref_ctr_offset,
> &uprobes[i].consumer);
please adjust indentation here
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index c98e3b3386ba..78a5c40e885a 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -1089,12 +1089,8 @@ static int trace_uprobe_enable(struct trace_uprobe
> *tu, filter_func_t filter)
> tu->consumer.filter = filter;
> tu->inode = d_real_inode(tu->path.dentry);
>
> - if (tu->ref_ctr_offset)
> - ret = uprobe_register_refctr(tu->inode, tu->offset,
> - tu->ref_ctr_offset, &tu->consumer);
> - else
> - ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
> -
> + ret = uprobe_register(tu->inode, tu->offset, tu->ref_ctr_offset,
> + &tu->consumer);
doesn't fit under 100 characters? If it does, please keep as a single line.
> if (ret)
> tu->inode = NULL;
>
> --
> 2.25.1.362.g51ebf55
>
>
