Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1489376 > unrolled thread

[PATCH 0/2] Ajust lockdep static allocations

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] Ajust lockdep static allocations Babu Moger <babu.moger@oracle.com> - 2016-09-22 20:50 +0200
    [PATCH 1/2] lockdep: Keep the default static allocations small Babu Moger <babu.moger@oracle.com> - 2016-09-22 20:50 +0200
    Re: [PATCH 0/2] Ajust lockdep static allocations Peter Zijlstra <peterz@infradead.org> - 2016-09-23 09:20 +0200
      Re: [PATCH 0/2] Ajust lockdep static allocations Babu Moger <babu.moger@oracle.com> - 2016-09-23 16:10 +0200
        Re: [PATCH 0/2] Ajust lockdep static allocations Peter Zijlstra <peterz@infradead.org> - 2016-09-23 16:40 +0200
          Re: [PATCH 0/2] Ajust lockdep static allocations Babu Moger <babu.moger@oracle.com> - 2016-09-23 17:00 +0200
            Re: [PATCH 0/2] Ajust lockdep static allocations Peter Zijlstra <peterz@infradead.org> - 2016-09-23 17:10 +0200
              Re: [PATCH 0/2] Ajust lockdep static allocations Babu Moger <babu.moger@oracle.com> - 2016-09-23 17:20 +0200
                Re: [PATCH 0/2] Ajust lockdep static allocations Peter Zijlstra <peterz@infradead.org> - 2016-09-23 17:50 +0200
                  Re: [PATCH 0/2] Ajust lockdep static allocations Babu Moger <babu.moger@oracle.com> - 2016-09-23 22:00 +0200
                    Re: [PATCH 0/2] Ajust lockdep static allocations Rob Gardner <rob.gardner@oracle.com> - 2016-09-23 22:10 +0200
                    Re: [PATCH 0/2] Ajust lockdep static allocations Peter Zijlstra <peterz@infradead.org> - 2016-09-23 22:20 +0200
                      Re: [PATCH 0/2] Ajust lockdep static allocations Babu Moger <babu.moger@oracle.com> - 2016-09-23 22:40 +0200

#1489376 — [PATCH 0/2] Ajust lockdep static allocations

These patches adjust the static allocations for lockdep
data structures used for debugging locking correctness. The current
code reserves about 4MB extra space for these data structures. Most
of the configurations do not need these many data structures. While
testing, I have not seen it go beyond 20% of already reserved entries.

$grep "lock-classes" /proc/lockdep_stats
	lock-classes:                          1560 [max: 8191]

Reserving even more space seems unreasonable. So, keeping the default
entries small as before the Commit 1413c0389333 ("lockdep: Increase static
allocations"). Added new CONFIG_PROVE_LOCKING_PLUS in case someone
needs more entries to debug their large configuration.

Patch 1 : Adjusts the sizes based on the new config parameter
patch 2 : Adds new config parameter

Babu Moger (2):
  lockdep: Keep the default static allocations small
  config: Add new CONFIG_PROVE_LOCKING_PLUS

 kernel/locking/lockdep_internals.h |   14 +++++++++++---
 lib/Kconfig.debug                  |   10 ++++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)

[toc] | [next] | [standalone]

#1489380 — [PATCH 1/2] lockdep: Keep the default static allocations small

The Commit 1413c0389333 ("lockdep: Increase static allocations")
doubled the static allocation for lockdep. The size is unusually
high and not required for majority of the configurations. This
could cause problems to some environments with limited memory
configurations. We are already seeing issues on our sparc
configuration where kernel fails to boot when lockdep feature
is enabled. This patch keeps the default size to same as before
Commit 1413c0389333 ("lockdep: Increase static allocations").
Adding the new config parameter CONFIG_PROVE_LOCKING_PLUS in case
someone needs to enable more static space for lockdep entries,
lock chains and stack traces to debug large configurations.

Signed-off-by: Babu Moger <babu.moger@oracle.com>
---
 kernel/locking/lockdep_internals.h |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/kernel/locking/lockdep_internals.h b/kernel/locking/lockdep_internals.h
