[PATCH RT] mm: do not warn for suspend when allocate memory on RT

[PATCH RT] mm: do not warn for suspend when allocate memory on RT

[Liwei Song]

Interrupts are off during resume from RAM, this will triger a warning
when allocate memory in non-preemptible context on RT since commit
b5d5bc970f209 ("mm: Warn on memory allocation in non-preemptible
context on RT"), exclude suspend from this warning check.

Fixes: b5d5bc970f209 ("mm: Warn on memory allocation in non-preemptible context on RT")
Signed-off-by: Liwei Song <liwei.song@windriver.com>
---
 mm/slub.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 1929645daa53..ebff24ae50ea 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2771,7 +2771,8 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
 	unsigned long tid;
 
 	if (IS_ENABLED(CONFIG_PREEMPT_RT) && IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP))
-		WARN_ON_ONCE(!preemptible() && system_state >= SYSTEM_SCHEDULING);
+		WARN_ON_ONCE(!preemptible() && system_state >= SYSTEM_SCHEDULING &&
+				system_state != SYSTEM_SUSPEND);
 
 	s = slab_pre_alloc_hook(s, gfpflags);
 	if (!s)
@@ -3236,7 +3237,8 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
 	int i;
 
 	if (IS_ENABLED(CONFIG_PREEMPT_RT) && IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP))
-		WARN_ON_ONCE(!preemptible() && system_state >= SYSTEM_SCHEDULING);
+		WARN_ON_ONCE(!preemptible() && system_state >= SYSTEM_SCHEDULING &&
+				system_state != SYSTEM_SUSPEND);
 
 	/* memcg and kmem_cache debug support */
 	s = slab_pre_alloc_hook(s, flags);
-- 
2.17.1

Re: [PATCH RT] mm: do not warn for suspend when allocate memory on RT

[Sebastian Andrzej Siewior]

On 2020-04-13 12:18:17 [+0800], Liwei Song wrote:
> Interrupts are off during resume from RAM, this will triger a warning
> when allocate memory in non-preemptible context on RT since commit
> b5d5bc970f209 ("mm: Warn on memory allocation in non-preemptible
> context on RT"), exclude suspend from this warning check.

Is this the ACPI backtrace or do you have something else?

I'm currently not 100% sure if we can skip the warning.

> Fixes: b5d5bc970f209 ("mm: Warn on memory allocation in non-preemptible context on RT")
> Signed-off-by: Liwei Song <liwei.song@windriver.com>
> ---
>  mm/slub.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 1929645daa53..ebff24ae50ea 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2771,7 +2771,8 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
>  	unsigned long tid;
>  
>  	if (IS_ENABLED(CONFIG_PREEMPT_RT) && IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP))
> -		WARN_ON_ONCE(!preemptible() && system_state >= SYSTEM_SCHEDULING);
> +		WARN_ON_ONCE(!preemptible() && system_state >= SYSTEM_SCHEDULING &&
> +				system_state != SYSTEM_SUSPEND);
>  
>  	s = slab_pre_alloc_hook(s, gfpflags);
>  	if (!s)
> @@ -3236,7 +3237,8 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
>  	int i;
>  
>  	if (IS_ENABLED(CONFIG_PREEMPT_RT) && IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP))
> -		WARN_ON_ONCE(!preemptible() && system_state >= SYSTEM_SCHEDULING);
> +		WARN_ON_ONCE(!preemptible() && system_state >= SYSTEM_SCHEDULING &&
> +				system_state != SYSTEM_SUSPEND);
>  
>  	/* memcg and kmem_cache debug support */
>  	s = slab_pre_alloc_hook(s, flags);

Sebastian

Re: [PATCH RT] mm: do not warn for suspend when allocate memory on RT

[Liwei Song]

On 4/17/20 01:09, Sebastian Andrzej Siewior wrote:
> On 2020-04-13 12:18:17 [+0800], Liwei Song wrote:
>> Interrupts are off during resume from RAM, this will triger a warning
>> when allocate memory in non-preemptible context on RT since commit
>> b5d5bc970f209 ("mm: Warn on memory allocation in non-preemptible
>> context on RT"), exclude suspend from this warning check.
> 
> Is this the ACPI backtrace or do you have something else?

Yes, there are two backtrace here include one ACPI Calltrace, another one
is from iommu:

1).
[ 88.992342] ? acpi_os_allocate_zeroed+0x28/0x2a
[ 88.992349] acpi_os_allocate_zeroed+0x28/0x2a
[ 88.992351] acpi_ns_internalize_name+0x4b/0xa0
[ 88.992354] ? trace_preempt_on+0x2a/0x120
[ 88.992360] acpi_ns_get_node_unlocked+0x6f/0xd7
[ 88.992362] ? _raw_spin_unlock_irqrestore+0x42/0x90
[ 88.992367] ? down_timeout+0x3c/0x60
[ 88.992372] ? acpi_os_wait_semaphore+0x4d/0x70
[ 88.992378] acpi_ns_get_node+0x43/0x5d

