On Mon, Apr 06, 2009 at 03:29:43PM +0200, Rafael J. Wysocki wrote:
> On Monday 06 April 2009, Gautham R Shenoy wrote:
> > On Sun, Apr 05, 2009 at 03:44:54PM +0200, Ingo Molnar wrote:
> > >
> > > * Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> > >
> > > > On Sunday 05 April 2009, Ming Lei wrote:
> > > > > kernel version : one simple usb-serial patch against commit
> > > > > 6bb597507f9839b13498781e481f5458aea33620.
> > > > >
> > > > > Thanks.
> > > >
> > > > Hmm, CPU hotplug again, it seems.
> > > >
> > > > I'm not sure who's the maintainer at the moment. Andrew, is that
> > > > Gautham?
> > >
> > > CPU hotplug tends to land on the scheduler people's desk normally.
> > >
> > > But i'm not sure that's the real thing here - key appears to be this
> > > work_on_cpu() worklet by the cpufreq code:
> >
> > Actually, there are two dependency chains here which can lead to a deadlock.
> > The one we're seeing here is the longer of the two.
> >
> > If the relevant locks are numbered as follows:
> > [1]: cpu_policy_rwsem
> > [2]: work_on_cpu
> > [3]: cpu_hotplug.lock
> > [4]: dpm_list_mtx
> >
> >
> > The individual callpaths are:
> >
> > 1) do_dbs_timer()[1] --> dbs_check_cpu() --> __cpufreq_driver_getavg()
> > |
> > work_on_cpu()[2] <-- get_measured_perf() <--|
> >
> >
> > 2) pci_device_probe() --> .. --> pci_call_probe() [3] --> work_on_cpu()[2]
> > |
> > [4] device_pm_add() <-- ..<-- local_pci_probe() <--|
>
> This should block on [4] held by hibernate(). That's why it calls
> device_pm_lock() after all.
Agreed. But it does so holding the cpu_hotplug.lock at pci_call_probe().
See below.
>
> > 3) hibernate() --> hibernatioin_snapshot() --> create_image()
> > |
> > disable_nonboot_cpus() <-- [4] device_pm_lock() <--|
> > |
> > |--> _cpu_down() [3] --> cpufreq_cpu_callback() [1]
> >
> >
> > The two chains which can deadlock are
> >
> > a) [1] --> [2] --> [4] --> [3] --> [1] (The one in this log)
> > and
> > b) [3] --> [2] --> [4] --> [3]
>
> What exactly is the b) scenario?
pci_call_probe() calls work_on_cpu() within
get_online_cpus()/put_online_cpus(), the cpu hotplug read path.
Thus we have a cpu_hotplug.lock --> work_on_cpu dependency here.
This work_on_cpu() calls local_pci_probe() which, in one of the
registration paths calls pcie_portdrv_probe(). This would
eventually end up calling device_pm_add() which takes the
dpm_list_mtx. Thus we have a work_on_cpu --> dpm_list_mtx
dependency here. This is reflected in the lockdep log for dpm_list_mtx:
> > [ 2276.033054] -> #3 (dpm_list_mtx){+.+.+.}:
> > [ 2276.033057] [<ffffffff80265579>] __lock_acquire+0x1402/0x178c
> > [ 2276.033061] [<ffffffff80265996>] lock_acquire+0x93/0xbf
> > [ 2276.033065] [<ffffffff804763db>] mutex_lock_nested+0x6a/0x362
> > [ 2276.033068] [<ffffffff803c4339>] device_pm_add+0x46/0xed
> > [ 2276.033073] [<ffffffff803bdeee>] device_add+0x488/0x61a
> > [ 2276.033077] [<ffffffff803be099>] device_register+0x19/0x1d
> > [ 2276.033080] [<ffffffff8036031a>] pcie_port_device_register+0x450/0x4b6
> > [ 2276.033085] [<ffffffff80469999>] pcie_portdrv_probe+0x69/0x82
> > [ 2276.033089] [<ffffffff8035bf77>] local_pci_probe+0x12/0x16
> > [ 2276.033093] [<ffffffff8024fdf8>] do_work_for_cpu+0x13/0x1b
> > [ 2276.033097] [<ffffffff80250038>] worker_thread+0x1b2/0x2c9
> > [ 2276.033100] [<ffffffff80253d40>] kthread+0x49/0x76
> > [ 2276.033104] [<ffffffff8020c1fa>] child_rip+0xa/0x20
> > [ 2276.033107] [<ffffffffffffffff>] 0xffffffffffffffff
The dependency chain on this device_registration path would be
cpu_hotplug.lock --> work_on_cpu --> dpm_list_mtx.
On the hibernate path, we hold the dpm_list_mtx and call
disable_nonboot_cpus() in create_image().
disable_nonboot_cpus() calls _cpu_down() which again takes the
cpu_hotplug.lock, this time the write-path. Thus we have a
dpm_list_mtx --> cpu_hotplug.lock dependency here.
These two dependency chains being in reverse order can cause a
dead-lock, right ? Or am I reading something wrong here?
>
> >
> > Rafael,
> > Sorry, I am not well versed with the hibernation code. But does the
> > following make sense:
>
> Not really ->
>
> > create_image()
> > {
> > device_pm_lock();
> > device_power_down(PMSG_FREEZE);
> > platform_pre_snapshot(platform_mode);
> >
> > device_pm_unlock();
>
> -> because dpm_list is under control of the hibernation code at this point
> and it should remain locked.
>
> > disable_nonboot_cpus()
>
> disable_nonboot_cpus() must not take dpm_list_mtx itself.
>
> > device_pm_lock();
> > .
> > .
> > .
> > .
> > }
>
> Thanks,
> Rafael
--
Thanks and Regards
gautham
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