index 51c4b24..47336a6 100644
--- a/kernel/locking/lockdep_internals.h
+++ b/kernel/locking/lockdep_internals.h
@@ -54,18 +54,26 @@ enum {
  * table (if it's not there yet), and we check it for lock order
  * conflicts and deadlocks.
  */
+#ifdef CONFIG_PROVE_LOCKING_PLUS
 #define MAX_LOCKDEP_ENTRIES	32768UL
 
 #define MAX_LOCKDEP_CHAINS_BITS	16
-#define MAX_LOCKDEP_CHAINS	(1UL << MAX_LOCKDEP_CHAINS_BITS)
-
-#define MAX_LOCKDEP_CHAIN_HLOCKS (MAX_LOCKDEP_CHAINS*5)
 
 /*
  * Stack-trace: tightly packed array of stack backtrace
  * addresses. Protected by the hash_lock.
  */
 #define MAX_STACK_TRACE_ENTRIES	524288UL
+#else
+#define MAX_LOCKDEP_ENTRIES	16384UL
+#define MAX_LOCKDEP_CHAINS_BITS	15
+#define MAX_STACK_TRACE_ENTRIES	262144UL
+#endif
+
+#define MAX_LOCKDEP_CHAINS	(1UL << MAX_LOCKDEP_CHAINS_BITS)
+
+#define MAX_LOCKDEP_CHAIN_HLOCKS (MAX_LOCKDEP_CHAINS*5)
+
 
 extern struct list_head all_lock_classes;
 extern struct lock_chain lock_chains[];
-- 
1.7.1

[toc] | [prev] | [next] | [standalone]

#1489800

On Thu, Sep 22, 2016 at 11:43:34AM -0700, Babu Moger wrote:
> These patches adjust the static allocations for lockdep
> data structures used for debugging locking correctness. The current
> code reserves about 4MB extra space for these data structures. Most
> of the configurations do not need these many data structures. While
> testing, I have not seen it go beyond 20% of already reserved entries.
> 
> $grep "lock-classes" /proc/lockdep_stats
> 	lock-classes:                          1560 [max: 8191]
> 
> Reserving even more space seems unreasonable. So, keeping the default
> entries small as before the Commit 1413c0389333 ("lockdep: Increase static
> allocations"). Added new CONFIG_PROVE_LOCKING_PLUS in case someone
> needs more entries to debug their large configuration.

Why make this more complicated? There's absolutely no upside to this
change as far as I can see.

[toc] | [prev] | [next] | [standalone]

#1490142

On 9/23/2016 2:12 AM, Peter Zijlstra wrote:
> On Thu, Sep 22, 2016 at 11:43:34AM -0700, Babu Moger wrote:
>> These patches adjust the static allocations for lockdep
>> data structures used for debugging locking correctness. The current
>> code reserves about 4MB extra space for these data structures. Most
>> of the configurations do not need these many data structures. While
>> testing, I have not seen it go beyond 20% of already reserved entries.
>>
>> $grep "lock-classes" /proc/lockdep_stats
>> 	lock-classes:                          1560 [max: 8191]
>>
>> Reserving even more space seems unreasonable. So, keeping the default
>> entries small as before the Commit 1413c0389333 ("lockdep: Increase static
>> allocations"). Added new CONFIG_PROVE_LOCKING_PLUS in case someone
>> needs more entries to debug their large configuration.
> Why make this more complicated? There's absolutely no upside to this
> change as far as I can see.
Peter, What do you mean?  Revert the commit 1413c038933?  Right now,
I cannot boot my setup after enabling lockdep. How do you think we can 
handle this?

[toc] | [prev] | [next] | [standalone]

#1490176

On Fri, Sep 23, 2016 at 09:04:42AM -0500, Babu Moger wrote:
> On 9/23/2016 2:12 AM, Peter Zijlstra wrote:
> >On Thu, Sep 22, 2016 at 11:43:34AM -0700, Babu Moger wrote:
> >>These patches adjust the static allocations for lockdep
> >>data structures used for debugging locking correctness. The current
> >>code reserves about 4MB extra space for these data structures. Most
> >>of the configurations do not need these many data structures. While
> >>testing, I have not seen it go beyond 20% of already reserved entries.
> >>
> >>$grep "lock-classes" /proc/lockdep_stats
> >>	lock-classes:                          1560 [max: 8191]
> >>
> >>Reserving even more space seems unreasonable. So, keeping the default
> >>entries small as before the Commit 1413c0389333 ("lockdep: Increase static
> >>allocations"). Added new CONFIG_PROVE_LOCKING_PLUS in case someone
> >>needs more entries to debug their large configuration.
> >Why make this more complicated? There's absolutely no upside to this
> >change as far as I can see.
> Peter, What do you mean?  

I mean I see no point to the patches you send.

> Revert the commit 1413c038933? 

Nah, why would I?

> Right now, I cannot boot my setup after enabling lockdep. How do you
> think we can handle this?

Why can't you boot? You have that little memories? 4MB doesn't seem like
a worthwhile amount of memory.

Also, you didn't say.  This seems a somewhat crucial point.

In any case, maybe invert this, add make it depend on CONFIG_BASE_SMALL,
since this really only matters for really dinky systems.

[toc] | [prev] | [next] | [standalone]

#1490206

On 9/23/2016 9:34 AM, Peter Zijlstra wrote:
> On Fri, Sep 23, 2016 at 09:04:42AM -0500, Babu Moger wrote:
>> On 9/23/2016 2:12 AM, Peter Zijlstra wrote:
>>> On Thu, Sep 22, 2016 at 11:43:34AM -0700, Babu Moger wrote:
>>>> These patches adjust the static allocations for lockdep
>>>> data structures used for debugging locking correctness. The current
>>>> code reserves about 4MB extra space for these data structures. Most
>>>> of the configurations do not need these many data structures. While
>>>> testing, I have not seen it go beyond 20% of already reserved entries.
>>>>
>>>> $grep "lock-classes" /proc/lockdep_stats
>>>> 	lock-classes:                          1560 [max: 8191]
>>>>
>>>> Reserving even more space seems unreasonable. So, keeping the default
>>>> entries small as before the Commit 1413c0389333 ("lockdep: Increase static
>>>> allocations"). Added new CONFIG_PROVE_LOCKING_PLUS in case someone
>>>> needs more entries to debug their large configuration.
>>> Why make this more complicated? There's absolutely no upside to this
>>> change as far as I can see.
>> Peter, What do you mean?
> I mean I see no point to the patches you send.
>
>> Revert the commit 1413c038933?
> Nah, why would I?
>
>> Right now, I cannot boot my setup after enabling lockdep. How do you
>> think we can handle this?
> Why can't you boot? You have that little memories? 4MB doesn't seem like
> a worthwhile amount of memory.
>
> Also, you didn't say.  This seems a somewhat crucial point.

     Correct, We can't boot with lockdep. Sorry I did not make that 
clear.  We have a limit on static size of the kernel.

>
> In any case, maybe invert this, add make it depend on CONFIG_BASE_SMALL,
> since this really only matters for really dinky systems.
      Sure. Will  use  CONFIG_BASE_SMALL and re-post the patches. Thanks
>

[toc] | [prev] | [next] | [standalone]

#1490226

On Fri, Sep 23, 2016 at 09:50:52AM -0500, Babu Moger wrote:
> >Why can't you boot? You have that little memories? 4MB doesn't seem like
> >a worthwhile amount of memory.
> >
> >Also, you didn't say.  This seems a somewhat crucial point.
> 
>     Correct, We can't boot with lockdep. Sorry I did not make that clear.
> We have a limit on static size of the kernel.

This stuff should be in .bss not .data. It should not affect the static
size at all. Or am I misunderstanding things?

[toc] | [prev] | [next] | [standalone]

#1490235

On 9/23/2016 10:04 AM, Peter Zijlstra wrote:
> On Fri, Sep 23, 2016 at 09:50:52AM -0500, Babu Moger wrote:
>>> Why can't you boot? You have that little memories? 4MB doesn't seem like
>>> a worthwhile amount of memory.
>>>
>>> Also, you didn't say.  This seems a somewhat crucial point.
>>      Correct, We can't boot with lockdep. Sorry I did not make that clear.
>> We have a limit on static size of the kernel.
> This stuff should be in .bss not .data. It should not affect the static
> size at all. Or am I misunderstanding things?
  Here it is.
$ ./scripts/bloat-o-meter vmlinux.lockdep.small vmlinux.lockdep.big
add/remove: 0/0 grow/shrink: 5/0 up/down: 4653056/0 (4653056)
function                                     old     new   delta
stack_trace                              2097152 4194304 +2097152
lock_chains                              1048576 2097152 +1048576
list_entries                             1048576 2097152 +1048576
chain_hlocks                              327680  655360 +327680
chainhash_table                           131072  262144 +131072
Total: Before=21046200, After=25699256, chg 22.000000%

[toc] | [prev] | [next] | [standalone]

#1490285

On Fri, Sep 23, 2016 at 10:15:46AM -0500, Babu Moger wrote:

> >>     Correct, We can't boot with lockdep. Sorry I did not make that clear.
> >>We have a limit on static size of the kernel.

> >This stuff should be in .bss not .data. It should not affect the static
> >size at all. Or am I misunderstanding things?

>  Here it is.
> $ ./scripts/bloat-o-meter vmlinux.lockdep.small vmlinux.lockdep.big

What does bloat-o-meter have to do with things? The static image size is
not dependent on .bss, right?

[toc] | [prev] | [next] | [standalone]

#1490421

On 9/23/2016 10:40 AM, Peter Zijlstra wrote:
> On Fri, Sep 23, 2016 at 10:15:46AM -0500, Babu Moger wrote:
>
>>>>      Correct, We can't boot with lockdep. Sorry I did not make that clear.
>>>> We have a limit on static size of the kernel.
>>> This stuff should be in .bss not .data. It should not affect the static
>>> size at all. Or am I misunderstanding things?
>>   Here it is.
>> $ ./scripts/bloat-o-meter vmlinux.lockdep.small vmlinux.lockdep.big
> What does bloat-o-meter have to do with things? The static image size is
> not dependent on .bss, right?

  Peter,
  We checked again. Yes, It goes in .bss section. But in sparc we have 
to fit .text, .data,
  .bss in 7 permanent TLBs(that is totally 28MB). It was fine so far.  
But the commit
  1413c0389333 ("lockdep: Increase static allocations") added extra 4MB 
which makes
  it go beyond 28MB. That is causing system boot up problems in sparc. 
Yes. We know it.
  This is a limitation. Changing this limit in our hardware is a much 
bigger change which
  we cannot address right away. So, we are trying to come up with a 
solution which can
  work for all. I will re-post the patches with  CONFIG_BASE_SMALL 
option if there is no
  objections.

  CCing David Miller and Rob Gardner. They might be able to explain more 
if you
  have any more questions. Here is the discussion thread if you guys 
want to look at history.
  https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1237642.html

[toc] | [prev] | [next] | [standalone]

#1490426

On 09/23/2016 01:57 PM, Babu Moger wrote:
>
> On 9/23/2016 10:40 AM, Peter Zijlstra wrote:
>> On Fri, Sep 23, 2016 at 10:15:46AM -0500, Babu Moger wrote:
>>
>>>>>      Correct, We can't boot with lockdep. Sorry I did not make 
>>>>> that clear.
>>>>> We have a limit on static size of the kernel.
>>>> This stuff should be in .bss not .data. It should not affect the 
>>>> static
>>>> size at all. Or am I misunderstanding things?
>>>   Here it is.
>>> $ ./scripts/bloat-o-meter vmlinux.lockdep.small vmlinux.lockdep.big
>> What does bloat-o-meter have to do with things? The static image size is
>> not dependent on .bss, right?
>
>  Peter,
>  We checked again. Yes, It goes in .bss section. But in sparc we have 
> to fit .text, .data,
>  .bss in 7 permanent TLBs(that is totally 28MB). It was fine so far.  
> But the commit
>  1413c0389333 ("lockdep: Increase static allocations") added extra 4MB 
> which makes
>  it go beyond 28MB. That is causing system boot up problems in sparc. 
> Yes. We know it.
>  This is a limitation. Changing this limit in our hardware is a much 
> bigger change which
>  we cannot address right away. So, we are trying to come up with a 
> solution which can
>  work for all. I will re-post the patches with  CONFIG_BASE_SMALL 
> option if there is no
>  objections.
>
>  CCing David Miller and Rob Gardner. They might be able to explain 
> more if you
>  have any more questions. Here is the discussion thread if you guys 
> want to look at history.
>  https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1237642.html 
>
>
>

Yes, perhaps I can help clarify the problem Babu is seeing. It is true 
that stuff in bss doesn't increase the static size of the file that 
contains the kernel. But it does increase the kernel's memory footprint. 
And as the system is booting, all the kernel's code, data, and bss, must 
have locked translations in the TLB so that we don't get TLB misses on 
kernel code and data. Current sparc chips have 8 TLB entries available 
that may be locked down, and with a 4mb page size, this gives a maximum 
of 32mb of memory that can be covered. One of these is used for kexec (I 
think), so that leaves 28mb. It sounds to me like Babu is saying that 
the change in question has increased the size of bss data so this limit 
is exceeded, thus causing boot problems, and he proposes to somewhat 
reduce the added space to alleviate this problem.

And also as Babu says, changing to a larger page size is very tricky. 
Not only do different sparc cpus support a different set of h/w page 
sizes, the effects of changing this are quite far reaching and would 
affect a lot of code.

Rob Gardner

[toc] | [prev] | [next] | [standalone]

#1490431

On Fri, Sep 23, 2016 at 02:57:39PM -0500, Babu Moger wrote:

>  We checked again. Yes, It goes in .bss section. But in sparc we have
>  to fit .text, .data, .bss in 7 permanent TLBs(that is totally 28MB).
>  It was fine so far.  But the commit 1413c0389333 ("lockdep: Increase
>  static allocations") added extra 4MB which makes it go beyond 28MB.
>  That is causing system boot up problems in sparc. 

*sigh*, why didn't you start with that :/

> Yes.  We know it.  This is a limitation. Changing this limit in our
> hardware is a much bigger change which we cannot address right away.
> So, we are trying to come up with a solution which can work for all. I
> will re-post the patches with  CONFIG_BASE_SMALL option if there is no
> objections.

OK, so double check BASE_SMALL doesn't imply other things you cannot
live with, Sparc64 isn't a dinky system. If BASE_SMALL works for you
then good, otherwise do a PROVE_LOCKING_SMALL symbol that is not user
selectable and have SPARC select that. Use the invisible Help for that
symbol to explain all this again.

>  CCing David Miller and Rob Gardner. They might be able to explain
>  more if you have any more questions.

Nah, I think I remember enough of how the Sparc MMU works to see reason.

[toc] | [prev] | [next] | [standalone]

#1490444

On 9/23/2016 3:17 PM, Peter Zijlstra wrote:
> On Fri, Sep 23, 2016 at 02:57:39PM -0500, Babu Moger wrote:
>
>>   We checked again. Yes, It goes in .bss section. But in sparc we have
>>   to fit .text, .data, .bss in 7 permanent TLBs(that is totally 28MB).
>>   It was fine so far.  But the commit 1413c0389333 ("lockdep: Increase
>>   static allocations") added extra 4MB which makes it go beyond 28MB.
>>   That is causing system boot up problems in sparc.
> *sigh*, why didn't you start with that :/
>
>> Yes.  We know it.  This is a limitation. Changing this limit in our
>> hardware is a much bigger change which we cannot address right away.
>> So, we are trying to come up with a solution which can work for all. I
>> will re-post the patches with  CONFIG_BASE_SMALL option if there is no
>> objections.
> OK, so double check BASE_SMALL doesn't imply other things you cannot
> live with, Sparc64 isn't a dinky system. If BASE_SMALL works for you
> then good, otherwise do a PROVE_LOCKING_SMALL symbol that is not user
> selectable and have SPARC select that. Use the invisible Help for that
> symbol to explain all this again.

      Thanks. Will work on it.

>
>>   CCing David Miller and Rob Gardner. They might be able to explain
>>   more if you have any more questions.
> Nah, I think I remember enough of how the Sparc MMU works to see reason.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web