[PATCH] mm, slub: add cpus_read_lock/unlock() for slab_mem_going_offline_callback()

[PATCH] mm, slub: add cpus_read_lock/unlock() for slab_mem_going_offline_callback()

[qiang.zhang]

From: "Qiang.Zhang" <qiang.zhang@windriver.com>

The flush_all_cpus_locked() should be called with cpus_read_lock/unlock(),
ensure flush_cpu_slab() can be executed on schedule_on CPU.

Fixes: 1c84f3c91640 ("mm, slub: fix memory and cpu hotplug related lock ordering issues")
Signed-off-by: Qiang.Zhang <qiang.zhang@windriver.com>
---
 mm/slub.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/slub.c b/mm/slub.c
index 5543d57cb128..cf3f93abbd3e 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -4593,12 +4593,14 @@ static int slab_mem_going_offline_callback(void *arg)
 {
 	struct kmem_cache *s;
 
+	cpus_read_lock();
 	mutex_lock(&slab_mutex);
 	list_for_each_entry(s, &slab_caches, list) {
 		flush_all_cpus_locked(s);
 		__kmem_cache_do_shrink(s);
 	}
 	mutex_unlock(&slab_mutex);
+	cpus_read_unlock();
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH] mm, slub: add cpus_read_lock/unlock() for slab_mem_going_offline_callback()

[David Hildenbrand]

On 16.08.21 09:46, qiang.zhang@windriver.com wrote:
> From: "Qiang.Zhang" <qiang.zhang@windriver.com>
> 
> The flush_all_cpus_locked() should be called with cpus_read_lock/unlock(),
> ensure flush_cpu_slab() can be executed on schedule_on CPU.
> 
> Fixes: 1c84f3c91640 ("mm, slub: fix memory and cpu hotplug related lock ordering issues")

Which branch contains this commit? At least not linux.git or linux-next

> Signed-off-by: Qiang.Zhang <qiang.zhang@windriver.com>
> ---
>   mm/slub.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 5543d57cb128..cf3f93abbd3e 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -4593,12 +4593,14 @@ static int slab_mem_going_offline_callback(void *arg)
>   {
>   	struct kmem_cache *s;
>   
> +	cpus_read_lock();
>   	mutex_lock(&slab_mutex);
>   	list_for_each_entry(s, &slab_caches, list) {
>   		flush_all_cpus_locked(s);
>   		__kmem_cache_do_shrink(s);
>   	}
>   	mutex_unlock(&slab_mutex);
> +	cpus_read_unlock();
>   
>   	return 0;
>   }
> 

Memory notifiers are getting called from online_pages()/offline_pages(), 
where we call memory_notify(MEM_GOING_OFFLINE, &arg) under 
mem_hotplug_begin().

mem_hotplug_begin() does a cpus_read_lock().

How does this even work or against which branch is this?

-- 
Thanks,

David / dhildenb

Re: [PATCH] mm, slub: add cpus_read_lock/unlock() for slab_mem_going_offline_callback()

[Zhang, Qiang]

________________________________________
From: David Hildenbrand <david@redhat.com>
Sent: Monday, 16 August 2021 16:04
To: Zhang, Qiang; vbabka@suse.cz; akpm@linux-foundation.org; sfr@canb.auug.org.au
Cc: linux-mm@kvack.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm, slub: add cpus_read_lock/unlock() for slab_mem_going_offline_callback()

[Please note: This e-mail is from an EXTERNAL e-mail address]

On 16.08.21 09:46, qiang.zhang@windriver.com wrote:
> From: "Qiang.Zhang" <qiang.zhang@windriver.com>
>
> The flush_all_cpus_locked() should be called with cpus_read_lock/unlock(),
> ensure flush_cpu_slab() can be executed on schedule_on CPU.
>
> Fixes: 1c84f3c91640 ("mm, slub: fix memory and cpu hotplug related lock ordering issues")

>Which branch contains this commit? At least not linux.git or linux-next

> Signed-off-by: Qiang.Zhang <qiang.zhang@windriver.com>
> ---
>   mm/slub.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 5543d57cb128..cf3f93abbd3e 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -4593,12 +4593,14 @@ static int slab_mem_going_offline_callback(void *arg)
>   {
>       struct kmem_cache *s;
>
> +     cpus_read_lock();
>       mutex_lock(&slab_mutex);
>       list_for_each_entry(s, &slab_caches, list) {
>               flush_all_cpus_locked(s);
>               __kmem_cache_do_shrink(s);
>       }
>       mutex_unlock(&slab_mutex);
> +     cpus_read_unlock();
>
>       return 0;
>   }
>