2).
[ 88.991560] Call Trace:
[ 88.991561] ? _raw_spin_unlock_irqrestore+0x42/0x90
[ 88.991563] ? iommu_suspend+0x4f/0x1c0
[ 88.991569] iommu_suspend+0x4f/0x1c0
[ 88.991572] syscore_suspend+0x9b/0x3e0
[ 88.991575] suspend_devices_and_enter+0x1fb/0xec0
[ 88.991579] ? rcu_read_lock_sched_held+0x4f/0x80
[ 88.991580] ? prb_unlock+0x18/0x80
[ 88.991585] pm_suspend.cold+0x326/0x37b

During suspend phase the only way to make preemptible() happy is
enable interrupt before kcalloc/kmalloc/kzalloc, but enable interrupt
may cause some unknown issue if device access I/O address which already
suspend, so I think it's better and safe to exclude suspend from this
warning check.

Thanks,
Liwei.


> 
> I'm currently not 100% sure if we can skip the warning.
> 
>> Fixes: b5d5bc970f209 ("mm: Warn on memory allocation in non-preemptible context on RT")
>> Signed-off-by: Liwei Song <liwei.song@windriver.com>
>> ---
>>  mm/slub.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 1929645daa53..ebff24ae50ea 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -2771,7 +2771,8 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
>>  	unsigned long tid;
>>  
>>  	if (IS_ENABLED(CONFIG_PREEMPT_RT) && IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP))
>> -		WARN_ON_ONCE(!preemptible() && system_state >= SYSTEM_SCHEDULING);
>> +		WARN_ON_ONCE(!preemptible() && system_state >= SYSTEM_SCHEDULING &&
>> +				system_state != SYSTEM_SUSPEND);
>>  
>>  	s = slab_pre_alloc_hook(s, gfpflags);
>>  	if (!s)
>> @@ -3236,7 +3237,8 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
>>  	int i;
>>  
>>  	if (IS_ENABLED(CONFIG_PREEMPT_RT) && IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP))
>> -		WARN_ON_ONCE(!preemptible() && system_state >= SYSTEM_SCHEDULING);
>> +		WARN_ON_ONCE(!preemptible() && system_state >= SYSTEM_SCHEDULING &&
>> +				system_state != SYSTEM_SUSPEND);
>>  
>>  	/* memcg and kmem_cache debug support */
>>  	s = slab_pre_alloc_hook(s, flags);
> 
> Sebastian
> 

Re: [PATCH RT] mm: do not warn for suspend when allocate memory on RT

[Sebastian Andrzej Siewior]

On 2020-04-17 10:50:35 [+0800], Liwei Song wrote:
> 
> 
> On 4/17/20 01:09, Sebastian Andrzej Siewior wrote:
> > On 2020-04-13 12:18:17 [+0800], Liwei Song wrote:
> >> Interrupts are off during resume from RAM, this will triger a warning
> >> when allocate memory in non-preemptible context on RT since commit
> >> b5d5bc970f209 ("mm: Warn on memory allocation in non-preemptible
> >> context on RT"), exclude suspend from this warning check.
> > 
> > Is this the ACPI backtrace or do you have something else?
> 
> Yes, there are two backtrace here include one ACPI Calltrace, another one
> is from iommu:
> 
> 1).
> [ 88.992342] ? acpi_os_allocate_zeroed+0x28/0x2a
> [ 88.992349] acpi_os_allocate_zeroed+0x28/0x2a
> [ 88.992351] acpi_ns_internalize_name+0x4b/0xa0
> [ 88.992354] ? trace_preempt_on+0x2a/0x120
> [ 88.992360] acpi_ns_get_node_unlocked+0x6f/0xd7
> [ 88.992362] ? _raw_spin_unlock_irqrestore+0x42/0x90
> [ 88.992367] ? down_timeout+0x3c/0x60
> [ 88.992372] ? acpi_os_wait_semaphore+0x4d/0x70
> [ 88.992378] acpi_ns_get_node+0x43/0x5d
> 
> 2).
> [ 88.991560] Call Trace:
> [ 88.991561] ? _raw_spin_unlock_irqrestore+0x42/0x90
> [ 88.991563] ? iommu_suspend+0x4f/0x1c0
> [ 88.991569] iommu_suspend+0x4f/0x1c0
> [ 88.991572] syscore_suspend+0x9b/0x3e0
> [ 88.991575] suspend_devices_and_enter+0x1fb/0xec0
> [ 88.991579] ? rcu_read_lock_sched_held+0x4f/0x80
> [ 88.991580] ? prb_unlock+0x18/0x80
> [ 88.991585] pm_suspend.cold+0x326/0x37b
> 
> During suspend phase the only way to make preemptible() happy is
> enable interrupt before kcalloc/kmalloc/kzalloc, but enable interrupt
> may cause some unknown issue if device access I/O address which already
> suspend, so I think it's better and safe to exclude suspend from this
> warning check.

