[PATCH] Markers : fix reentrancy - Kernel

This is a discussion on [PATCH] Markers : fix reentrancy - Kernel ; Lai Jiangshan wrote : > marker_synchronize_unregister must be called _also_ between unregistration > and destruction the data that unregistration-ed probes need to make sure > there is no caller executing a probe when it's data is destroyed. > Ah, you're ...

+ Reply to Thread
Results 1 to 3 of 3

Thread: [PATCH] Markers : fix reentrancy

  1. [PATCH] Markers : fix reentrancy

    Lai Jiangshan wrote :
    > marker_synchronize_unregister must be called _also_ between unregistration
    > and destruction the data that unregistration-ed probes need to make sure
    > there is no caller executing a probe when it's data is destroyed.
    >


    Ah, you're right. I looked again through your patch description and I
    think using a :


    if (entry->rcu_pending)
    rcu_barrier_sched();

    After each time the markers_mutex is taken would keep the fast
    batch registration/unregistration and fix the reentrancy problem.
    The following patch should address the problem.

    Thanks,

    Mathieu


    Lai Jiangshan discovered a reentrancy issue with markers. This patch implements
    a version of the fix which won't slow down marker batch
    registration/unregistration.

    Signed-off-by: Mathieu Desnoyers
    CC: Ingo Molnar
    CC: Lai Jiangshan
    CC: "Frank Ch. Eigler"
    ---
    kernel/marker.c | 6 ++++++
    1 file changed, 6 insertions(+)

    Index: linux-2.6-lttng/kernel/marker.c
    ================================================== =================
    --- linux-2.6-lttng.orig/kernel/marker.c 2008-09-30 01:29:18.000000000 -0400
    +++ linux-2.6-lttng/kernel/marker.c 2008-09-30 01:31:28.000000000 -0400
    @@ -674,6 +674,8 @@ int marker_probe_register(const char *na
    mutex_lock(&markers_mutex);
    entry = get_marker(name);
    WARN_ON(!entry);
    + if (entry->rcu_pending)
    + rcu_barrier_sched();
    entry->oldptr = old;
    entry->rcu_pending = 1;
    /* write rcu_pending before calling the RCU callback */
    @@ -717,6 +719,8 @@ int marker_probe_unregister(const char *
    entry = get_marker(name);
    if (!entry)
    goto end;
    + if (entry->rcu_pending)
    + rcu_barrier_sched();
    entry->oldptr = old;
    entry->rcu_pending = 1;
    /* write rcu_pending before calling the RCU callback */
    @@ -795,6 +799,8 @@ int marker_probe_unregister_private_data
    mutex_lock(&markers_mutex);
    entry = get_marker_from_private_data(probe, probe_private);
    WARN_ON(!entry);
    + if (entry->rcu_pending)
    + rcu_barrier_sched();
    entry->oldptr = old;
    entry->rcu_pending = 1;
    /* write rcu_pending before calling the RCU callback */

    --
    Mathieu Desnoyers
    OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH] Markers : fix reentrancy


    * Mathieu Desnoyers wrote:

    > Lai Jiangshan wrote :
    > > marker_synchronize_unregister must be called _also_ between unregistration
    > > and destruction the data that unregistration-ed probes need to make sure
    > > there is no caller executing a probe when it's data is destroyed.
    > >

    >
    > Ah, you're right. I looked again through your patch description and I
    > think using a :
    >
    >
    > if (entry->rcu_pending)
    > rcu_barrier_sched();
    >
    > After each time the markers_mutex is taken would keep the fast batch
    > registration/unregistration and fix the reentrancy problem. The
    > following patch should address the problem.


    could you please send a delta patch against tip/master? Lai's patch is
    already applied and is tested. Thanks,

    Ingo
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH] Markers : fix reentrancy

    * Ingo Molnar (mingo@elte.hu) wrote:
    >
    > * Mathieu Desnoyers wrote:
    >
    > > Lai Jiangshan wrote :
    > > > marker_synchronize_unregister must be called _also_ between unregistration
    > > > and destruction the data that unregistration-ed probes need to make sure
    > > > there is no caller executing a probe when it's data is destroyed.
    > > >

    > >
    > > Ah, you're right. I looked again through your patch description and I
    > > think using a :
    > >
    > >
    > > if (entry->rcu_pending)
    > > rcu_barrier_sched();
    > >
    > > After each time the markers_mutex is taken would keep the fast batch
    > > registration/unregistration and fix the reentrancy problem. The
    > > following patch should address the problem.

    >
    > could you please send a delta patch against tip/master? Lai's patch is
    > already applied and is tested. Thanks,
    >
    > Ingo


    Hrm,

    Most of Lai "simplifications" to use synchronize_sched() instead of
    call_rcu_sched() will make batch registration/unregistration much
    slower (very noticeable on ~50 markers on a loaded machine). It also
    contains two different fixes in one, one of which has been nacked.

    The correct list of changes against tip now becomes :

    Revert commit d587284c1d26b796bf52a6e3d3f783db4e462119
    Apply patch "Markers : marker_synchronize_unregister()"
    Apply patch "Markers : probe example fix teardown"
    Apply patch "Markers : documentation fix teardown"
    Apply patch "sputrace : use marker_synchronize_unregister()"
    Apply patch "Markers : fix reentrancy"

    Sorry, but rebasing those marker fixes will just make all those changes
    really messy. I think the cleanest way is to just revert. :-/

    Mathieu
    --
    Mathieu Desnoyers
    OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread