Messages in this thread | | | Date | Thu, 26 Apr 2018 11:03:02 -0400 (EDT) | From | Mathieu Desnoyers <> | Subject | Re: [RFC v4 3/4] irqflags: Avoid unnecessary calls to trace_ if you can |
| |
----- On Apr 25, 2018, at 6:51 PM, rostedt rostedt@goodmis.org wrote:
> On Wed, 25 Apr 2018 17:40:56 -0400 (EDT) > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > >> One problem with your approach is that you can have multiple callers >> for the same tracepoint name, where some could be non-preemptible and >> others blocking. Also, there is then no clear way for the callback >> registration API to enforce whether the callback expects the tracepoint >> to be blocking or non-preemptible. This can introduce hard to diagnose >> issues in a kernel without debug options enabled. > > I agree that it should not be tied to an implementation name. But > "blocking" is confusing. I would say "can_sleep" or some such name that > states that the trace point caller is indeed something that can sleep.
"trace_*event*_{can,might,may}_sleep" are all acceptable candidates for me.
> >> >> Regarding the name, I'm OK with having something along the lines of >> trace_*event*_blocking or such. Please don't use "srcu" or other naming >> that is explicitly tied to the underlying mechanism used internally >> however: what we want to convey is that this specific tracepoint probe >> can be preempted and block. The underlying implementation could move to >> a different RCU flavor brand in the future, and it should not impact >> users of the tracepoint APIs. >> >> In order to ensure that probes that may block only register themselves >> to tracepoints that allow blocking, we should introduce new tracepoint >> declaration/definition *and* registration APIs also contain the >> "BLOCKING/blocking" keywords (or such), so we can ensure that a >> tracepoint probe being registered to a "blocking" tracepoint is indeed >> allowed to block. > > I'd really don't want to add more declaration/definitions, as we > already have too many as is, and with different meanings and the number > is of incarnations is n! in growth. > > I'd say we just stick with a trace_<event>_can_sleep() call, and make > sure that if that is used that no trace_<event>() call is also used, and > enforce this with linker or compiler tricks.
My main concern is not about having both trace_<event>_can_sleep() mixed with trace_<event>() calls. It's more about having a registration API allowing modules registering probes that may need to sleep to explicitly declare it, and enforce that tracepoint never connects a probe that may need to sleep with an instrumentation site which cannot sleep.
I'm unsure what's the best way to achieve this goal though. We could possibly extend the tracepoint_probe_register_* APIs to introduce e.g. tracepoint_probe_register_prio_flags() and provide a TRACEPOINT_PROBE_CAN_SLEEP as parameter upon registration. If this flag is provided, then we could figure out an way to iterate on all callers, and ensure they are all "can_sleep" type of callers.
Thoughts ?
Thanks,
Mathieu
> > -- Steve
-- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
| |