On Tue, Mar 19, 2024 at 5:48 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Tue, Mar 19, 2024 at 11:19:08AM +0200, Eduard Zingerman wrote:
> > On Mon, 2024-03-18 at 11:40 -0700, Andrii Nakryiko wrote:
> >
> > [...]
> >
> > > @@ -2373,15 +2378,23 @@ static __always_inline
> > > void __bpf_trace_run(struct bpf_raw_tp_link *link, u64 *args)
> > > {
> > > struct bpf_prog *prog = link->link.prog;
> > > + struct bpf_run_ctx *old_run_ctx;
> > > + struct bpf_trace_run_ctx run_ctx;
> >
> > The struct bpf_trace_run_ctx has two fields: bpf_cookie, is_uprobe
> > (there is also run_ctx but it's size is zero).
> > The is_uprobe field is not set by the code below.
> > Is it necessary to zero-init `run_ctx` variable?
no, not really, we need to initialize fields that are going to be
used, and is_uprobe *shouldn't be used*, but see below
> >
>
> I think it's ok because the is_uprobe is used by kprobes/uprobes helpers
> while here it's used for tp programs, so it won't be used
>
yep, that's my understanding and why I didn't add is_uprobe = false,
no need to pay for that (however the cost would be trivial, so if
necessary it's not a problem to add it)
> OTOH it might be cleaner to add special run ctx struct for tp programs,
> (like bpf_tp_run_ctx) to avoid confusion.. but then we'd need new helper
> version for bpf_get_attach_cookie.. perhaps just adding some explaining
> comment will be fine
so when I was implementing this, I didn't want to touch yet more code,
but I felt it wrong that bpf_trace_run_ctx is shared between
kprobe/uprobe and other BPF_PROG_TYPE_TRACE programs (fentry/fexit,
tp_btf) and also perf_event/tracepoint programs. I'd say kprobe/uprobe
should get its own, given they have these extra things like uprobe vs
kprobe flag.
But I'd like to leave it to some future clean ups, and minimize the
amount of changes in this patch set, as I intend to backport it to a
rather old kernel we need this functionality in.
>
> jirka
