diff mbox

[0/2] ARM: omap2+: omap_hwmod: Fix false lockdep warning

Message ID 20150206141346.GP21418@twins.programming.kicks-ass.net (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Zijlstra Feb. 6, 2015, 2:13 p.m. UTC
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(+)

Comments

Peter Ujfalusi Feb. 6, 2015, 4:05 p.m. UTC | #1
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.
Peter Zijlstra Feb. 6, 2015, 6:32 p.m. UTC | #2
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.
Peter Ujfalusi Feb. 6, 2015, 7:26 p.m. UTC | #3
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
Peter Ujfalusi Feb. 9, 2015, 8:27 a.m. UTC | #4
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?
Peter Zijlstra Feb. 9, 2015, 9:23 a.m. UTC | #5
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.
Paul Walmsley Feb. 9, 2015, 4 p.m. UTC | #6
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
Tony Lindgren Feb. 9, 2015, 5:31 p.m. UTC | #7
* 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
Paul Walmsley Feb. 9, 2015, 6:55 p.m. UTC | #8
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
Tony Lindgren Feb. 9, 2015, 10:16 p.m. UTC | #9
* 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 mbox

Patch

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);