From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Young Subject: Re: [PATCH] remove rwsem lock from CPUFREQ_GOV_STOP call (second call site) Date: Tue, 9 Jun 2009 09:15:31 +0800 Message-ID: References: <84144f020906070621r1f480eaeief026d23662df380@mail.gmail.com> <1244447366.13471.4.camel@penberg-laptop> <20090608124844.GA17588@Krystal> <20090608143220.GC2516@redhat.com> <20090608152316.GA21033@Krystal> Mime-Version: 1.0 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=Rd/nClE/O+l6eSggLUZB2RLfvudcKFENgGAD9IQ01mI=; b=LGiTdIFNpC98F3d3cIv3ojWglmnQyqUpvJQFFyXvjsLX4LHspHgtzSX1xEYCG5uMPr Fnqd9ZHM4iRfyuedRfasHX/8/5YRZhOIKoH2JFjqDFO/IXmE/8oPT423cor278chzpPd uK+AbbcDFdWv0iZu3BCEoDjwVyAKosB/2yCl8= In-Reply-To: <20090608152316.GA21033@Krystal> Sender: kernel-testers-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Content-Type: text/plain; charset="utf-8" To: Mathieu Desnoyers Cc: Dave Jones , Pekka Enberg , "Rafael J. Wysocki" , Linux Kernel Mailing List , Kernel Testers List , cpufreq-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rusty Russell , trenn-l3A5Bk7waGM@public.gmane.org, sven.wegener-sQQoR7IzGU7R7s880joybQ@public.gmane.org, Venkatesh Pallipadi , mingo-X9Un+BFzKDI@public.gmane.org, Shaohua Li On Mon, Jun 8, 2009 at 11:23 PM, Mathieu Desnoyers wrote: > * Dave Jones (davej-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org) wrote: >> On Mon, Jun 08, 2009 at 08:48:45AM -0400, Mathieu Desnoyers wrote: >> >> =C2=A0> > > >> Bug-Entry =C2=A0 =C2=A0 =C2=A0 : http://bugzilla.kern= el.org/show_bug.cgi?id=3D13475 >> =C2=A0> > > >> Subject =C2=A0 =C2=A0 =C2=A0 =C2=A0 : suspend/hiberna= te lockdep warning >> =C2=A0> > > >> References =C2=A0 =C2=A0 =C2=A0: http://marc.info/?l=3D= linux-kernel&m=3D124393723321241&w=3D4 >> =C2=A0> > > >> =C2=A0> > > I suspect the following commit, after revert this patch = I test 5 times >> =C2=A0> > > without lockdep warnings. >> =C2=A0> > > >> =C2=A0> > > commit b14893a62c73af0eca414cfed505b8c09efc613c >> =C2=A0> > > Author: Mathieu Desnoyers >> =C2=A0> > > Date: =C2=A0 Sun May 17 10:30:45 2009 -0400 >> =C2=A0> > > >> =C2=A0> > > =C2=A0 =C2=A0 =C2=A0 =C2=A0[CPUFREQ] fix timer teardown = in ondemand governor >> =C2=A0> > >> =C2=A0> > The patch is probably not at fault here. I suspect it's so= me latent bug >> =C2=A0> > that simply got exposed by the change to cancel_delayed_wo= rk_sync(). In >> =C2=A0> > any case, Mathieu, can you take a look at this please? >> =C2=A0> >> =C2=A0> Yes, it's been looked at and discussed on the cpufreq ML. Th= e short >> =C2=A0> answer is that they plan to re-engineer cpufreq and remove t= he policy >> =C2=A0> rwlock taken around almost every operations at the cpufreq l= evel. >> =C2=A0> >> =C2=A0> The short-term solution, which is recognised as ugly, would = be do to the >> =C2=A0> following before doing the cancel_delayed_work_sync() : >> =C2=A0> >> =C2=A0> unlock policy rwlock write lock >> =C2=A0> >> =C2=A0> lock policy rwlock write lock >> =C2=A0> >> =C2=A0> It basically works because this rwlock is unneeded for teard= own, hence >> =C2=A0> the future re-work planned. >> =C2=A0> >> =C2=A0> I'm sorry I cannot prepare a patch current... I've got quite= a few pages >> =C2=A0> of Ph.D. thesis due for the beginning of July. >> >> I'm kinda scared to touch this code at all for .30 due to the number= of >> unexpected gotchas we seem to run into every time we touch something >> locking related. =C2=A0So I'm inclined to just live with the lockdep= warning >> for .30, and see how the real fixes look for .31, and push them back >> as -stable updates if they work out. >> >> >> Venki, what are your thoughts? >> > > Hi Dave, > > I've looked through the cpufreq code, and the following patch should > address the call site I've missed in commit > 42a06f2166f2f6f7bf04f32b4e823eacdceafdc9. I've followed all > __cpufreq_set_policy call sites within cpufreq.c to make sure they al= l > hold the rwsem write lock. An extra round of review would be good > though. > > Can someone try the following patch and see if it fixes the regressio= n ? Bad news, I have tried the patch and It does not fix the regression. > My test machine is currently busy running long formal verifications, = and > therefore unavailable for kernel patch testing. It compiles fine on a > 2.6.30-rc5 kernel with my (now mainlined) cpufreq patches applied. > > Mathieu > > > remove rwsem lock from CPUFREQ_GOV_STOP call (second call site) > > commit =C2=A042a06f2166f2f6f7bf04f32b4e823eacdceafdc9 > > Missed a call site for CPUFREQ_GOV_STOP to remove the rwlock taken ar= ound the > teardown. To make a long story short, the rwlock write-lock causes a = circular > dependency with cancel_delayed_work_sync(), because the timer handler= takes the > read lock. > > Note that all callers to __cpufreq_set_policy are taking the rwsem. A= ll sysfs > callers (writers) hold the write rwsem at the earliest sysfs calling = stage. > > However, the rwlock write-lock is not needed upon governor stop. > > Signed-off-by: Mathieu Desnoyers > CC: rjw-KKrjLPT3xs0@public.gmane.org > CC: mingo-X9Un+BFzKDI@public.gmane.org > CC: Shaohua Li > CC: Pekka Enberg > CC: Dave Young > CC: "Rafael J. Wysocki" > CC: Rusty Russell > CC: trenn-l3A5Bk7waGM@public.gmane.org > CC: sven.wegener-sQQoR7IzGU7R7s880joybQ@public.gmane.org > CC: Venkatesh Pallipadi > CC: cpufreq-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > --- > =C2=A0drivers/cpufreq/cpufreq.c | =C2=A0 11 ++++++++++- > =C2=A01 file changed, 10 insertions(+), 1 deletion(-) > > Index: linux-2.6-lttng/drivers/cpufreq/cpufreq.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- linux-2.6-lttng.orig/drivers/cpufreq/cpufreq.c =C2=A0 =C2=A0 =C2=A0= 2009-06-08 10:20:48.000000000 -0400 > +++ linux-2.6-lttng/drivers/cpufreq/cpufreq.c =C2=A0 2009-06-08 10:48= :52.000000000 -0400 > @@ -1697,8 +1697,17 @@ static int __cpufreq_set_policy(struct c > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0dprintk("governor switch\n"); > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0/* end old governor */ > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 if (data->governor) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 if (data->governor) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Need to release the rwsem a= round governor > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* stop due to lock dependency= between > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* cancel_delayed_work_sync an= d the read lock > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* taken in the delayed work h= andler. > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 unlock_policy_rwsem_write(data->cpu= ); > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0__cpufreq_governor(data, CPUF= REQ_GOV_STOP); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 lock_policy_rwsem_write(data->cpu); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 } > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0/* start new governor */ > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0data->governor =3D policy->governor; > > > -- > Mathieu Desnoyers > OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F =C2=A0BA06 3F25 A8F= E 3BAE 9A68 > -- > To unsubscribe from this list: send the line "unsubscribe linux-kerne= l" in > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.ht= ml > Please read the FAQ at =C2=A0http://www.tux.org/lkml/ > --=20 Regards dave From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756249AbZFIBPl (ORCPT ); Mon, 8 Jun 2009 21:15:41 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751536AbZFIBPd (ORCPT ); Mon, 8 Jun 2009 21:15:33 -0400 Received: from mail-ew0-f210.google.com ([209.85.219.210]:47324 "EHLO mail-ew0-f210.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751432AbZFIBPc convert rfc822-to-8bit (ORCPT ); Mon, 8 Jun 2009 21:15:32 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=VmX+cNy+OyjBl3WCCDuMOv19cGswiX37l+/sn4hZdk0rKJYpt8RLpSLzsjXt1VuaWs /p3u6BiVWlEW1FXAGaMqowTABfXBR8OfnAUH8FKM1kT1eYR3f+BbuaMCrz2/X0C0E6Y6 w8K6uZ3judcdSfOKFyMlmRCS7kPN8EQrHOb+Q= MIME-Version: 1.0 In-Reply-To: <20090608152316.GA21033@Krystal> References: <84144f020906070621r1f480eaeief026d23662df380@mail.gmail.com> <1244447366.13471.4.camel@penberg-laptop> <20090608124844.GA17588@Krystal> <20090608143220.GC2516@redhat.com> <20090608152316.GA21033@Krystal> Date: Tue, 9 Jun 2009 09:15:31 +0800 Message-ID: Subject: Re: [PATCH] remove rwsem lock from CPUFREQ_GOV_STOP call (second call site) From: Dave Young To: Mathieu Desnoyers Cc: Dave Jones , Pekka Enberg , "Rafael J. Wysocki" , Linux Kernel Mailing List , Kernel Testers List , cpufreq@vger.kernel.org, Rusty Russell , trenn@suse.de, sven.wegener@stealer.net, Venkatesh Pallipadi , mingo@elte.hu, Shaohua Li Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jun 8, 2009 at 11:23 PM, Mathieu Desnoyers wrote: > * Dave Jones (davej@redhat.com) wrote: >> On Mon, Jun 08, 2009 at 08:48:45AM -0400, Mathieu Desnoyers wrote: >> >>  > > > >> Bug-Entry       : http://bugzilla.kernel.org/show_bug.cgi?id=13475 >>  > > > >> Subject         : suspend/hibernate lockdep warning >>  > > > >> References      : http://marc.info/?l=linux-kernel&m=124393723321241&w=4 >>  > > > >>  > > > I suspect the following commit, after revert this patch I test 5 times >>  > > > without lockdep warnings. >>  > > > >>  > > > commit b14893a62c73af0eca414cfed505b8c09efc613c >>  > > > Author: Mathieu Desnoyers >>  > > > Date:   Sun May 17 10:30:45 2009 -0400 >>  > > > >>  > > >        [CPUFREQ] fix timer teardown in ondemand governor >>  > > >>  > > The patch is probably not at fault here. I suspect it's some latent bug >>  > > that simply got exposed by the change to cancel_delayed_work_sync(). In >>  > > any case, Mathieu, can you take a look at this please? >>  > >>  > Yes, it's been looked at and discussed on the cpufreq ML. The short >>  > answer is that they plan to re-engineer cpufreq and remove the policy >>  > rwlock taken around almost every operations at the cpufreq level. >>  > >>  > The short-term solution, which is recognised as ugly, would be do to the >>  > following before doing the cancel_delayed_work_sync() : >>  > >>  > unlock policy rwlock write lock >>  > >>  > lock policy rwlock write lock >>  > >>  > It basically works because this rwlock is unneeded for teardown, hence >>  > the future re-work planned. >>  > >>  > I'm sorry I cannot prepare a patch current... I've got quite a few pages >>  > of Ph.D. thesis due for the beginning of July. >> >> I'm kinda scared to touch this code at all for .30 due to the number of >> unexpected gotchas we seem to run into every time we touch something >> locking related.  So I'm inclined to just live with the lockdep warning >> for .30, and see how the real fixes look for .31, and push them back >> as -stable updates if they work out. >> >> >> Venki, what are your thoughts? >> > > Hi Dave, > > I've looked through the cpufreq code, and the following patch should > address the call site I've missed in commit > 42a06f2166f2f6f7bf04f32b4e823eacdceafdc9. I've followed all > __cpufreq_set_policy call sites within cpufreq.c to make sure they all > hold the rwsem write lock. An extra round of review would be good > though. > > Can someone try the following patch and see if it fixes the regression ? Bad news, I have tried the patch and It does not fix the regression. > My test machine is currently busy running long formal verifications, and > therefore unavailable for kernel patch testing. It compiles fine on a > 2.6.30-rc5 kernel with my (now mainlined) cpufreq patches applied. > > Mathieu > > > remove rwsem lock from CPUFREQ_GOV_STOP call (second call site) > > commit  42a06f2166f2f6f7bf04f32b4e823eacdceafdc9 > > Missed a call site for CPUFREQ_GOV_STOP to remove the rwlock taken around the > teardown. To make a long story short, the rwlock write-lock causes a circular > dependency with cancel_delayed_work_sync(), because the timer handler takes the > read lock. > > Note that all callers to __cpufreq_set_policy are taking the rwsem. All sysfs > callers (writers) hold the write rwsem at the earliest sysfs calling stage. > > However, the rwlock write-lock is not needed upon governor stop. > > Signed-off-by: Mathieu Desnoyers > CC: rjw@sisk.pl > CC: mingo@elte.hu > CC: Shaohua Li > CC: Pekka Enberg > CC: Dave Young > CC: "Rafael J. Wysocki" > CC: Rusty Russell > CC: trenn@suse.de > CC: sven.wegener@stealer.net > CC: Venkatesh Pallipadi > CC: cpufreq@vger.kernel.org > --- >  drivers/cpufreq/cpufreq.c |   11 ++++++++++- >  1 file changed, 10 insertions(+), 1 deletion(-) > > Index: linux-2.6-lttng/drivers/cpufreq/cpufreq.c > =================================================================== > --- linux-2.6-lttng.orig/drivers/cpufreq/cpufreq.c      2009-06-08 10:20:48.000000000 -0400 > +++ linux-2.6-lttng/drivers/cpufreq/cpufreq.c   2009-06-08 10:48:52.000000000 -0400 > @@ -1697,8 +1697,17 @@ static int __cpufreq_set_policy(struct c >                        dprintk("governor switch\n"); > >                        /* end old governor */ > -                       if (data->governor) > +                       if (data->governor) { > +                               /* > +                                * Need to release the rwsem around governor > +                                * stop due to lock dependency between > +                                * cancel_delayed_work_sync and the read lock > +                                * taken in the delayed work handler. > +                                */ > +                               unlock_policy_rwsem_write(data->cpu); >                                __cpufreq_governor(data, CPUFREQ_GOV_STOP); > +                               lock_policy_rwsem_write(data->cpu); > +                       } > >                        /* start new governor */ >                        data->governor = policy->governor; > > > -- > 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/ > -- Regards dave