At this point all CPUs are down so there is no scheduling. No lock
within the buddy allocator should not be acquired by another task. That
looks good. 
But: calling into the buddy allocator goes via allocate_slab() which
enables interrupts.

> Thanks,
> Liwei.

Sebastian

Re: [PATCH RT] mm: do not warn for suspend when allocate memory on RT

[Liwei Song]

On 4/21/20 22:51, Sebastian Andrzej Siewior wrote:
> On 2020-04-17 10:50:35 [+0800], Liwei Song wrote:
>>
>>
>> On 4/17/20 01:09, Sebastian Andrzej Siewior wrote:
>>> On 2020-04-13 12:18:17 [+0800], Liwei Song wrote:
>>>> Interrupts are off during resume from RAM, this will triger a warning
>>>> when allocate memory in non-preemptible context on RT since commit
>>>> b5d5bc970f209 ("mm: Warn on memory allocation in non-preemptible
>>>> context on RT"), exclude suspend from this warning check.
>>>
>>> Is this the ACPI backtrace or do you have something else?
>>
>> Yes, there are two backtrace here include one ACPI Calltrace, another one
>> is from iommu:
>>
>> 1).
>> [ 88.992342] ? acpi_os_allocate_zeroed+0x28/0x2a
>> [ 88.992349] acpi_os_allocate_zeroed+0x28/0x2a
>> [ 88.992351] acpi_ns_internalize_name+0x4b/0xa0
>> [ 88.992354] ? trace_preempt_on+0x2a/0x120
>> [ 88.992360] acpi_ns_get_node_unlocked+0x6f/0xd7
>> [ 88.992362] ? _raw_spin_unlock_irqrestore+0x42/0x90
>> [ 88.992367] ? down_timeout+0x3c/0x60
>> [ 88.992372] ? acpi_os_wait_semaphore+0x4d/0x70
>> [ 88.992378] acpi_ns_get_node+0x43/0x5d
>>
>> 2).
>> [ 88.991560] Call Trace:
>> [ 88.991561] ? _raw_spin_unlock_irqrestore+0x42/0x90
>> [ 88.991563] ? iommu_suspend+0x4f/0x1c0
>> [ 88.991569] iommu_suspend+0x4f/0x1c0
>> [ 88.991572] syscore_suspend+0x9b/0x3e0
>> [ 88.991575] suspend_devices_and_enter+0x1fb/0xec0
>> [ 88.991579] ? rcu_read_lock_sched_held+0x4f/0x80
>> [ 88.991580] ? prb_unlock+0x18/0x80
>> [ 88.991585] pm_suspend.cold+0x326/0x37b
>>
>> During suspend phase the only way to make preemptible() happy is
>> enable interrupt before kcalloc/kmalloc/kzalloc, but enable interrupt
>> may cause some unknown issue if device access I/O address which already
>> suspend, so I think it's better and safe to exclude suspend from this
>> warning check.
> 
> At this point all CPUs are down so there is no scheduling. No lock
> within the buddy allocator should not be acquired by another task. That
> looks good. 
> But: calling into the buddy allocator goes via allocate_slab() which
> enables interrupts.

I trace the invoke path, allocate_slab() will not be called during
suspend, and if enable interrupt before request memory when suspend, it will cause
many ACPI calltrace, kmem_cache_alloc_bulk() will not
be called during suspend, while slab_alloc_node() will be called,
how about just exclude suspend from slab_alloc_node()?

Thanks,
Liwei.


> 
>> Thanks,
>> Liwei.
> 
> Sebastian
> 

[PATCH RT] mm: Don't warn about atomic memory allocations during suspend

