Traceback with CONFIG_REGMAP_KUNIT=y+CONFIG_DEBUG_ATOMIC_SLEEP=y

Traceback with CONFIG_REGMAP_KUNIT=y+CONFIG_DEBUG_ATOMIC_SLEEP=y

[Guenter Roeck]

Hi,

when booting images with both CONFIG_REGMAP_KUNIT and
CONFIG_DEBUG_ATOMIC_SLEEP enabled, I get the following backtrace.

[    4.994957]     # Subtest: regmap
[    4.995067]     1..14
[    4.995190]         KTAP version 1
[    4.995308]         # Subtest: basic_read_write
[    4.999402]         ok 1 none
[    5.003028]         ok 2 flat
[    5.005510] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:306
[    5.005960] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 117, name: kunit_try_catch
[    5.006219] preempt_count: 1, expected: 0
[    5.006414] 1 lock held by kunit_try_catch/117:
[    5.006590]  #0: 833b9010 (regmap_kunit:86:(config)->lock){....}-{2:2}, at: regmap_lock_spinlock+0x14/0x1c
[    5.007493] irq event stamp: 162
[    5.007627] hardirqs last  enabled at (161): [<80786738>] crng_make_state+0x1a0/0x294
[    5.007871] hardirqs last disabled at (162): [<80c531ec>] _raw_spin_lock_irqsave+0x7c/0x80
[    5.008119] softirqs last  enabled at (0): [<801110ac>] copy_process+0x810/0x2138
[    5.008356] softirqs last disabled at (0): [<00000000>] 0x0
[    5.008688] CPU: 0 PID: 117 Comm: kunit_try_catch Tainted: G                 N 6.4.4-rc3-g0e8d2fdfb188 #1
[    5.009011] Hardware name: Generic DT based system
[    5.009277]  unwind_backtrace from show_stack+0x18/0x1c
[    5.009497]  show_stack from dump_stack_lvl+0x38/0x5c
[    5.009676]  dump_stack_lvl from __might_resched+0x188/0x2d0
[    5.009860]  __might_resched from __kmem_cache_alloc_node+0x1dc/0x25c
[    5.010061]  __kmem_cache_alloc_node from kmalloc_trace+0x30/0xc8
[    5.010254]  kmalloc_trace from regcache_rbtree_write+0x26c/0x468
[    5.010446]  regcache_rbtree_write from _regmap_write+0x88/0x140
[    5.010634]  _regmap_write from regmap_write+0x44/0x68
[    5.010803]  regmap_write from basic_read_write+0x8c/0x270
[    5.010980]  basic_read_write from kunit_try_run_case+0x48/0xa0
[    5.011170]  kunit_try_run_case from kunit_generic_run_threadfn_adapter+0x1c/0x28
[    5.011396]  kunit_generic_run_threadfn_adapter from kthread+0xf8/0x120
[    5.011603]  kthread from ret_from_fork+0x14/0x3c
[    5.011801] Exception stack(0x881a5fb0 to 0x881a5ff8)
[    5.012024] 5fa0:                                     00000000 00000000 00000000 00000000
[    5.012269] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    5.012502] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    5.014546]         ok 3 rbtree
[    5.018988]         ok 4 maple

This isn't surprising given that the regmap code allocates rbcache nodes
with GFP_KERNEL and the regmap configuration results in using spinlock
to lock regmap accesses.

I'll be happy to submit a patch to fix the problem, but I would need
some advice. I could

- Update the unit test regmap configuration to avoid using
  spinlock as locking mechanism for rbtree tests.
  That would work, but fail to catch situations where this happens
  in the real world.
- Use a different method to allocate memory in regcache_rbtree_node_alloc().
  - Replace GFP_KERNEL with GFP_NOWAIT or GFP_KERNEL | __GFP_NOWARN (?)
    or something else, possibly only if gfpflags_allow_blocking() is false
  - Use some other allocation function if gfpflags_allow_blocking()
    is false (which one ?)

Any suggestions ?

Thanks,
Guenter

Re: Traceback with CONFIG_REGMAP_KUNIT=y+CONFIG_DEBUG_ATOMIC_SLEEP=y

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 441 bytes --]

On Wed, Jul 19, 2023 at 03:37:54PM -0700, Guenter Roeck wrote:

