lkml.org 
[lkml]   [2010]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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



    \
     
     \ /
      Last update: 2010-12-17 21:55    [W:0.037 / U:0.504 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site