Re: [PATCH v2 05/12] uprobes: move offset and ref_ctr_offset into uprobe_consumer

[Oleg Nesterov <oleg@redhat.com>]

On 07/01, Andrii Nakryiko wrote:
>
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -42,6 +42,11 @@ struct uprobe_consumer {
>  				enum uprobe_filter_ctx ctx,
>  				struct mm_struct *mm);
>  
> +	/* associated file offset of this probe */
> +	loff_t offset;
> +	/* associated refctr file offset of this probe, or zero */
> +	loff_t ref_ctr_offset;
> +	/* for internal uprobe infra use, consumers shouldn't touch fields below */
>  	struct uprobe_consumer *next;


Well, I don't really like this patch either...

If nothing else because all the consumers in uprobe->consumers list
must have the same offset/ref_ctr_offset.

--------------------------------------------------------------------------
But I agree, the ugly uprobe_register_refctr() must die, we need a single
function

	int uprobe_register(inode, offset, ref_ctr_offset, consumer);

This change is trivial.

--------------------------------------------------------------------------
And speaking of cleanups, I think another change makes sense:

	-	int uprobe_register(...);
	+	struct uprobe* uprobe_register(...);

so that uprobe_register() returns uprobe or ERR_PTR.

	-	void uprobe_unregister(inode, offset, consumer);
	+	void uprobe_unregister(uprobe, consumer);

this way unregister() doesn't need the extra find_uprobe() + put_uprobe().
The same for uprobe_apply().

The necessary changes in kernel/trace/trace_uprobe.c are trivial, we just
need to change struct trace_uprobe

	-	struct inode                    *inode;
	+	struct uprobe			*uprobe;

and fix the compilation errors.


As for kernel/trace/bpf_trace.c, I guess struct bpf_uprobe  needs the new
->uprobe member, we can't kill bpf_uprobe->offset because of
bpf_uprobe_multi_link_fill_link_info(), but I think this is not that bad.

What do you think?

Oleg.