> - Update the unit test regmap configuration to avoid using
>   spinlock as locking mechanism for rbtree tests.
>   That would work, but fail to catch situations where this happens
>   in the real world.

Do this.  The test is buggy, users should simply not do this.  Users who
need to allocate in atomic context need to use a cache type which does
not do dynamic allocation.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Traceback with CONFIG_REGMAP_KUNIT=y+CONFIG_DEBUG_ATOMIC_SLEEP=y

[Dan Carpenter]

On Wed, Jul 19, 2023 at 03:37:54PM -0700, Guenter Roeck wrote:
> Hi,
> 
> when booting images with both CONFIG_REGMAP_KUNIT and
> CONFIG_DEBUG_ATOMIC_SLEEP enabled, I get the following backtrace.
> 
> [    4.994957]     # Subtest: regmap
> [    4.995067]     1..14
> [    4.995190]         KTAP version 1
> [    4.995308]         # Subtest: basic_read_write
> [    4.999402]         ok 1 none
> [    5.003028]         ok 2 flat
> [    5.005510] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:306
> [    5.005960] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 117, name: kunit_try_catch
> [    5.006219] preempt_count: 1, expected: 0
> [    5.006414] 1 lock held by kunit_try_catch/117:
> [    5.006590]  #0: 833b9010 (regmap_kunit:86:(config)->lock){....}-{2:2}, at: regmap_lock_spinlock+0x14/0x1c
> [    5.007493] irq event stamp: 162
> [    5.007627] hardirqs last  enabled at (161): [<80786738>] crng_make_state+0x1a0/0x294
> [    5.007871] hardirqs last disabled at (162): [<80c531ec>] _raw_spin_lock_irqsave+0x7c/0x80
> [    5.008119] softirqs last  enabled at (0): [<801110ac>] copy_process+0x810/0x2138
> [    5.008356] softirqs last disabled at (0): [<00000000>] 0x0
> [    5.008688] CPU: 0 PID: 117 Comm: kunit_try_catch Tainted: G                 N 6.4.4-rc3-g0e8d2fdfb188 #1
> [    5.009011] Hardware name: Generic DT based system
> [    5.009277]  unwind_backtrace from show_stack+0x18/0x1c
> [    5.009497]  show_stack from dump_stack_lvl+0x38/0x5c
> [    5.009676]  dump_stack_lvl from __might_resched+0x188/0x2d0
> [    5.009860]  __might_resched from __kmem_cache_alloc_node+0x1dc/0x25c
> [    5.010061]  __kmem_cache_alloc_node from kmalloc_trace+0x30/0xc8
> [    5.010254]  kmalloc_trace from regcache_rbtree_write+0x26c/0x468
> [    5.010446]  regcache_rbtree_write from _regmap_write+0x88/0x140
> [    5.010634]  _regmap_write from regmap_write+0x44/0x68
> [    5.010803]  regmap_write from basic_read_write+0x8c/0x270
> [    5.010980]  basic_read_write from kunit_try_run_case+0x48/0xa0
> [    5.011170]  kunit_try_run_case from kunit_generic_run_threadfn_adapter+0x1c/0x28
> [    5.011396]  kunit_generic_run_threadfn_adapter from kthread+0xf8/0x120
> [    5.011603]  kthread from ret_from_fork+0x14/0x3c
> [    5.011801] Exception stack(0x881a5fb0 to 0x881a5ff8)
> [    5.012024] 5fa0:                                     00000000 00000000 00000000 00000000
> [    5.012269] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    5.012502] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    5.014546]         ok 3 rbtree
> [    5.018988]         ok 4 maple
> 
> This isn't surprising given that the regmap code allocates rbcache nodes
> with GFP_KERNEL and the regmap configuration results in using spinlock
> to lock regmap accesses.
> 
> I'll be happy to submit a patch to fix the problem, but I would need
> some advice. I could
> 
> - Update the unit test regmap configuration to avoid using
>   spinlock as locking mechanism for rbtree tests.
>   That would work, but fail to catch situations where this happens
>   in the real world.
> - Use a different method to allocate memory in regcache_rbtree_node_alloc().
>   - Replace GFP_KERNEL with GFP_NOWAIT or GFP_KERNEL | __GFP_NOWARN (?)
>     or something else, possibly only if gfpflags_allow_blocking() is false

__GFP_NOWARN is only about allocation failure, not sleeping.  I would
have thought it should be.

