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.
Regards,
Peter
---
Peter Ujfalusi (2):
ARM: omap2+: omap_hwmod: Use _nested version of spinlock for oh->_lock
ARM: DRA7: hwmod_data: Change locked_class for atl hwmod
arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++--------
arch/arm/mach-omap2/omap_hwmod.h | 5 +++++
arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 1 +
3 files changed, 14 insertions(+), 8 deletions(-)
--
2.2.2
Add lockdep_class member to struct omap_hwmod and use this number to
specify the subclass of the given hwmod's lock.
When defining the hwmods the lockdep_level can be initialized to indicate
that this lock might be used in a nested fashion.
DRA7x's ATL hwmod is one example for this since McASP can select ATL clock
as functional clock, which will trigger nested oh->_lock usage. This will
trigger false warning from lockdep validator since it is dealing with
classes and for it all hwmod clocks are the same class.
Signed-off-by: Peter Ujfalusi <[email protected]>
---
arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++--------
arch/arm/mach-omap2/omap_hwmod.h | 5 +++++
2 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 92afb723dcfc..b5afde560054 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -3334,7 +3334,7 @@ int omap_hwmod_enable(struct omap_hwmod *oh)
if (!oh)
return -EINVAL;
- spin_lock_irqsave(&oh->_lock, flags);
+ spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
r = _enable(oh);
spin_unlock_irqrestore(&oh->_lock, flags);
@@ -3355,7 +3355,7 @@ int omap_hwmod_idle(struct omap_hwmod *oh)
if (!oh)
return -EINVAL;
- spin_lock_irqsave(&oh->_lock, flags);
+ spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
_idle(oh);
spin_unlock_irqrestore(&oh->_lock, flags);
@@ -3377,7 +3377,7 @@ int omap_hwmod_shutdown(struct omap_hwmod *oh)
if (!oh)
return -EINVAL;
- spin_lock_irqsave(&oh->_lock, flags);
+ spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
_shutdown(oh);
spin_unlock_irqrestore(&oh->_lock, flags);
@@ -3667,7 +3667,7 @@ int omap_hwmod_enable_wakeup(struct omap_hwmod *oh)
unsigned long flags;
u32 v;
- spin_lock_irqsave(&oh->_lock, flags);
+ spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
if (oh->class->sysc &&
(oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP)) {
@@ -3700,7 +3700,7 @@ int omap_hwmod_disable_wakeup(struct omap_hwmod *oh)
unsigned long flags;
u32 v;
- spin_lock_irqsave(&oh->_lock, flags);
+ spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
if (oh->class->sysc &&
(oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP)) {
@@ -3735,7 +3735,7 @@ int omap_hwmod_assert_hardreset(struct omap_hwmod *oh, const char *name)
if (!oh)
return -EINVAL;
- spin_lock_irqsave(&oh->_lock, flags);
+ spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
ret = _assert_hardreset(oh, name);
spin_unlock_irqrestore(&oh->_lock, flags);
@@ -3762,7 +3762,7 @@ int omap_hwmod_deassert_hardreset(struct omap_hwmod *oh, const char *name)
if (!oh)
return -EINVAL;
- spin_lock_irqsave(&oh->_lock, flags);
+ spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
ret = _deassert_hardreset(oh, name);
spin_unlock_irqrestore(&oh->_lock, flags);
@@ -3836,7 +3836,7 @@ int omap_hwmod_set_postsetup_state(struct omap_hwmod *oh, u8 state)
state != _HWMOD_STATE_IDLE)
return -EINVAL;
- spin_lock_irqsave(&oh->_lock, flags);
+ spin_lock_irqsave_nested(&oh->_lock, flags, oh->lockdep_class);
if (oh->_state != _HWMOD_STATE_REGISTERED) {
ret = -EINVAL;
diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h
index 9d4bec6ee742..260d3e9c9f6d 100644
--- a/arch/arm/mach-omap2/omap_hwmod.h
+++ b/arch/arm/mach-omap2/omap_hwmod.h
@@ -606,6 +606,9 @@ struct omap_hwmod_link {
struct list_head node;
};
+#define HWMOD_LOCKDEP_SUBCLASS_NORMAL 0
+#define HWMOD_LOCKDEP_SUBCLASS_CLASS1 1 /* might be used as nested */
+
/**
* struct omap_hwmod - integration data for OMAP hardware "modules" (IP blocks)
* @name: name of the hwmod
@@ -632,6 +635,7 @@ struct omap_hwmod_link {
* @_postsetup_state: internal-use state to leave the hwmod in after _setup()
* @flags: hwmod flags (documented below)
* @_lock: spinlock serializing operations on this hwmod
+ * @lockdep_class: subclass to use with spin_lock_irqsave_nested()
* @node: list node for hwmod list (internal use)
* @parent_hwmod: (temporary) a pointer to the hierarchical parent of this hwmod
*
@@ -674,6 +678,7 @@ struct omap_hwmod {
u32 _sysc_cache;
void __iomem *_mpu_rt_va;
spinlock_t _lock;
+ unsigned int lockdep_class;
struct list_head node;
struct omap_hwmod_ocp_if *_mpu_port;
unsigned int (*xlate_irq)(unsigned int);
--
2.2.2
The ATL hwmod can be used in nested way when it is selected to be the
functional clock for McASP. For this lockdep validator will trigger false
positive warning.
By assigning separate class to atl locking will sort this out.
Signed-off-by: Peter Ujfalusi <[email protected]>
---
arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
index e8692e7675b8..87f259e8c574 100644
--- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
@@ -185,6 +185,7 @@ static struct omap_hwmod dra7xx_atl_hwmod = {
.class = &dra7xx_atl_hwmod_class,
.clkdm_name = "atl_clkdm",
.main_clk = "atl_gfclk_mux",
+ .lockdep_class = HWMOD_LOCKDEP_SUBCLASS_CLASS1,
.prcm = {
.omap4 = {
.clkctrl_offs = DRA7XX_CM_ATL_ATL_CLKCTRL_OFFSET,
--
2.2.2
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);
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.
--
P?ter
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?
--
P?ter
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 <[email protected]> [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 <[email protected]> [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 <[email protected]> [150209 10:59]:
> On Mon, 9 Feb 2015, Tony Lindgren wrote:
>
> > * Paul Walmsley <[email protected]> [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