>Memory notifiers are getting called from online_pages()/offline_pages(),
>where we call memory_notify(MEM_GOING_OFFLINE, &arg) under
>mem_hotplug_begin().
>
>mem_hotplug_begin() does a cpus_read_lock().

Thanks David
this is my mistake, sorry make noise.

>
>How does this even work or against which branch is this?
>
>--
>Thanks,
>
>David / dhildenb

Re: [PATCH] mm, slub: add cpus_read_lock/unlock() for slab_mem_going_offline_callback()

[Stephen Rothwell]

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

Hi David,

On Mon, 16 Aug 2021 10:04:13 +0200 David Hildenbrand <david@redhat.com> wrote:
>
> On 16.08.21 09:46, qiang.zhang@windriver.com wrote:
> > From: "Qiang.Zhang" <qiang.zhang@windriver.com>
> > 
> > The flush_all_cpus_locked() should be called with cpus_read_lock/unlock(),
> > ensure flush_cpu_slab() can be executed on schedule_on CPU.
> > 
> > Fixes: 1c84f3c91640 ("mm, slub: fix memory and cpu hotplug related lock ordering issues")  
> 
> Which branch contains this commit? At least not linux.git or linux-next

It is Andrew's mmotm which is included in linux-next but gets rebased
often, so, for example, that SHA1 is no longer valid in linux-next
today (is is now fd917c6407fb).

This (unfortunately) make Fixes tags less useful for mmotm :-(
-- 
Cheers,
Stephen Rothwell

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

Re: [PATCH] mm, slub: add cpus_read_lock/unlock() for slab_mem_going_offline_callback()

[David Hildenbrand]

On 16.08.21 10:17, Stephen Rothwell wrote:
> Hi David,
> 
> On Mon, 16 Aug 2021 10:04:13 +0200 David Hildenbrand <david@redhat.com> wrote:
>>
>> On 16.08.21 09:46, qiang.zhang@windriver.com wrote:
>>> From: "Qiang.Zhang" <qiang.zhang@windriver.com>
>>>
>>> The flush_all_cpus_locked() should be called with cpus_read_lock/unlock(),
>>> ensure flush_cpu_slab() can be executed on schedule_on CPU.
>>>
>>> Fixes: 1c84f3c91640 ("mm, slub: fix memory and cpu hotplug related lock ordering issues")
>>
>> Which branch contains this commit? At least not linux.git or linux-next
> 
> It is Andrew's mmotm which is included in linux-next but gets rebased

Maybe I am blind or need more coffee:

https://www.ozlabs.org/~akpm/mmotm/series

> often, so, for example, that SHA1 is no longer valid in linux-next
> today (is is now fd917c6407fb).
> 
> This (unfortunately) make Fixes tags less useful for mmotm :-(

Right, and it somewhat makes sense, because Andrew will actually squash 
patches before sending them further upstream.

Ideally, such fixes should be discussed in the respective patch series, 
because before they go upstream, they are still under development.


-- 
Thanks,

David / dhildenb

Re: [PATCH] mm, slub: add cpus_read_lock/unlock() for slab_mem_going_offline_callback()

[Vlastimil Babka]

On 8/16/21 10:04 AM, David Hildenbrand wrote:
> On 16.08.21 09:46, qiang.zhang@windriver.com wrote:
>> From: "Qiang.Zhang" <qiang.zhang@windriver.com>
>>
>> The flush_all_cpus_locked() should be called with cpus_read_lock/unlock(),
>> ensure flush_cpu_slab() can be executed on schedule_on CPU.
>>
>> Fixes: 1c84f3c91640 ("mm, slub: fix memory and cpu hotplug related lock ordering issues")
> Memory notifiers are getting called from online_pages()/offline_pages(), 
> where we call memory_notify(MEM_GOING_OFFLINE, &arg) under 
> mem_hotplug_begin().
> 
> mem_hotplug_begin() does a cpus_read_lock().

Exactly. Also flush_all_cpus_locked() has a lockdep assert for
cpus_read_lock() which doesn't trigger in testing.

> How does this even work or against which branch is this?
>