diff --git a/drivers/base/regmap/regcache-rbtree.c b/drivers/base/regmap/regcache-rbtree.c
index fabf87058d80..d94e25120f2f 100644
--- a/drivers/base/regmap/regcache-rbtree.c
+++ b/drivers/base/regmap/regcache-rbtree.c
@@ -187,7 +187,7 @@ static int regcache_rbtree_init(struct regmap *map)
 	int i;
 	int ret;
 
-	map->cache = kmalloc(sizeof *rbtree_ctx, GFP_KERNEL);
+	map->cache = kmalloc(sizeof *rbtree_ctx, map->alloc_flags);
 	if (!map->cache)
 		return -ENOMEM;
 
@@ -277,7 +277,7 @@ static int regcache_rbtree_insert_to_block(struct regmap *map,
 
 	blk = krealloc(rbnode->block,
 		       blklen * map->cache_word_size,
-		       GFP_KERNEL);
+		       map->alloc_flags);
 	if (!blk)
 		return -ENOMEM;
 
@@ -286,7 +286,7 @@ static int regcache_rbtree_insert_to_block(struct regmap *map,
 	if (BITS_TO_LONGS(blklen) > BITS_TO_LONGS(rbnode->blklen)) {
 		present = krealloc(rbnode->cache_present,
 				   BITS_TO_LONGS(blklen) * sizeof(*present),
-				   GFP_KERNEL);
+				   map->alloc_flags);
 		if (!present)
 			return -ENOMEM;
 
@@ -320,7 +320,7 @@ regcache_rbtree_node_alloc(struct regmap *map, unsigned int reg)
 	const struct regmap_range *range;
 	int i;
 
-	rbnode = kzalloc(sizeof(*rbnode), GFP_KERNEL);
+	rbnode = kzalloc(sizeof(*rbnode), map->alloc_flags);
 	if (!rbnode)
 		return NULL;
 
@@ -346,13 +346,13 @@ regcache_rbtree_node_alloc(struct regmap *map, unsigned int reg)
 	}
 
 	rbnode->block = kmalloc_array(rbnode->blklen, map->cache_word_size,
-				      GFP_KERNEL);
+				      map->alloc_flags);
 	if (!rbnode->block)
 		goto err_free;
 
 	rbnode->cache_present = kcalloc(BITS_TO_LONGS(rbnode->blklen),
 					sizeof(*rbnode->cache_present),
-					GFP_KERNEL);
+					map->alloc_flags);
 	if (!rbnode->cache_present)
 		goto err_free_block;
 

Re: Traceback with CONFIG_REGMAP_KUNIT=y+CONFIG_DEBUG_ATOMIC_SLEEP=y

[Guenter Roeck]

On 7/20/23 01:50, Dan Carpenter wrote:
> On Wed, Jul 19, 2023 at 03:37:54PM -0700, Guenter Roeck wrote:
>> Hi,
>>
>> when booting images with both CONFIG_REGMAP_KUNIT and
>> CONFIG_DEBUG_ATOMIC_SLEEP enabled, I get the following backtrace.
>>
>> [    4.994957]     # Subtest: regmap
>> [    4.995067]     1..14
>> [    4.995190]         KTAP version 1
>> [    4.995308]         # Subtest: basic_read_write
>> [    4.999402]         ok 1 none
>> [    5.003028]         ok 2 flat
>> [    5.005510] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:306
>> [    5.005960] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 117, name: kunit_try_catch
>> [    5.006219] preempt_count: 1, expected: 0
>> [    5.006414] 1 lock held by kunit_try_catch/117:
>> [    5.006590]  #0: 833b9010 (regmap_kunit:86:(config)->lock){....}-{2:2}, at: regmap_lock_spinlock+0x14/0x1c
>> [    5.007493] irq event stamp: 162
>> [    5.007627] hardirqs last  enabled at (161): [<80786738>] crng_make_state+0x1a0/0x294
>> [    5.007871] hardirqs last disabled at (162): [<80c531ec>] _raw_spin_lock_irqsave+0x7c/0x80
>> [    5.008119] softirqs last  enabled at (0): [<801110ac>] copy_process+0x810/0x2138
>> [    5.008356] softirqs last disabled at (0): [<00000000>] 0x0
>> [    5.008688] CPU: 0 PID: 117 Comm: kunit_try_catch Tainted: G                 N 6.4.4-rc3-g0e8d2fdfb188 #1
>> [    5.009011] Hardware name: Generic DT based system
>> [    5.009277]  unwind_backtrace from show_stack+0x18/0x1c
>> [    5.009497]  show_stack from dump_stack_lvl+0x38/0x5c
>> [    5.009676]  dump_stack_lvl from __might_resched+0x188/0x2d0
>> [    5.009860]  __might_resched from __kmem_cache_alloc_node+0x1dc/0x25c
>> [    5.010061]  __kmem_cache_alloc_node from kmalloc_trace+0x30/0xc8
>> [    5.010254]  kmalloc_trace from regcache_rbtree_write+0x26c/0x468
>> [    5.010446]  regcache_rbtree_write from _regmap_write+0x88/0x140
>> [    5.010634]  _regmap_write from regmap_write+0x44/0x68
>> [    5.010803]  regmap_write from basic_read_write+0x8c/0x270
>> [    5.010980]  basic_read_write from kunit_try_run_case+0x48/0xa0
>> [    5.011170]  kunit_try_run_case from kunit_generic_run_threadfn_adapter+0x1c/0x28
>> [    5.011396]  kunit_generic_run_threadfn_adapter from kthread+0xf8/0x120
>> [    5.011603]  kthread from ret_from_fork+0x14/0x3c
>> [    5.011801] Exception stack(0x881a5fb0 to 0x881a5ff8)
>> [    5.012024] 5fa0:                                     00000000 00000000 00000000 00000000
>> [    5.012269] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> [    5.012502] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
>> [    5.014546]         ok 3 rbtree
>> [    5.018988]         ok 4 maple
>>
>> This isn't surprising given that the regmap code allocates rbcache nodes
>> with GFP_KERNEL and the regmap configuration results in using spinlock
>> to lock regmap accesses.
>>
>> I'll be happy to submit a patch to fix the problem, but I would need
>> some advice. I could
>>
>> - Update the unit test regmap configuration to avoid using
>>    spinlock as locking mechanism for rbtree tests.
>>    That would work, but fail to catch situations where this happens
>>    in the real world.
>> - Use a different method to allocate memory in regcache_rbtree_node_alloc().
>>    - Replace GFP_KERNEL with GFP_NOWAIT or GFP_KERNEL | __GFP_NOWARN (?)
>>      or something else, possibly only if gfpflags_allow_blocking() is false
> 
> __GFP_NOWARN is only about allocation failure, not sleeping.  I would
> have thought it should be.
> 
> diff --git a/drivers/base/regmap/regcache-rbtree.c b/drivers/base/regmap/regcache-rbtree.c
> index fabf87058d80..d94e25120f2f 100644
> --- a/drivers/base/regmap/regcache-rbtree.c
> +++ b/drivers/base/regmap/regcache-rbtree.c
> @@ -187,7 +187,7 @@ static int regcache_rbtree_init(struct regmap *map)
>   	int i;
>   	int ret;
>   
> -	map->cache = kmalloc(sizeof *rbtree_ctx, GFP_KERNEL);
> +	map->cache = kmalloc(sizeof *rbtree_ctx, map->alloc_flags);

