Re: [PATCH v2 bpf-next 03/17] bpf: Introduce BPF trampoline

On Nov 7, 2019, at 7:11 PM, Alexei Starovoitov [off-list ref] wrote:

On Thu, Nov 7, 2019 at 5:10 PM Song Liu [off-list ref] wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

+               goto out;
+       tr->selector++;

Shall we do selector-- for unlink?

It's a bit flip. I think it would be more confusing with --

Right.. Maybe should use int instead of u64 for selector?

No, since int can overflow.

I guess it is OK to overflow, no?

overflow is not ok, since transition 0->1 should use nop->call patching
whereas 1->2, 2->3 should use call->call.

In my initial implementation (one I didn't share with anyone) I had
trampoline_mutex taken inside bpf_trampoline_update(). And multiple link()
operation were allowed. The idea was to attach multiple progs and update
trampoline once. But then I realized that I cannot do that since 'unlink +
update' where only 'update' is taking lock will not guarantee success. Since
other 'link' operations can race and 'update' can potentially fail in
arch_prepare_bpf_trampoline() due to new things that 'link' brought in. In that
version (since there several fentry/fexit progs can come in at once) I used
separate 'selector' ticker to pick the side of the page. Once I realized the
issue (to guarantee that unlink+update == always success) I moved mutex all the
way to unlink and link and left 'selector' as-is. Just now I realized that
'selector' can be removed.  fentry_cnt + fexit_cnt can be used instead. This
sum of counters will change 1 bit at a time. Am I right?

Yeah, I think fentry_cnt + fexit_cnt is cleaner.

... and that didn't work.
It's transition that matters. Either need to remember previous sum value
or have separate selector. imo selector is cleaner, so I'm back to that.

Hmm.. is this because of the error handling path?

+	tr->progs_cnt[kind]++;
+	err = bpf_trampoline_update(prog);
+	if (err) {
+		hlist_del(&prog->aux->tramp_hlist);
+		tr->progs_cnt[kind]--;
+	}

Thanks,
Song