Messages in this thread | | | Date | Tue, 27 Mar 2018 11:40:43 -0400 (EDT) | From | Mathieu Desnoyers <> | Subject | Re: [RFC PATCH] tracepoint: Provide tracepoint_kernel_find_by_name |
| |
----- On Mar 27, 2018, at 11:28 AM, joel opensrc joel.opensrc@gmail.com wrote:
> On Tue, Mar 27, 2018 at 6:27 AM, Mathieu Desnoyers > <mathieu.desnoyers@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.
Alexei proposed an alternative patch. I don't strongly mind one approach or the other.
Thanks,
Mathieu
> > thanks, > > - Joel
-- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
| |