Yes, that might work as well (and after looking more deeply into the code
I wondered why it wasn't used in the first place).

Based on Mark's feedback I submitted
https://lore.kernel.org/lkml/20230720032848.1306349-1-linux@roeck-us.net/
Sorry, I forgot to copy you on that one.

Mark, please let me know what you prefer.

Thanks,
Guenter

>   	if (!map->cache)
>   		return -ENOMEM;
>   
> @@ -277,7 +277,7 @@ static int regcache_rbtree_insert_to_block(struct regmap *map,
>   
>   	blk = krealloc(rbnode->block,
>   		       blklen * map->cache_word_size,
> -		       GFP_KERNEL);
> +		       map->alloc_flags);
>   	if (!blk)
>   		return -ENOMEM;
>   
> @@ -286,7 +286,7 @@ static int regcache_rbtree_insert_to_block(struct regmap *map,
>   	if (BITS_TO_LONGS(blklen) > BITS_TO_LONGS(rbnode->blklen)) {
>   		present = krealloc(rbnode->cache_present,
>   				   BITS_TO_LONGS(blklen) * sizeof(*present),
> -				   GFP_KERNEL);
> +				   map->alloc_flags);
>   		if (!present)
>   			return -ENOMEM;
>   
> @@ -320,7 +320,7 @@ regcache_rbtree_node_alloc(struct regmap *map, unsigned int reg)
>   	const struct regmap_range *range;
>   	int i;
>   
> -	rbnode = kzalloc(sizeof(*rbnode), GFP_KERNEL);
> +	rbnode = kzalloc(sizeof(*rbnode), map->alloc_flags);
>   	if (!rbnode)
>   		return NULL;
>   
> @@ -346,13 +346,13 @@ regcache_rbtree_node_alloc(struct regmap *map, unsigned int reg)
>   	}
>   
>   	rbnode->block = kmalloc_array(rbnode->blklen, map->cache_word_size,
> -				      GFP_KERNEL);
> +				      map->alloc_flags);
>   	if (!rbnode->block)
>   		goto err_free;
>   
>   	rbnode->cache_present = kcalloc(BITS_TO_LONGS(rbnode->blklen),
>   					sizeof(*rbnode->cache_present),
> -					GFP_KERNEL);
> +					map->alloc_flags);
>   	if (!rbnode->cache_present)
>   		goto err_free_block;
>   

Re: Traceback with CONFIG_REGMAP_KUNIT=y+CONFIG_DEBUG_ATOMIC_SLEEP=y

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 859 bytes --]

On Thu, Jul 20, 2023 at 07:26:54AM -0700, Guenter Roeck wrote:
> On 7/20/23 01:50, Dan Carpenter wrote:

> > +++ b/drivers/base/regmap/regcache-rbtree.c
> > @@ -187,7 +187,7 @@ static int regcache_rbtree_init(struct regmap *map)
> >   	int i;
> >   	int ret;
> > -	map->cache = kmalloc(sizeof *rbtree_ctx, GFP_KERNEL);
> > +	map->cache = kmalloc(sizeof *rbtree_ctx, map->alloc_flags);

