| Message ID | 20150206141346.GP21418@twins.programming.kicks-ass.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/06/2015 04:13 PM, Peter Zijlstra wrote: > On Fri, Feb 06, 2015 at 02:48:34PM +0200, Peter Ujfalusi wrote: >> Hi, >> >> In case when hwmods are used in nested way the lockdep validator will print out >> a warning message about possible deadlock situation: >> >> [ 4.514882] ============================================= >> [ 4.520530] [ INFO: possible recursive locking detected ] >> [ 4.526176] 3.14.30-00289-ge44872fdca8f-dirty #198 Not tainted >> [ 4.532285] --------------------------------------------- >> [ 4.537936] kworker/u4:1/18 is trying to acquire lock: >> [ 4.543317] (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58 >> [ 4.552109] >> [ 4.552109] but task is already holding lock: >> [ 4.558216] (&(&oh->_lock)->rlock){......}, at: [<c002d2dc>] omap_hwmod_enable+0x2c/0x58 >> [ 4.566999] >> [ 4.566999] other info that might help us debug this: >> [ 4.573831] Possible unsafe locking scenario: >> [ 4.573831] >> [ 4.580025] CPU0 >> [ 4.582584] ---- >> [ 4.585142] lock(&(&oh->_lock)->rlock); >> [ 4.589544] lock(&(&oh->_lock)->rlock); >> [ 4.593945] >> [ 4.593945] *** DEADLOCK *** >> [ 4.593945] >> [ 4.600146] May be due to missing lock nesting notation >> >> What lockdep did not realizes is that the two oh is not the same hwmod object >> and we have taken different locks. >> >> One example of nested hwmod usage is on DRA7xx platforms, where McASP can be >> configured to use ATL clock as it's functional clock. In this case the >> pm_runtime_get/put_sync call will enable the mcasp hwmod and as part of this >> operation it will enable the needed clocks. Since ATL clock is needed, we will >> have another pm_runtime operation from the ATL clock enable callback which >> will take the atl hwmod's lock. This will be seen by lockdep as possible >> deadlock situation. >> >> In order to notify lockdep about this, we need to switch using _nested >> version of locking function and assign different subclass to those hwmods which >> could be used in nested way. > > IFF struct omap_hwmod is always statically allocated; as I think the > __init on _register() mandates, you can use the below annotation. > > --- > arch/arm/mach-omap2/omap_hwmod.c | 1 + > arch/arm/mach-omap2/omap_hwmod.h | 2 ++ > 2 files changed, 3 insertions(+) > > diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c > index 9025ffffd2dc..222d654ad6fd 100644 > --- a/arch/arm/mach-omap2/omap_hwmod.c > +++ b/arch/arm/mach-omap2/omap_hwmod.c > @@ -2698,6 +2698,7 @@ static int __init _register(struct omap_hwmod *oh) > INIT_LIST_HEAD(&oh->master_ports); > INIT_LIST_HEAD(&oh->slave_ports); > spin_lock_init(&oh->_lock); > + lockdep_set_class(&oh->_lock, &oh->hwmod_key); > > oh->_state = _HWMOD_STATE_REGISTERED; > > diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h > index 5b42fafcaf55..754bdfb3f811 100644 > --- a/arch/arm/mach-omap2/omap_hwmod.h > +++ b/arch/arm/mach-omap2/omap_hwmod.h > @@ -689,6 +689,8 @@ struct omap_hwmod { > u8 _state; > u8 _postsetup_state; > struct omap_hwmod *parent_hwmod; > + > + struct lock_class_key hwmod_key; > }; > > struct omap_hwmod *omap_hwmod_lookup(const char *name); Certainly looks much simpler, but it adds quite a bit of data to the omap_hwmod struct, and we have a _lot_ of them for omap2plus configuration. ls -al vmlinux w/o any the lockdep warning fixes: 110109168 With my series applied: 110112031 (base + 2863) With setting individual lockdep class: 110114275 (base + 5107) I certainly like the lockdep_set_class() way since it is cleaner, but it adds almost double amount of bytes to the kernel. I'll test the patch on my board tomorrow as first thing.
On Fri, Feb 06, 2015 at 06:05:32PM +0200, Peter Ujfalusi wrote: > Certainly looks much simpler, but it adds quite a bit of data to the > omap_hwmod struct, and we have a _lot_ of them for omap2plus configuration. > > ls -al vmlinux > > w/o any the lockdep warning fixes: > 110109168 > > With my series applied: > 110112031 (base + 2863) > > With setting individual lockdep class: > 110114275 (base + 5107) > > I certainly like the lockdep_set_class() way since it is cleaner, but it adds > almost double amount of bytes to the kernel. Yeah, I've never really bothered with data too much, its a debug feature. So lock_class_key is 8 bytes, and strictly speaking you could union them over other fields, all we really need is unique addresses, we don't actually use the storage.
On 02/06/2015 08:32 PM, Peter Zijlstra wrote: > On Fri, Feb 06, 2015 at 06:05:32PM +0200, Peter Ujfalusi wrote: >> Certainly looks much simpler, but it adds quite a bit of data to the >> omap_hwmod struct, and we have a _lot_ of them for omap2plus configuration. >> >> ls -al vmlinux >> >> w/o any the lockdep warning fixes: >> 110109168 >> >> With my series applied: >> 110112031 (base + 2863) >> >> With setting individual lockdep class: >> 110114275 (base + 5107) >> >> I certainly like the lockdep_set_class() way since it is cleaner, but it adds >> almost double amount of bytes to the kernel. > > Yeah, I've never really bothered with data too much, its a debug > feature. So lock_class_key is 8 bytes, and strictly speaking you could > union them over other fields, all we really need is unique addresses, we > don't actually use the storage. True. our omap2plus defconfig does not have LOCKDEP enabled so it should not add anything to the data when running default kernel. I'll test the lockdep_set_class() method you suggested on Monday (not tomorrow), but still as first thing. If it is working as expected I'll send a patch with you as author. Thanks, Péter
On 02/06/2015 09:26 PM, Peter Ujfalusi wrote: >> Yeah, I've never really bothered with data too much, its a debug >> feature. So lock_class_key is 8 bytes, and strictly speaking you could >> union them over other fields, all we really need is unique addresses, we >> don't actually use the storage. > > True. our omap2plus defconfig does not have LOCKDEP enabled so it should not > add anything to the data when running default kernel. > I'll test the lockdep_set_class() method you suggested on Monday (not > tomorrow), but still as first thing. > If it is working as expected I'll send a patch with you as author. With omap2plus_defconfig my build produces (vmlinux size): Base: 99905522 with my series: 99908385 (base + 2863) with Peter Zijlstra's patch: 99910625 (base + 5103) The reason for this is that we will only have struct lock_class_key { }; in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by default, while the CONFIG_DEBUG_LOCKDEP is disabled. So it does add more data to our default omap2plus config. Tony: do you have preference on the way we fix this issue? As I recall there is a plan to remove the hwmod static database and move it or generate it from DT? Not sure when and how this will be done, but will it affect the lockdep_set_class() way?
On Mon, Feb 09, 2015 at 10:27:32AM +0200, Peter Ujfalusi wrote: > As I recall there is a plan to remove the hwmod static database and move it or > generate it from DT? Not sure when and how this will be done, but will it > affect the lockdep_set_class() way? Yes, struct lock_class_key wants to be in static storage. So you could still allocate a few of those statically and then use them where appropriate, but it'd going to be more work. The advantage of having the 1:1 relation is that any parent hierarchy works naturally.
On Mon, 9 Feb 2015, Peter Ujfalusi wrote: > On 02/06/2015 09:26 PM, Peter Ujfalusi wrote: > >> Yeah, I've never really bothered with data too much, its a debug > >> feature. So lock_class_key is 8 bytes, and strictly speaking you could > >> union them over other fields, all we really need is unique addresses, we > >> don't actually use the storage. > > > > True. our omap2plus defconfig does not have LOCKDEP enabled so it should not > > add anything to the data when running default kernel. > > I'll test the lockdep_set_class() method you suggested on Monday (not > > tomorrow), but still as first thing. > > If it is working as expected I'll send a patch with you as author. > > With omap2plus_defconfig my build produces (vmlinux size): > Base: 99905522 > with my series: 99908385 (base + 2863) > with Peter Zijlstra's patch: 99910625 (base + 5103) > > The reason for this is that we will only have > struct lock_class_key { }; > in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by > default, while the CONFIG_DEBUG_LOCKDEP is disabled. > > So it does add more data to our default omap2plus config. > > Tony: do you have preference on the way we fix this issue? > > As I recall there is a plan to remove the hwmod static database and move it or > generate it from DT? Not sure when and how this will be done, but will it > affect the lockdep_set_class() way? Well I guess we could see what Tony says, but you do realize that the difference in sizes that you posted above is about .003% of the total binary size, right? If there's one thing we can say about the last few years of ARM kernel development, it's that those kind of size increases are utterly dwarfed by other changes in the kernel. So I'd say, post a patch based on PeterZ's fix and be done with it... - Paul
* Paul Walmsley <paul@pwsan.com> [150209 08:04]: > On Mon, 9 Feb 2015, Peter Ujfalusi wrote: > > > On 02/06/2015 09:26 PM, Peter Ujfalusi wrote: > > >> Yeah, I've never really bothered with data too much, its a debug > > >> feature. So lock_class_key is 8 bytes, and strictly speaking you could > > >> union them over other fields, all we really need is unique addresses, we > > >> don't actually use the storage. > > > > > > True. our omap2plus defconfig does not have LOCKDEP enabled so it should not > > > add anything to the data when running default kernel. > > > I'll test the lockdep_set_class() method you suggested on Monday (not > > > tomorrow), but still as first thing. > > > If it is working as expected I'll send a patch with you as author. > > > > With omap2plus_defconfig my build produces (vmlinux size): > > Base: 99905522 > > with my series: 99908385 (base + 2863) > > with Peter Zijlstra's patch: 99910625 (base + 5103) > > > > The reason for this is that we will only have > > struct lock_class_key { }; > > in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by > > default, while the CONFIG_DEBUG_LOCKDEP is disabled. > > > > So it does add more data to our default omap2plus config. > > > > Tony: do you have preference on the way we fix this issue? > > > > As I recall there is a plan to remove the hwmod static database and move it or > > generate it from DT? Not sure when and how this will be done, but will it > > affect the lockdep_set_class() way? > > Well I guess we could see what Tony says, but you do realize that the > difference in sizes that you posted above is about .003% of the total > binary size, right? > > If there's one thing we can say about the last few years of ARM kernel > development, it's that those kind of size increases are utterly dwarfed by > other changes in the kernel. So I'd say, post a patch based on PeterZ's > fix and be done with it... Well the thing to consider here is what Peter U is saying about having struct omap_hwmod allocated based on the data from .dts files. If the fix makes the dynamic allocation harder to do later on, we should probably avoid it. If it's relatively easy to do later on, then I don't have a problem with it. Regards, Tony
On Mon, 9 Feb 2015, Tony Lindgren wrote: > * Paul Walmsley <paul@pwsan.com> [150209 08:04]: > > On Mon, 9 Feb 2015, Peter Ujfalusi wrote: > > > > > On 02/06/2015 09:26 PM, Peter Ujfalusi wrote: > > > >> Yeah, I've never really bothered with data too much, its a debug > > > >> feature. So lock_class_key is 8 bytes, and strictly speaking you could > > > >> union them over other fields, all we really need is unique addresses, we > > > >> don't actually use the storage. > > > > > > > > True. our omap2plus defconfig does not have LOCKDEP enabled so it should not > > > > add anything to the data when running default kernel. > > > > I'll test the lockdep_set_class() method you suggested on Monday (not > > > > tomorrow), but still as first thing. > > > > If it is working as expected I'll send a patch with you as author. > > > > > > With omap2plus_defconfig my build produces (vmlinux size): > > > Base: 99905522 > > > with my series: 99908385 (base + 2863) > > > with Peter Zijlstra's patch: 99910625 (base + 5103) > > > > > > The reason for this is that we will only have > > > struct lock_class_key { }; > > > in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by > > > default, while the CONFIG_DEBUG_LOCKDEP is disabled. > > > > > > So it does add more data to our default omap2plus config. > > > > > > Tony: do you have preference on the way we fix this issue? > > > > > > As I recall there is a plan to remove the hwmod static database and move it or > > > generate it from DT? Not sure when and how this will be done, but will it > > > affect the lockdep_set_class() way? > > > > Well I guess we could see what Tony says, but you do realize that the > > difference in sizes that you posted above is about .003% of the total > > binary size, right? > > > > If there's one thing we can say about the last few years of ARM kernel > > development, it's that those kind of size increases are utterly dwarfed by > > other changes in the kernel. So I'd say, post a patch based on PeterZ's > > fix and be done with it... > > Well the thing to consider here is what Peter U is saying about > having struct omap_hwmod allocated based on the data from .dts > files. If the fix makes the dynamic allocation harder to do later on, > we should probably avoid it. If it's relatively easy to do later on, > then I don't have a problem with it. The future destination for that code that makes the most sense to me is for it to become integrated with the OMAP Sonics & Arteris bus drivers and DT data. So I wouldn't worry too much about it; I don't think the lockdep fix will affect that at all. - Paul
* Paul Walmsley <paul@pwsan.com> [150209 10:59]: > On Mon, 9 Feb 2015, Tony Lindgren wrote: > > > * Paul Walmsley <paul@pwsan.com> [150209 08:04]: > > > On Mon, 9 Feb 2015, Peter Ujfalusi wrote: > > > > > > > On 02/06/2015 09:26 PM, Peter Ujfalusi wrote: > > > > >> Yeah, I've never really bothered with data too much, its a debug > > > > >> feature. So lock_class_key is 8 bytes, and strictly speaking you could > > > > >> union them over other fields, all we really need is unique addresses, we > > > > >> don't actually use the storage. > > > > > > > > > > True. our omap2plus defconfig does not have LOCKDEP enabled so it should not > > > > > add anything to the data when running default kernel. > > > > > I'll test the lockdep_set_class() method you suggested on Monday (not > > > > > tomorrow), but still as first thing. > > > > > If it is working as expected I'll send a patch with you as author. > > > > > > > > With omap2plus_defconfig my build produces (vmlinux size): > > > > Base: 99905522 > > > > with my series: 99908385 (base + 2863) > > > > with Peter Zijlstra's patch: 99910625 (base + 5103) > > > > > > > > The reason for this is that we will only have > > > > struct lock_class_key { }; > > > > in case of !CONFIG_LOCKDEP. On ARM however CONFIG_LOCKDEP is enabled by > > > > default, while the CONFIG_DEBUG_LOCKDEP is disabled. > > > > > > > > So it does add more data to our default omap2plus config. > > > > > > > > Tony: do you have preference on the way we fix this issue? > > > > > > > > As I recall there is a plan to remove the hwmod static database and move it or > > > > generate it from DT? Not sure when and how this will be done, but will it > > > > affect the lockdep_set_class() way? > > > > > > Well I guess we could see what Tony says, but you do realize that the > > > difference in sizes that you posted above is about .003% of the total > > > binary size, right? > > > > > > If there's one thing we can say about the last few years of ARM kernel > > > development, it's that those kind of size increases are utterly dwarfed by > > > other changes in the kernel. So I'd say, post a patch based on PeterZ's > > > fix and be done with it... > > > > Well the thing to consider here is what Peter U is saying about > > having struct omap_hwmod allocated based on the data from .dts > > files. If the fix makes the dynamic allocation harder to do later on, > > we should probably avoid it. If it's relatively easy to do later on, > > then I don't have a problem with it. > > The future destination for that code that makes the most sense to me is > for it to become integrated with the OMAP Sonics & Arteris bus drivers and > DT data. So I wouldn't worry too much about it; I don't think the > lockdep fix will affect that at all. OK up to you guys then. Regards, Tony
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c index 9025ffffd2dc..222d654ad6fd 100644 --- a/arch/arm/mach-omap2/omap_hwmod.c +++ b/arch/arm/mach-omap2/omap_hwmod.c @@ -2698,6 +2698,7 @@ static int __init _register(struct omap_hwmod *oh) INIT_LIST_HEAD(&oh->master_ports); INIT_LIST_HEAD(&oh->slave_ports); spin_lock_init(&oh->_lock); + lockdep_set_class(&oh->_lock, &oh->hwmod_key); oh->_state = _HWMOD_STATE_REGISTERED; diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h index 5b42fafcaf55..754bdfb3f811 100644 --- a/arch/arm/mach-omap2/omap_hwmod.h +++ b/arch/arm/mach-omap2/omap_hwmod.h @@ -689,6 +689,8 @@ struct omap_hwmod { u8 _state; u8 _postsetup_state; struct omap_hwmod *parent_hwmod; + + struct lock_class_key hwmod_key; }; struct omap_hwmod *omap_hwmod_lookup(const char *name);