[Sebastian Andrzej Siewior]

From: Liwei Song <liwei.song@windriver.com>

The ACPI code allocates larger amount of memory during resume. This
triggers a warning because the allocation happens with disabled
interrupts.
At this stage only one CPU is active so there should be no lock
contention. If SLUB needs to call into the buddy allocator for more
memory then it should not enable interrupts.

Limit the check to system state with more CPUs and scheduling and only
enable interrupts in SLUB at this stage.

Signed-off-by: Liwei Song <liwei.song@windriver.com>
[bigeasy: commit description, allocate_slab() hunk]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---

Are you okay with this Liwei?

 mm/slub.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 15c194ff16e6e..6ea1057edf1ab 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1655,7 +1655,7 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
 		enableirqs = true;
 
 #ifdef CONFIG_PREEMPT_RT
-	if (system_state > SYSTEM_BOOTING)
+	if (system_state > SYSTEM_BOOTING && system_state < SYSTEM_SUSPEND)
 		enableirqs = true;
 #endif
 	if (enableirqs)
@@ -2771,7 +2771,8 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
 	unsigned long tid;
 
 	if (IS_ENABLED(CONFIG_PREEMPT_RT) && IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP))
-		WARN_ON_ONCE(!preemptible() && system_state >= SYSTEM_SCHEDULING);
+		WARN_ON_ONCE(!preemptible() &&
+			     (system_state > SYSTEM_BOOTING && system_state < SYSTEM_SUSPEND));
 
 	s = slab_pre_alloc_hook(s, gfpflags);
 	if (!s)
@@ -3236,7 +3237,8 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
 	int i;
 
 	if (IS_ENABLED(CONFIG_PREEMPT_RT) && IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP))
-		WARN_ON_ONCE(!preemptible() && system_state >= SYSTEM_SCHEDULING);
+		WARN_ON_ONCE(!preemptible() &&
+			     (system_state > SYSTEM_BOOTING && system_state < SYSTEM_SUSPEND));
 
 	/* memcg and kmem_cache debug support */
 	s = slab_pre_alloc_hook(s, flags);
-- 
2.26.2

Re: [PATCH RT] mm: Don't warn about atomic memory allocations during suspend

[Liwei Song]

On 5/4/20 22:53, Sebastian Andrzej Siewior wrote:
> From: Liwei Song <liwei.song@windriver.com>
> 
> The ACPI code allocates larger amount of memory during resume. This
> triggers a warning because the allocation happens with disabled
> interrupts.
> At this stage only one CPU is active so there should be no lock
> contention. If SLUB needs to call into the buddy allocator for more
> memory then it should not enable interrupts.
> 
> Limit the check to system state with more CPUs and scheduling and only
> enable interrupts in SLUB at this stage.
> 
> Signed-off-by: Liwei Song <liwei.song@windriver.com>
> [bigeasy: commit description, allocate_slab() hunk]
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> 
> Are you okay with this Liwei?

Yes, I'm fine with this, thanks for your help.

Liwei.


> 
>  mm/slub.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 15c194ff16e6e..6ea1057edf1ab 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1655,7 +1655,7 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
>  		enableirqs = true;
>  
>  #ifdef CONFIG_PREEMPT_RT
> -	if (system_state > SYSTEM_BOOTING)
> +	if (system_state > SYSTEM_BOOTING && system_state < SYSTEM_SUSPEND)
>  		enableirqs = true;
>  #endif
>  	if (enableirqs)
> @@ -2771,7 +2771,8 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
>  	unsigned long tid;
>  
>  	if (IS_ENABLED(CONFIG_PREEMPT_RT) && IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP))
> -		WARN_ON_ONCE(!preemptible() && system_state >= SYSTEM_SCHEDULING);
> +		WARN_ON_ONCE(!preemptible() &&
> +			     (system_state > SYSTEM_BOOTING && system_state < SYSTEM_SUSPEND));
>  
>  	s = slab_pre_alloc_hook(s, gfpflags);
>  	if (!s)
> @@ -3236,7 +3237,8 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
>  	int i;
>  
>  	if (IS_ENABLED(CONFIG_PREEMPT_RT) && IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP))
> -		WARN_ON_ONCE(!preemptible() && system_state >= SYSTEM_SCHEDULING);
> +		WARN_ON_ONCE(!preemptible() &&
> +			     (system_state > SYSTEM_BOOTING && system_state < SYSTEM_SUSPEND));
>  
>  	/* memcg and kmem_cache debug support */
>  	s = slab_pre_alloc_hook(s, flags);
>