> Yes, that might work as well (and after looking more deeply into the code
> I wondered why it wasn't used in the first place).

> Based on Mark's feedback I submitted
> https://lore.kernel.org/lkml/20230720032848.1306349-1-linux@roeck-us.net/
> Sorry, I forgot to copy you on that one.

> Mark, please let me know what you prefer.

They're both independently fine, but I wouldn't expect anything that's
running in atomic context to be actually using dynamic allocations.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Traceback with CONFIG_REGMAP_KUNIT=y+CONFIG_DEBUG_ATOMIC_SLEEP=y

[Guenter Roeck]

On 7/20/23 07:31, Mark Brown wrote:
> On Thu, Jul 20, 2023 at 07:26:54AM -0700, Guenter Roeck wrote:
>> On 7/20/23 01:50, Dan Carpenter wrote:
> 
>>> +++ b/drivers/base/regmap/regcache-rbtree.c
>>> @@ -187,7 +187,7 @@ static int regcache_rbtree_init(struct regmap *map)
>>>    	int i;
>>>    	int ret;
>>> -	map->cache = kmalloc(sizeof *rbtree_ctx, GFP_KERNEL);
>>> +	map->cache = kmalloc(sizeof *rbtree_ctx, map->alloc_flags);
> 
>> Yes, that might work as well (and after looking more deeply into the code
>> I wondered why it wasn't used in the first place).
> 
>> Based on Mark's feedback I submitted
>> https://lore.kernel.org/lkml/20230720032848.1306349-1-linux@roeck-us.net/
>> Sorry, I forgot to copy you on that one.
> 
>> Mark, please let me know what you prefer.
> 
> They're both independently fine, but I wouldn't expect anything that's
> running in atomic context to be actually using dynamic allocations.

Which one do you prefer ? As I mentioned in my second patch, there are
two drivers which use fast_io together with REGCACHE_RBTREE and thus
are likely affected by this problem. Dan's solution would cover that,
while my current RFC patch would likely cause those drivers to fail.
Plus, of course, they could get stuck if they actually end up trying to
sleep while allocating memory.

Thanks,
Guenter

Re: Traceback with CONFIG_REGMAP_KUNIT=y+CONFIG_DEBUG_ATOMIC_SLEEP=y

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

On Thu, Jul 20, 2023 at 08:03:13AM -0700, Guenter Roeck wrote:
> On 7/20/23 07:31, Mark Brown wrote:

> > They're both independently fine, but I wouldn't expect anything that's
> > running in atomic context to be actually using dynamic allocations.

> Which one do you prefer ? As I mentioned in my second patch, there are
> two drivers which use fast_io together with REGCACHE_RBTREE and thus
> are likely affected by this problem. Dan's solution would cover that,
> while my current RFC patch would likely cause those drivers to fail.
> Plus, of course, they could get stuck if they actually end up trying to
> sleep while allocating memory.

Like I say I don't think it's an either/or - we can do both
independently, they both make sense standalone and don't conflict with
each other.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Traceback with CONFIG_REGMAP_KUNIT=y+CONFIG_DEBUG_ATOMIC_SLEEP=y

[Guenter Roeck]

On 7/20/23 08:07, Mark Brown wrote:
> On Thu, Jul 20, 2023 at 08:03:13AM -0700, Guenter Roeck wrote:
>> On 7/20/23 07:31, Mark Brown wrote:
> 
>>> They're both independently fine, but I wouldn't expect anything that's
>>> running in atomic context to be actually using dynamic allocations.
> 
>> Which one do you prefer ? As I mentioned in my second patch, there are
>> two drivers which use fast_io together with REGCACHE_RBTREE and thus
>> are likely affected by this problem. Dan's solution would cover that,
>> while my current RFC patch would likely cause those drivers to fail.
>> Plus, of course, they could get stuck if they actually end up trying to
>> sleep while allocating memory.
> 
> Like I say I don't think it's an either/or - we can do both
> independently, they both make sense standalone and don't conflict with
> each other.

I guess I am missing something. I have not tried it, but wouldn't my patches
be unnecessary if Dan's patch is applied ?

Thanks,
Guenter

Re: Traceback with CONFIG_REGMAP_KUNIT=y+CONFIG_DEBUG_ATOMIC_SLEEP=y

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

On Thu, Jul 20, 2023 at 09:25:42AM -0700, Guenter Roeck wrote:
> On 7/20/23 08:07, Mark Brown wrote:

> > Like I say I don't think it's an either/or - we can do both
> > independently, they both make sense standalone and don't conflict with
> > each other.

> I guess I am missing something. I have not tried it, but wouldn't my patches
> be unnecessary if Dan's patch is applied ?

Dan's patch doesn't cover the maple tree cache so there's still an issue
there, but yes once Dan's patch is applied (assuming it covered all
cases) the tests should run fine modulo any issues caused by trying to
do atomic allocations triggering us to fail allocations on smaller
systems or something.  My inclination is to do things like that in -next
and send your simpler approach to Linus, though it's really not
something I think it's a good idea for anyone to actually try to use.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

Re: Traceback with CONFIG_REGMAP_KUNIT=y+CONFIG_DEBUG_ATOMIC_SLEEP=y

[Guenter Roeck]

On 7/20/23 09:25, Guenter Roeck wrote:
> On 7/20/23 08:07, Mark Brown wrote:
>> On Thu, Jul 20, 2023 at 08:03:13AM -0700, Guenter Roeck wrote:
>>> On 7/20/23 07:31, Mark Brown wrote:
>>
>>>> They're both independently fine, but I wouldn't expect anything that's
>>>> running in atomic context to be actually using dynamic allocations.
>>
>>> Which one do you prefer ? As I mentioned in my second patch, there are
>>> two drivers which use fast_io together with REGCACHE_RBTREE and thus
>>> are likely affected by this problem. Dan's solution would cover that,
>>> while my current RFC patch would likely cause those drivers to fail.
>>> Plus, of course, they could get stuck if they actually end up trying to
>>> sleep while allocating memory.
>>
>> Like I say I don't think it's an either/or - we can do both
>> independently, they both make sense standalone and don't conflict with
>> each other.
> 
> I guess I am missing something. I have not tried it, but wouldn't my patches
> be unnecessary if Dan's patch is applied ?
> 

Actually, Dan's patch isn't complete. With it applied:

[    4.816104]     # Subtest: regmap
[    4.816175]     1..22
[    4.816266]         KTAP version 1
[    4.816343]         # Subtest: basic_read_write
[    4.821773]         ok 1 none
[    4.827032]         ok 2 flat
[    4.832404]         ok 3 rbtree
[    4.834664] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:306
[    4.834935] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 167, name: kunit_try_catch
[    4.835059] preempt_count: 1, expected: 0
[    4.835198] 1 lock held by kunit_try_catch/167:
[    4.835297]  #0: 838e9c10 (regmap_kunit:86:(config)->lock){....}-{2:2}, at: regmap_lock_spinlock+0x14/0x1c
[    4.835980] irq event stamp: 146
[    4.836057] hardirqs last  enabled at (145): [<8078bfa8>] crng_make_state+0x1a0/0x294
[    4.836176] hardirqs last disabled at (146): [<80c5f62c>] _raw_spin_lock_irqsave+0x7c/0x80
[    4.836297] softirqs last  enabled at (0): [<80110cc4>] copy_process+0x810/0x216c
[    4.836413] softirqs last disabled at (0): [<00000000>] 0x0
[    4.836628] CPU: 0 PID: 167 Comm: kunit_try_catch Tainted: G                 N 6.5.0-rc1-00028-gc4be22597a36-dirty #6
[    4.836809] Hardware name: Generic DT based system
[    4.837002]  unwind_backtrace from show_stack+0x18/0x1c
[    4.837134]  show_stack from dump_stack_lvl+0x38/0x5c
[    4.837229]  dump_stack_lvl from __might_resched+0x188/0x2d0
[    4.837325]  __might_resched from __kmem_cache_alloc_node+0x1f4/0x258
[    4.837426]  __kmem_cache_alloc_node from __kmalloc+0x48/0x170
[    4.837521]  __kmalloc from regcache_maple_write+0x194/0x248
[    4.837617]  regcache_maple_write from _regmap_write+0x88/0x140
[    4.837711]  _regmap_write from regmap_write+0x44/0x68
[    4.837797]  regmap_write from basic_read_write+0x8c/0x27c
[    4.837889]  basic_read_write from kunit_generic_run_threadfn_adapter+0x1c/0x28
[    4.837996]  kunit_generic_run_threadfn_adapter from kthread+0xf8/0x120
[    4.838099]  kthread from ret_from_fork+0x14/0x3c
[    4.838214] Exception stack(0x881a5fb0 to 0x881a5ff8)
[    4.838346] 5fa0:                                     00000000 00000000 00000000 00000000
[    4.838465] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    4.838576] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    4.841868]         ok 4 maple
[    4.841923]     # basic_read_write: pass:4 fail:0 skip:0 total:4
[    4.842022]     ok 1 basic_read_write

