On Tue, Mar 27, 2018 at 6:27 AM, Mathieu Desnoyers <mathieu.desnoy...@efficios.com> wrote: <snip> >>> +static void find_tp(struct tracepoint *tp, void *priv) >>> +{ >>> + struct tp_find_args *args = priv; >>> + >>> + if (!strcmp(tp->name, args->name)) { >>> + WARN_ON_ONCE(args->tp); >>> + args->tp = tp; >> >> I think this runtime check is not needed if it really can't happen >> (linker verifies that already as you mentioned) although I'm not >> opposed if you still want to keep it, because there's no way to break >> out of the outer loop anyway so I guess your call.. > > We can change the outer loop and break from it if needed, that's not > an issue. > >> I would just do: >> >> if (args->tp) >> return; >> >> if find_tp already found the tracepoint. >> >> Tried to also create a duplicate tracepoint and I get: >> CC init/version.o >> AR init/built-in.o >> AR built-in.o >> LD vmlinux.o >> block/blk-core.o:(__tracepoints+0x440): multiple definition of >> `__tracepoint_sched_switch' >> kernel/sched/core.o:(__tracepoints+0x440): first defined here >> Makefile:1032: recipe for target 'vmlinux' failed >> make: *** [vmlinux] Error 1 > > Yeah, as I state in my changelog, I'm very well aware that the linker > is able to catch those. This was the purpose of emitting a > __tracepoint_##name symbol in the tracepoint definition macro. This > WARN_ON_ONCE() is a redundant check for an invariant that we expect > is provided by the linker. Given that it's not a fast path, I would > prefer to keep this redundant check in place, given that an average > factor 2 speedup on a slow path should really not be something we > need to optimize for. IMHO, for slow paths, robustness is more important > than speed (unless the slow path becomes so slow that it really affects > the user). > > I envision that a way to break this invariant would be to compile a > kernel object with gcc -fvisibility=hidden or such. I admit this is > particular scenario is really far fetched, but it illustrates why I > prefer to keep an extra safety net at runtime for linker-based > validations when possible. > > If a faster tracepoint lookup function is needed, we should consider > perfect hashing algorithms done post-build, but so far nobody has > complained about speed of this lookup operation. Anyhow a factor 2 in > the speed of this lookup should really not matter, right ?
Yes, I agree with all the great reasons. So lets do it your way then. thanks, - Joel