LOCKDEP customizable numbers upper limit

LOCKDEP customizable numbers upper limit

[J. R. Okajima]

Hello,

According to the commit in v5.13-rc1,
	5dc33592e9553 2021-04-05 lockdep: Allow tuning tracing capacity constants.
several lockdep numbers have their own range as 10--30.
But if we set all 30s, we got a compilation error.

kernel/locking/lockdep.c:3536:2: note: in expansion of macro 'BUILD_BUG_ON'
  BUILD_BUG_ON((1UL << 24) <= ARRAY_SIZE(chain_hlocks));

kernel/locking/lockdep.c
----------------------------------------
static u16 chain_hlocks[MAX_LOCKDEP_CHAIN_HLOCKS];
	:::
	BUILD_BUG_ON((1UL << 24) <= ARRAY_SIZE(chain_hlocks));
	:::
----------------------------------------

MAX_LOCKDEP_CHAIN_HLOCKS is defined in kernel/locking/lockdep_internal.h
as this.

#define MAX_LOCKDEP_CHAINS_BITS		CONFIG_LOCKDEP_CHAINS_BITS
#define MAX_LOCKDEP_CHAINS		(1UL << MAX_LOCKDEP_CHAINS_BITS)
#define MAX_LOCKDEP_CHAIN_HLOCKS	(MAX_LOCKDEP_CHAINS*5)

I don't know what this 'multiply by 5' means and why
ARRAY_SIZE(chain_hlocks) is limited to (1UL << 24), but setting 30 to
CONFIG_LOCKDEP_CHAINS_BITS obviously causes BUILD_BUG.
'*5' is more than 2 bits shift, so CONFIG_LOCKDEP_CHAINS_BITS has to be
less than (24-2), limited to the range 10--21.

Hmm, I tried.

CONFIG_LOCKDEP_BITS=30
CONFIG_LOCKDEP_CHAINS_BITS=21
CONFIG_LOCKDEP_STACK_TRACE_BITS=30
CONFIG_LOCKDEP_STACK_TRACE_HASH_BITS=30
CONFIG_LOCKDEP_CIRCULAR_QUEUE_BITS=30

Arg, LD failed.
ld: kernel/locking/lockdep.o: in function `lockdep_hlock_class':
lockdep.c:(.text+0x84f): relocation truncated to fit: R_X86_64_PC32 against `.bss'

I am afraid these LOCKDEP configurations need some sort of balancing.


J. R. Okajima

Re: LOCKDEP customizable numbers upper limit

[Tetsuo Handa]

On 2021/05/13 22:39, J. R. Okajima wrote:
> Hello,
> 
> According to the commit in v5.13-rc1,
> 	5dc33592e9553 2021-04-05 lockdep: Allow tuning tracing capacity constants.
> several lockdep numbers have their own range as 10--30.
> But if we set all 30s, we got a compilation error.

Thanks for your report.

Initial proposal at
https://lore.kernel.org/lkml/1595640639-9310-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp/
allowed only "double the upper limits", and subsequent proposals allowed arbitrary sizes in
response to https://lore.kernel.org/lkml/CACT4Y+YXT9iLij-AbrUwj=yPq-YNFw=Au9g0LQJCKwYonaHCDQ@mail.gmail.com/ ,
but practically increasing by a few bits should be sufficient.

I'm never expecting to use 30. I'm sure that setting 30s will become a too
much memory consumer that will prevent the kernel from booting correctly.
Thus, I don't think we need to care about randconfig kernels.

Please submit a patch that avoids only BUILD_BUG_ON().

Regards.

PATCH: Re: LOCKDEP customizable numbers upper limit

[hooanon05g]

Tetsuo Handa:
> Please submit a patch that avoids only BUILD_BUG_ON().

Here it is.

J. R. Okajima