It would have to be extended to also address the same problem in the maple tree
code. Also, the change would probably not be needed in regcache_rbtree_init().

After adding the GFP_KERNEL -> map->alloc_flags changes to the maple tree
code while skipping the init functions, I no longer see the traceback.
This is without my patches.

Thanks,
Guenter

Re: Traceback with CONFIG_REGMAP_KUNIT=y+CONFIG_DEBUG_ATOMIC_SLEEP=y

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 505 bytes --]

On Thu, Jul 20, 2023 at 09:41:42AM -0700, Guenter Roeck wrote:

> After adding the GFP_KERNEL -> map->alloc_flags changes to the maple tree
> code while skipping the init functions, I no longer see the traceback.
> This is without my patches.

Ah, excellent - yes, that should sort it.  I've already queued your
original patch for CI for 6.5, we could then apply both your new patch
and Dan's for 6.6 to support anyone who for some reason does want to try
dynamic allocation at runtime in atomic context.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Traceback with CONFIG_REGMAP_KUNIT=y+CONFIG_DEBUG_ATOMIC_SLEEP=y

[Guenter Roeck]

On 7/20/23 09:48, Mark Brown wrote:
> On Thu, Jul 20, 2023 at 09:41:42AM -0700, Guenter Roeck wrote:
> 
>> After adding the GFP_KERNEL -> map->alloc_flags changes to the maple tree
>> code while skipping the init functions, I no longer see the traceback.
>> This is without my patches.
> 
> Ah, excellent - yes, that should sort it.  I've already queued your
> original patch for CI for 6.5, we could then apply both your new patch
> and Dan's for 6.6 to support anyone who for some reason does want to try
> dynamic allocation at runtime in atomic context.

Makes sense. I submitted a patch to address the problem in the maple tree
code. I assume Dan will submit one for the rbtree code. Dan, let me know
if you want me to handle that one as well.

Thanks,
Guenter