Alexei Starovoitov [off-list ref] writes:

On Thu, Sep 24, 2020 at 3:00 PM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

Alexei Starovoitov [off-list ref] writes:

quoted

quoted

+    struct mutex tgt_mutex; /* protects tgt_* pointers below, *after* prog becomes visible */
+    struct bpf_prog *tgt_prog;
+    struct bpf_trampoline *tgt_trampoline;
     bool verifier_zext; /* Zero extensions has been inserted by verifier. */
     bool offload_requested;
     bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */

...

quoted

 struct bpf_tracing_link {
     struct bpf_link link;
     enum bpf_attach_type attach_type;
+    struct bpf_trampoline *trampoline;
+    struct bpf_prog *tgt_prog;

imo it's confusing to have 'tgt_prog' to mean two different things.
In prog->aux->tgt_prog it means target prog to attach to in the future.
Whereas here it means the existing prog that was used to attached to.
They kinda both 'target progs' but would be good to disambiguate.
May be keep it as 'tgt_prog' here and
rename to 'dest_prog' and 'dest_trampoline' in prog->aux ?

I started changing this as you suggested, but I think it actually makes
the code weirder. We'll end up with a lot of 'tgt_prog =
prog->aux->dest_prog' assignments in the verifier, unless we also rename
all of the local variables, which I think is just code churn for very
little gain (the existing 'target' meaning is quite clear, I think).

you mean "churn" just for this patch. that's fine.
But it will make names more accurate for everyone reading it afterwards.
Hence I prefer distinct and specific names where possible.

quoted

I also think it's quite natural that the target moves; I mean, it's
literally the same pointer being re-assigned from prog->aux to the link.
We could rename the link member to 'attached_tgt_prog' or something like
that, but I'm not sure it helps (and I don't see much of a problem in
the first place).

'attached_tgt_prog' will not be the correct name.
There is 'prog' inside the link already. That's 'attached' prog.
Not this one. This one is the 'attached_to' prog.
But such name would be too long.
imo calling it 'dest_prog' in aux is shorter and more obvious.

Meh, don't really see how it helps ('destination' and 'target' are
literally synonyms). But I don't care enough to bikeshed about it
either, so I'll just do a search/replace...

-Toke