----------------------------------------
commit 43e103e1a5975c61334811d16e207e6d0ac57b77
Author: J. R. Okajima <hooanon05g@gmail.com>
Date:   Sat May 15 03:17:10 2021 +0900

    LOCKDEP: upper limit LOCKDEP_CHAINS_BITS
    
    CONFIG_LOCKDEP_CHAINS_BITS value decides the size of chain_hlocks[] in
    kernel/locking/lockdep.c, and it is checked by add_chain_cache() with
            BUILD_BUG_ON((1UL << 24) <= ARRAY_SIZE(chain_hlocks));
    This patch is just to silence BUILD_BUG_ON().
    
    See-also: https://marc.info/?l=linux-kernel&m=162091320503900&w=2
    Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Signed-off-by: J. R. Okajima <hooanon05g@gmail.com>

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 678c13967580e..999ed5aa6bcee 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1390,7 +1390,7 @@ config LOCKDEP_BITS
 config LOCKDEP_CHAINS_BITS
 	int "Bitsize for MAX_LOCKDEP_CHAINS"
 	depends on LOCKDEP && !LOCKDEP_SMALL
-	range 10 30
+	range 10 21
 	default 16
 	help
 	  Try increasing this value if you hit "BUG: MAX_LOCKDEP_CHAINS too low!" message.

Re: PATCH: Re: LOCKDEP customizable numbers upper limit

[Tetsuo Handa]

On 2021/05/15 3:22, hooanon05g@gmail.com wrote:
> Tetsuo Handa:
>> Please submit a patch that avoids only BUILD_BUG_ON().
> 
> Here it is.

Thank you. In practice, nobody will increase by more than 5 bits.

Acked-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

> 
> J. R. Okajima
> 
> ----------------------------------------
> commit 43e103e1a5975c61334811d16e207e6d0ac57b77
> Author: J. R. Okajima <hooanon05g@gmail.com>
> Date:   Sat May 15 03:17:10 2021 +0900
> 
>     LOCKDEP: upper limit LOCKDEP_CHAINS_BITS
>     
>     CONFIG_LOCKDEP_CHAINS_BITS value decides the size of chain_hlocks[] in
>     kernel/locking/lockdep.c, and it is checked by add_chain_cache() with
>             BUILD_BUG_ON((1UL << 24) <= ARRAY_SIZE(chain_hlocks));
>     This patch is just to silence BUILD_BUG_ON().
>     
>     See-also: https://marc.info/?l=linux-kernel&m=162091320503900&w=2
>     Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>     Cc: Peter Zijlstra <peterz@infradead.org>
>     Signed-off-by: J. R. Okajima <hooanon05g@gmail.com>
> 
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 678c13967580e..999ed5aa6bcee 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1390,7 +1390,7 @@ config LOCKDEP_BITS
>  config LOCKDEP_CHAINS_BITS
>  	int "Bitsize for MAX_LOCKDEP_CHAINS"
>  	depends on LOCKDEP && !LOCKDEP_SMALL
> -	range 10 30
> +	range 10 21
>  	default 16
>  	help
>  	  Try increasing this value if you hit "BUG: MAX_LOCKDEP_CHAINS too low!" message.
> 

[PATCH v2] lockdep: upper limit LOCKDEP_CHAINS_BITS

[Carlos Llamas]

From: "J. R. Okajima" <hooanon05g@gmail.com>

CONFIG_LOCKDEP_CHAINS_BITS value decides the size of chain_hlocks[] in
kernel/locking/lockdep.c, and it is checked by add_chain_cache() with
    BUILD_BUG_ON((1UL << 24) <= ARRAY_SIZE(chain_hlocks));
This patch is just to silence BUILD_BUG_ON().

See-also: https://lore.kernel.org/all/30795.1620913191@jrobl/
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: J. R. Okajima <hooanon05g@gmail.com>
Acked-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
[cmllamas: fix minor checkpatch issues in commit log]
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 lib/Kconfig.debug | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index a30c03a66172..04668248225c 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1515,7 +1515,7 @@ config LOCKDEP_BITS
 config LOCKDEP_CHAINS_BITS
 	int "Bitsize for MAX_LOCKDEP_CHAINS"
 	depends on LOCKDEP && !LOCKDEP_SMALL
