lkml.org 
[lkml]   [2009]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    Subject[PATCH] cpufreq: remove rwsem lock from CPUFREQ_GOV_STOP call
    * Rafael J. Wysocki (rjw@sisk.pl) wrote:
    > This message has been generated automatically as a part of a report
    > of regressions introduced between 2.6.28 and 2.6.29.
    >
    > The following bug entry is on the current list of known regressions
    > introduced between 2.6.28 and 2.6.29. Please verify if it still should
    > be listed and let me know (either way).
    >
    >
    > Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=13186
    > Subject : cpufreq timer teardown problem
    > Submitter : Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
    > Date : 2009-04-23 14:00 (24 days old)
    > References : http://marc.info/?l=linux-kernel&m=124049523515036&w=4
    > Handled-By : Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
    > Patch : http://patchwork.kernel.org/patch/19754/
    > http://patchwork.kernel.org/patch/19753/

    The patches linked above depend on the following patch to remove
    circular locking dependency :


    cpufreq: remove rwsem lock from CPUFREQ_GOV_STOP call

    (the following issue was faced when using cancel_delayed_work_sync() in the
    timer teardown (which fixes a race).

    * KOSAKI Motohiro (kosaki.motohiro@jp.fujitsu.com) wrote:
    > Hi
    >
    > my box output following warnings.
    > it seems regression by commit 7ccc7608b836e58fbacf65ee4f8eefa288e86fac.
    >
    > A: work -> do_dbs_timer() -> cpu_policy_rwsem
    > B: store() -> cpu_policy_rwsem -> cpufreq_governor_dbs() -> work
    >
    >

    Hrm, I think it must be due to my attempt to fix the timer teardown race
    in ondemand governor mixed with new locking behavior in 2.6.30-rc.

    The rwlock seems to be taken around the whole call to
    cpufreq_governor_dbs(), when it should be only taken around accesses to
    the locked data, and especially *not* around the call to
    dbs_timer_exit().

    Reverting my fix attempt would put the teardown race back in place
    (replacing the cancel_delayed_work_sync by cancel_delayed_work).
    Instead, a proper fix would imply modifying this critical section :

    cpufreq.c: __cpufreq_remove_dev()
    ...
    if (cpufreq_driver->target)
    __cpufreq_governor(data, CPUFREQ_GOV_STOP);

    unlock_policy_rwsem_write(cpu);

    To make sure the __cpufreq_governor() callback is not called with rwsem
    held. This would allow execution of cancel_delayed_work_sync() without
    being nested within the rwsem.

    Applies on top of the 2.6.30-rc5 tree.

    Required to remove circular dep in teardown of both conservative and
    ondemande governors so they can use cancel_delayed_work_sync().
    CPUFREQ_GOV_STOP does not modify the policy, therefore this locking seemed
    unneeded.

    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
    CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
    Cc: Greg KH <greg@kroah.com>
    CC: Ingo Molnar <mingo@elte.hu>
    CC: "Rafael J. Wysocki" <rjw@sisk.pl>
    CC: Ben Slusky <sluskyb@paranoiacs.org>
    CC: Dave Jones <davej@redhat.com>
    CC: Chris Wright <chrisw@sous-sol.org>
    CC: Andrew Morton <akpm@linux-foundation.org>
    ---
    drivers/cpufreq/cpufreq.c | 4 ++--
    1 file changed, 2 insertions(+), 2 deletions(-)

    Index: linux-2.6-lttng/drivers/cpufreq/cpufreq.c
    ===================================================================
    --- linux-2.6-lttng.orig/drivers/cpufreq/cpufreq.c 2009-05-10 14:41:53.000000000 -0400
    +++ linux-2.6-lttng/drivers/cpufreq/cpufreq.c 2009-05-10 14:42:29.000000000 -0400
    @@ -1070,11 +1070,11 @@ static int __cpufreq_remove_dev(struct s
    spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
    #endif

    + unlock_policy_rwsem_write(cpu);
    +
    if (cpufreq_driver->target)
    __cpufreq_governor(data, CPUFREQ_GOV_STOP);

    - unlock_policy_rwsem_write(cpu);
    -
    kobject_put(&data->kobj);

    /* we need to make sure that the underlying kobj is actually
    --
    Mathieu Desnoyers
    OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68


    \
     
     \ /
      Last update: 2009-05-17 16:27    [W:0.037 / U:89.544 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site