Messages in this thread | | | Date | Fri, 17 Dec 2010 12:51:44 -0800 | From | David Daney <> | Subject | Re: [PATCH/RFC 1/2] jump label: make enable/disable o(1) |
| |
On 12/17/2010 12:07 PM, Jason Baron wrote: > On Thu, Dec 16, 2010 at 09:56:25PM +0100, Peter Zijlstra wrote: >> On Thu, 2010-12-16 at 15:50 -0500, Mathieu Desnoyers wrote: >>> * Peter Zijlstra (peterz@infradead.org) wrote: >>>> On Thu, 2010-12-16 at 15:36 -0500, Mathieu Desnoyers wrote: >>>>> Tracepoints keep their own reference counts for enable/disable, so a >>>>> simple "enable/disable" is fine as far as tracepoints are concerned. Why >>>>> does perf need that refcounting done by the static jumps ? >>>> >>>> Because the refcount is all we have... Why not replace that tracepoint >>>> refcount with the jumplabel thing? >>> >>> The reason why tracepoints need to keep their own refcount is because >>> they support dynamically loadable modules, and hence the refcount must >>> be kept outside of the modules, in a table internal to tracepoints, >>> so we can attach a probe to a yet unloaded module. Therefore, relying on >>> this lower level jump label to keep the refcount is not appropriate for >>> tracepoints, because the refcount only exists when the module is live. >> >> That's not a logical conclusion, you can keep these jump_label keys >> outside of the module just fine. >> >>> I know that your point of view is "let users of modules suffer", but >>> this represents a very large portion of Linux users I am not willing to >>> let suffer knowingly. >> >> Feh, I'd argue to remove this special tracepoint crap, the only >> in-kernel user (ftrace) doesn't even make use of it. This weird ass >> tracepoint semantic being different from the ftrace trace_event >> semantics has caused trouble before. >> >> > > Hi, > > since atomic_t is just an 'int' from include/linux/types.h, so for all > arches. We can cast any refernces to an atomic_t in > include/linux/jump_label_ref.h >
Not acceptable I would think.
How about:
union fubar { int key_as_non_atomic; atomic_t key_as_atomic; };
Now explain the exact semantics of this thing including how you guarantee no conflicting accesses *ever* occur.
> So for when jump labels are disabled case we could have > one struct: > > struct jump_label_key { > int state; > } > > and then we could then have (rough c code): > > jump_label_enable(struct jump_label_key *key) > { > key->state = 1; > } > > jump_label_disable(struct jump_label_key *key) > { > key->state = 0; > } > > jump_label_inc(struct jump_label_key *key) > { > atomic_inc((atomic_t *)key) > } > > jump_label_dec(struct jump_label_key *key) > { > atomic_dec((atomic_t *)key) > } > > bool unlikely_switch(struct jump_label_key *key) > { > if (key->state) > return true; > return false; > } > > bool unlikely_switch_atomic(struct jump_label_key *key) > { > if (atomic_read((atomic_t *)key) > return true; > return false; > } > > can we agree on something like this?
I get a sick feeling whenever casting is used to give types with well defined semantics (atomic_t) poorly defined semantics (your usage).
David Daney
| |