-	range 10 30
+	range 10 21
 	default 16
 	help
 	  Try increasing this value if you hit "BUG: MAX_LOCKDEP_CHAINS too low!" message.
-- 
2.45.2.1089.g2a221341d9-goog

Re: [PATCH v2] lockdep: upper limit LOCKDEP_CHAINS_BITS

[Andrew Morton]

On Tue, 23 Jul 2024 16:40:17 +0000 Carlos Llamas <cmllamas@google.com> wrote:

> From: "J. R. Okajima" <hooanon05g@gmail.com>
> 
> CONFIG_LOCKDEP_CHAINS_BITS value decides the size of chain_hlocks[] in
> kernel/locking/lockdep.c, and it is checked by add_chain_cache() with
>     BUILD_BUG_ON((1UL << 24) <= ARRAY_SIZE(chain_hlocks));
> This patch is just to silence BUILD_BUG_ON().
> 
> ...
>
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1515,7 +1515,7 @@ config LOCKDEP_BITS
>  config LOCKDEP_CHAINS_BITS
>  	int "Bitsize for MAX_LOCKDEP_CHAINS"
>  	depends on LOCKDEP && !LOCKDEP_SMALL
> -	range 10 30
> +	range 10 21
>  	default 16
>  	help
>  	  Try increasing this value if you hit "BUG: MAX_LOCKDEP_CHAINS too low!" message.

checking your homework...

With LOCKDEP_CHAINS_BITS == 21:

	#define MAX_LOCKDEP_CHAINS_BITS CONFIG_LOCKDEP_CHAINS_BITS

		gives MAX_LOCKDEP_CHAINS_BITS == 21

	#define MAX_LOCKDEP_CHAINS      (1UL << MAX_LOCKDEP_CHAINS_BITS)

		gives MAX_LOCKDEP_CHAINS == (1UL << 21)

	#define MAX_LOCKDEP_CHAIN_HLOCKS (MAX_LOCKDEP_CHAINS*5)

		gives MAX_LOCKDEP_CHAIN_HLOCKS = 5 * (1UL << 21)

	static u16 chain_hlocks[MAX_LOCKDEP_CHAIN_HLOCKS];

		gives ARRAY_SIZE(chain_hlocks) == 5 * (1UL << 21)

so

	BUILD_BUG_ON((1UL << 24) <= ARRAY_SIZE(chain_hlocks));
	ie, BUILD_BUG_ON((1UL << 24) <= 5 * (1UL << 21));

is OK, whereas

	BUILD_BUG_ON((1UL << 24) <= 5 * (1UL << 22));

will bug out.  So LGTM, I'll add it to mm.git.


btw, the help text "Bitsize for MAX_LOCKDEP_CHAINS" is odd.  What's a
bitsize?  Maybe "bit shift count for..." or such.

Re: [PATCH v2] lockdep: upper limit LOCKDEP_CHAINS_BITS

[Carlos Llamas]

On Wed, Jul 31, 2024 at 04:48:23PM -0700, Andrew Morton wrote:
[...]
> so
> 
> 	BUILD_BUG_ON((1UL << 24) <= ARRAY_SIZE(chain_hlocks));
> 	ie, BUILD_BUG_ON((1UL << 24) <= 5 * (1UL << 21));
> 
> is OK, whereas
> 
> 	BUILD_BUG_ON((1UL << 24) <= 5 * (1UL << 22));
> 
> will bug out.  So LGTM, I'll add it to mm.git.
>

Right. I ran into the BUILD_BUG_ON() while trying to max out
LOCKDEP_CHAINS_BITS. I initially suspected the assert was incorrect as
the static array is being indexed as chain_hlocks[base + depth], which
according to the bitfileds in 'struct lock_chain' should likely be a 30
bit shift instead:

	unsigned int    irq_contex :  2,
			depth      :  6,
			base       : 24;

In practice though, using 1UL << 30 will blow up the bss section. This
is also true for the _any_ of the CONFIG_LOCKDEP_*_BITS. As they are all
shifts to determine the size of static arrays.

I simply dug up this patch from J.R. which avoids the BUILD_BUG_ON(),
but perhaps someone should limit the rest of the configs? In practice,
nobody should be using these 30 bit shifts.


> btw, the help text "Bitsize for MAX_LOCKDEP_CHAINS" is odd.  What's a
> bitsize?  Maybe "bit shift count for..." or such.

Indeed that is odd. I'm also not sure what to make of the "*5" magic
number. I suppose it could be the typical lock depth? I could try to
clarify these points, if no one with more insight wants to do it.

--
Carlos Llamas

Re: [PATCH v2] lockdep: upper limit LOCKDEP_CHAINS_BITS

[J. R. Okajima]

Carlos Llamas:
> I simply dug up this patch from J.R. which avoids the BUILD_BUG_ON(),
> but perhaps someone should limit the rest of the configs? In practice,
> nobody should be using these 30 bit shifts.

I posted the patch in 2021. It was against the commit in v5.13-rc1,
        5dc33592e9553 2021-04-05 lockdep: Allow tuning tracing capacity constants.
It is a little surprise for me that you could pick up such old post.

As I wrote in the old post
	https://lore.kernel.org/all/30795.1620913191@jrobl/
	I don't know what this 'multiply by 5' means and why
	ARRAY_SIZE(chain_hlocks) is limited to (1UL << 24)
And the post is just to silence BUILD_BUG_ON().
There are a few more "range 10 30" in lib/Kconfig.debug.


> > btw, the help text "Bitsize for MAX_LOCKDEP_CHAINS" is odd.  What's a
> > bitsize?  Maybe "bit shift count for..." or such.
>
> Indeed that is odd. I'm also not sure what to make of the "*5" magic
> number. I suppose it could be the typical lock depth? I could try to
> clarify these points, if no one with more insight wants to do it.

Also the original text comes from the commit in v5.13-rc1.


J. R. Okajima

Re: [PATCH v2] lockdep: upper limit LOCKDEP_CHAINS_BITS

[Carlos Llamas]

On Wed, Jul 31, 2024 at 04:48:23PM -0700, Andrew Morton wrote:
> btw, the help text "Bitsize for MAX_LOCKDEP_CHAINS" is odd.  What's a
> bitsize?  Maybe "bit shift count for..." or such.

Yeah, maybe a _SHIFT suffix would have been better fit for these configs
instead of _BITS. Similar to PAGE_SHIFT or NODES_SHIFT. Anyways, I can
send a patch to improve the help text. How about something like:
   "Size for ... (as a power of 2)"

I'll also send a separate patch to fix the rest of the upper limits. Any
of the (1 << 30) shifts allocates static arrays that blow past the .bss
segment. Just as originally reporeted by J. R. Okajima.

--
Carlos Llamas

Re: [PATCH v2] lockdep: upper limit LOCKDEP_CHAINS_BITS

[Carlos Llamas]

On Mon, Aug 05, 2024 at 07:58:07PM +0000, Carlos Llamas wrote:
> On Wed, Jul 31, 2024 at 04:48:23PM -0700, Andrew Morton wrote:
> > btw, the help text "Bitsize for MAX_LOCKDEP_CHAINS" is odd.  What's a
> > bitsize?  Maybe "bit shift count for..." or such.
> 
> Yeah, maybe a _SHIFT suffix would have been better fit for these configs
> instead of _BITS. Similar to PAGE_SHIFT or NODES_SHIFT. Anyways, I can
> send a patch to improve the help text. How about something like:
>    "Size for ... (as a power of 2)"
> 
> I'll also send a separate patch to fix the rest of the upper limits. Any
> of the (1 << 30) shifts allocates static arrays that blow past the .bss
> segment. Just as originally reporeted by J. R. Okajima.

Ok, I've sent a patchset for these bits at:
https://lore.kernel.org/all/20240806010128.402852-1-cmllamas@google.com/

Thanks,
Carlos Llamas