slub: Fix sysfs circular locking dependency

mm/slub.c +0 -0

index bec0e35..9831004 100644
@@ -2516,7 +2516,13 @@ void kmem_cache_destroy(struct kmem_cache *s)
 		}
 		if (s->flags & SLAB_DESTROY_BY_RCU)
 			rcu_barrier();

slub_lock protects slab_state following kmem_cache_init() if caches are created/destroyed prior to the sysfs slab initcall setting the global variable, so couldn't this leak the kobject if its initialization and add was deferred on kmem_cache_create() and added by slab_sysfs_init()?

How about this solution? In the common case of kernels without hotplug support there will be no locking at all.

Subject: slub: Avoid use of slub_lock in show_slab_objects()

The purpose of the locking is to prevent removal and additions of nodes when statistics are gathered for a slab cache. So we need to avoid racing with memory hotplug functionality.

It is enough to take the memory hotplug locks there instead of the slub_lock.

Signed-off-by Christoph Lameter <[email protected]>

---
 mm/slub.c |    4 ++--

 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/mm/slub.c
===================================================================

mm/slub.c 2011-01-05 09:56:27.000000000 -0600 renamed from mm/slub.c    2011-01-05 09:55:34.000000000 -0600 +1 -1

@@ -3821,7 +3821,7 @@ static ssize_t show_slab_objects(struct
                }
        }
 
-       down_read(&slub_lock);
+       lock_memory_hotplug();
  #ifdef CONFIG_SLUB_DEBUG
        if (flags & SO_ALL) {
                for_each_node_state(node, N_NORMAL_MEMORY) {

Quoted 28 lines · Christoph Lameter

How about this solution? In the common case of kernels without hotplug support there will be no locking at all.

Subject: slub: Avoid use of slub_lock in show_slab_objects()

The purpose of the locking is to prevent removal and additions of nodes when statistics are gathered for a slab cache. So we need to avoid racing with memory hotplug functionality.

It is enough to take the memory hotplug locks there instead of the slub_lock.

Signed-off-by Christoph Lameter <[email protected]>

---
 mm/slub.c |    4 ++--

 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/mm/slub.c
===================================================================

mm/slub.c 2011-01-05 09:56:27.000000000 -0600 renamed from mm/slub.c    2011-01-05 09:55:34.000000000 -0600 +0 -0

@@ -3821,7 +3821,7 @@ static ssize_t show_slab_objects(struct
                }
        }
 

Quoted 6 lines

mm/slub.c 2011-01-05 09:56:27.000000000 -0600 renamed from mm/slub.c    2011-01-05 09:55:34.000000000 -0600 +1 -1

-       down_read(&slub_lock);
+       lock_memory_hotplug();
  #ifdef CONFIG_SLUB_DEBUG
        if (flags & SO_ALL) {
                for_each_node_state(node, N_NORMAL_MEMORY) {

mm/slub.c 2011-01-05 09:56:27.000000000 -0600 static ssize_t show_slab_objects(struct · line 3868

Quoted 8 lines

mm/slub.c 2011-01-05 09:56:27.000000000 -0600 renamed from mm/slub.c    2011-01-05 09:55:34.000000000 -0600 +1 -1

@@ -3862,7 +3862,7 @@ static ssize_t show_slab_objects(struct
                        x += sprintf(buf + x, " N%d=%lu",
                                        node, nodes[node]);
  #endif
-       up_read(&slub_lock);
+       unlock_memory_hotplug();
        kfree(nodes);
        return x + sprintf(buf + x, "\n");
  }

Makes sense. Bart, does this fix the problem for you?

Quoted 30 lines · Pekka Enberg

Quoted 28 lines · Christoph Lameter

How about this solution? In the common case of kernels without hotplug support there will be no locking at all.

Subject: slub: Avoid use of slub_lock in show_slab_objects()

The purpose of the locking is to prevent removal and additions of nodes when statistics are gathered for a slab cache. So we need to avoid racing with memory hotplug functionality.

It is enough to take the memory hotplug locks there instead of the slub_lock.

Signed-off-by Christoph Lameter <[email protected]>

---
 mm/slub.c |    4 ++--

 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/mm/slub.c
===================================================================

mm/slub.c 2011-01-05 09:56:27.000000000 -0600 renamed from mm/slub.c    2011-01-05 09:55:34.000000000 -0600 +0 -0

@@ -3821,7 +3821,7 @@ static ssize_t show_slab_objects(struct
                }
        }
 

Quoted 16 lines

Quoted 6 lines

mm/slub.c 2011-01-05 09:56:27.000000000 -0600 renamed from mm/slub.c    2011-01-05 09:55:34.000000000 -0600 +1 -1

-       down_read(&slub_lock);
+       lock_memory_hotplug();
  #ifdef CONFIG_SLUB_DEBUG
        if (flags & SO_ALL) {
                for_each_node_state(node, N_NORMAL_MEMORY) {

mm/slub.c 2011-01-05 09:56:27.000000000 -0600 static ssize_t show_slab_objects(struct · line 3868

Quoted 8 lines

mm/slub.c 2011-01-05 09:56:27.000000000 -0600 renamed from mm/slub.c    2011-01-05 09:55:34.000000000 -0600 +1 -1

@@ -3862,7 +3862,7 @@ static ssize_t show_slab_objects(struct
                        x += sprintf(buf + x, " N%d=%lu",
                                        node, nodes[node]);
  #endif
-       up_read(&slub_lock);
+       unlock_memory_hotplug();
        kfree(nodes);
        return x + sprintf(buf + x, "\n");
  }

Makes sense. Bart, does this fix the problem for you?

The action sequence that had triggered this sequence (invoke kmem_cache_create(); read all files in /sys/kernel/slab; invoke kmem_cache_destroy()) does now pass without triggering lock inversion complaints.

Quoted 41 lines

Quoted 40 lines

Quoted 30 lines

Subject: slub: Avoid use of slub_lock in show_slab_objects()

The purpose of the locking is to prevent removal and additions of nodes when statistics are gathered for a slab cache. So we need to avoid racing with memory hotplug functionality.

It is enough to take the memory hotplug locks there instead of the slub_lock.

Signed-off-by Christoph Lameter <[email protected]>

---
 mm/slub.c |    4 ++--

 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/mm/slub.c
===================================================================

mm/slub.c 2011-01-05 09:56:27.000000000 -0600 renamed from mm/slub.c    2011-01-05 09:55:34.000000000 -0600 +1 -1

@@ -3821,7 +3821,7 @@ static ssize_t show_slab_objects(struct
                }
        }
 
-       down_read(&slub_lock);
+       lock_memory_hotplug();
  #ifdef CONFIG_SLUB_DEBUG
        if (flags & SO_ALL) {
                for_each_node_state(node, N_NORMAL_MEMORY) {

mm/slub.c 2011-01-05 09:56:27.000000000 -0600 static ssize_t show_slab_objects(struct · line 3868

Quoted 8 lines

mm/slub.c 2011-01-05 09:56:27.000000000 -0600 renamed from mm/slub.c    2011-01-05 09:55:34.000000000 -0600 +1 -1

@@ -3862,7 +3862,7 @@ static ssize_t show_slab_objects(struct
                        x += sprintf(buf + x, " N%d=%lu",
                                        node, nodes[node]);
  #endif
-       up_read(&slub_lock);
+       unlock_memory_hotplug();
        kfree(nodes);
        return x + sprintf(buf + x, "\n");
  }

Makes sense. Bart, does this fix the problem for you?

Quoted 4 lines

The action sequence that had triggered this sequence (invoke kmem_cache_create(); read all files in /sys/kernel/slab; invoke kmem_cache_destroy()) does now pass without triggering lock inversion complaints.

Thanks for testing. David, does Christoph's patch look OK to you?

Quoted 2 lines

Thanks for testing. David, does Christoph's patch look OK to you?

I think it certainly fixes the problem at hand, but I think we also need to do lock_memory_hotplug() for memory hotplug in slab_mem_going_online_callback() to make show_slab_objects() consistent when being printed during concurrent node hot-add since it sets bits in N_NORMAL_MEMORY. The MEM_OFFLINE callback is already handled at a higher level by taking the lock in the hotplug layer, but we need to protect the MEM_GOING_ONLINE and MEM_CANCEL_ONLINE callbacks if slub_lock is no longer used to protect node arrays (which was admittedly always convenient since it's typically associated with an iteration through slab_caches).

slab_mem_going_online_callback() etc already take the slub_lock since they have to iterate over the list of slab caches in existence. We could take the hotplug lock there as well.

Kame-san: Can you enlighten us on hotplug locking? And also check this patch?

Quoted 12 lines · David Rientjes

Quoted 2 lines

Thanks for testing. David, does Christoph's patch look OK to you?

I think it certainly fixes the problem at hand, but I think we also need to do lock_memory_hotplug() for memory hotplug in slab_mem_going_online_callback() to make show_slab_objects() consistent when being printed during concurrent node hot-add since it sets bits in N_NORMAL_MEMORY. The MEM_OFFLINE callback is already handled at a higher level by taking the lock in the hotplug layer, but we need to protect the MEM_GOING_ONLINE and MEM_CANCEL_ONLINE callbacks if slub_lock is no longer used to protect node arrays (which was admittedly always convenient since it's typically associated with an iteration through slab_caches).

Hmm, I would have expected the callbacks all to be done under hotplug locking.

The MEM_GOING_ONLINE callback is not that critical since a node that is coming online presumably has only a minimal set of objects necessary for potential future allocations.

slab_mem_going_online_callback() etc already take the slub_lock since they have to iterate over the list of slab caches in existence. We could take the hotplug lock there as well.

Kame-san: Can you enlighten us on hotplug locking? And also check this patch?

But, nice catch. I think MEM_GOING_ONLINE, MEM_ONLINE should be done under lock_memory_hotplug. So, could you update your patch and modify online_pages() ?

IIUC, online_pages() is an user interface function and there will be no downside to insert lock there. online_pages() should be serialized.

Subject: slub: Avoid use of slub_lock in show_slab_objects()

The purpose of the locking is to prevent removal and additions of nodes when statistics are gathered for a slab cache. So we need to avoid racing with memory hotplug functionality.

It is enough to take the memory hotplug locks there instead of the slub_lock.

online_pages() does not acquire the memory_hotplug lock. So add the missing locking there.

Signed-off-by Christoph Lameter <[email protected]>

Quoted 9 lines · Christoph Lameter

Subject: slub: Avoid use of slub_lock in show_slab_objects()

The purpose of the locking is to prevent removal and additions of nodes when statistics are gathered for a slab cache. So we need to avoid racing with memory hotplug functionality.

It is enough to take the memory hotplug locks there instead of the slub_lock.

Because memory hotplug is the only time s->node[] is modified after the sysfs files are created, which is the only time show_slab_objects() is called.

Quoted 4 lines

online_pages() does not acquire the memory_hotplug lock. So add the missing locking there.

Signed-off-by Christoph Lameter <[email protected]>

Acked-by David Rientjes <[email protected]>

 [ Should probably be seperated out into two patches, one for the memory hotplug locking addition and one for the slub fix, both should be pushed during this merge window.  If so, a comment describing the new semantics of lock_memory_hotplug() to protect data structures that may be modified in hotplug notifier callbacks would be appreciated. ]

Quoted 24 lines · David Rientjes

Quoted 9 lines · Christoph Lameter

Subject: slub: Avoid use of slub_lock in show_slab_objects()

The purpose of the locking is to prevent removal and additions of nodes when statistics are gathered for a slab cache. So we need to avoid racing with memory hotplug functionality.

It is enough to take the memory hotplug locks there instead of the slub_lock.

Because memory hotplug is the only time s->node[] is modified after the sysfs files are created, which is the only time show_slab_objects() is called.

Quoted 4 lines

online_pages() does not acquire the memory_hotplug lock. So add the missing locking there.

Signed-off-by Christoph Lameter <[email protected]>

Acked-by David Rientjes <[email protected]>

Quoted 6 lines

[ Should probably be seperated out into two patches, one for the
memory hotplug locking addition and one for the slub fix, both should be pushed during this merge window. If so, a comment describing the new semantics of lock_memory_hotplug() to protect data structures that may be modified in hotplug notifier callbacks would be appreciated. ]

Shouldn't [email protected] be CC-ed too ?

New patch that just covers the slub changes.

Subject: slub: Avoid use of slub_lock in show_slab_objects()

The purpose of the locking is to prevent removal and additions of nodes when statistics are gathered for a slab cache. So we need to avoid racing with memory hotplug functionality.

It is enough to take the memory hotplug locks there instead of the slub_lock.

online_pages() currently does not acquire the memory_hotplug lock. Another patch will be submitted by the memory hotplug authors to take the memory hotplug lock and describe the uses of the memory hotplug lock to protect against adding and removal of nodes from non hotplug data structures.

Signed-off-by Christoph Lameter <[email protected]>

Quoted 11 lines · Christoph Lameter

New patch that just covers the slub changes.

Subject: slub: Avoid use of slub_lock in show_slab_objects()

The purpose of the locking is to prevent removal and additions of nodes when statistics are gathered for a slab cache. So we need to avoid racing with memory hotplug functionality.

It is enough to take the memory hotplug locks there instead of the slub_lock.

Quoted 7 lines

online_pages() currently does not acquire the memory_hotplug lock. Another patch will be submitted by the memory hotplug authors to take the memory hotplug lock and describe the uses of the memory hotplug lock to protect against adding and removal of nodes from non hotplug data structures.

Signed-off-by Christoph Lameter<[email protected]>

Acked-by David Rientjes<[email protected]>

Quoted 19 lines · David Rientjes

Quoted 17 lines · Christoph Lameter

New patch that just covers the slub changes.

Subject: slub: Avoid use of slub_lock in show_slab_objects()

The purpose of the locking is to prevent removal and additions of nodes when statistics are gathered for a slab cache. So we need to avoid racing with memory hotplug functionality.

It is enough to take the memory hotplug locks there instead of the slub_lock.

online_pages() currently does not acquire the memory_hotplug lock. Another patch will be submitted by the memory hotplug authors to take the memory hotplug lock and describe the uses of the memory hotplug lock to protect against adding and removal of nodes from non hotplug data structures.

Quoted 2 lines

Quoted 1 line

Signed-off-by Christoph Lameter<[email protected]>

Acked-by David Rientjes<[email protected]>

Is this safe to be applied without the other hotplug parts?

From: KAMEZAWA Hiroyuki <[email protected]>

Now, memory_hotplug_(un)lock() is used for add/remove/offline pages for avoiding races with hibernation. But this should be held in online_pages(), too. It seems asymmetric.

There are cases where one has to avoid a race with memory hotplug notifier and his own local code, and hotplug v.s. hotplug. This will add a generic solution for avoiding races. In other view, having lock here has no big impacts. online pages is tend to be done by udev script at el against each memory section one by one.

Then, it's better to have lock here, too.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 include/linux/memory_hotplug.h |    6 ++++++
 mm/memory_hotplug.c            |    4 ++++
 2 files changed, 10 insertions(+)

Index: linux-2.6.37.org/include/linux/memory_hotplug.h
===================================================================

include/linux/memory_hotplug.h +6 -0

@@ -161,6 +161,12 @@ extern void register_page_bootmem_info_n
 extern void put_page_bootmem(struct page *page);
 #endif
 
+/*
+ * Lock for memory hotplug guarantees 1) all callbacks for memory hotplug
+ * notifier will be called under this. 2) offline/online/add/remove memory
+ * will not run simultaneously.
+ */
+
 void lock_memory_hotplug(void);
 void unlock_memory_hotplug(void);
 

Index: linux-2.6.37.org/mm/memory_hotplug.c
===================================================================

mm/memory_hotplug.c +4 -0

@@ -407,6 +407,7 @@ int online_pages(unsigned long pfn, unsi
 	int ret;
 	struct memory_notify arg;
 
+	lock_memory_hotplug();
 	arg.start_pfn = pfn;
 	arg.nr_pages = nr_pages;
 	arg.status_change_nid = -1;

@@ -419,6 +420,7 @@ int online_pages(unsigned long pfn, unsi
 	ret = notifier_to_errno(ret);
 	if (ret) {
 		memory_notify(MEM_CANCEL_ONLINE, &arg);
+		unlock_memory_hotplug();
 		return ret;
 	}
 	/*

@@ -443,6 +445,7 @@ int online_pages(unsigned long pfn, unsi
 		printk(KERN_DEBUG "online_pages %lx at %lx failed\n",
 			nr_pages, pfn);
 		memory_notify(MEM_CANCEL_ONLINE, &arg);
+		unlock_memory_hotplug();
 		return ret;
 	}
 

@@ -467,6 +470,7 @@ int online_pages(unsigned long pfn, unsi
 
 	if (onlined_pages)
 		memory_notify(MEM_ONLINE, &arg);
+	unlock_memory_hotplug();
 
 	return 0;
 }

Quoted 17 lines

Quoted 17 lines

New patch that just covers the slub changes.

Subject: slub: Avoid use of slub_lock in show_slab_objects()

The purpose of the locking is to prevent removal and additions of nodes when statistics are gathered for a slab cache. So we need to avoid racing with memory hotplug functionality.

It is enough to take the memory hotplug locks there instead of the slub_lock.

online_pages() currently does not acquire the memory_hotplug lock. Another patch will be submitted by the memory hotplug authors to take the memory hotplug lock and describe the uses of the memory hotplug lock to protect against adding and removal of nodes from non hotplug data structures.

Quoted 2 lines

Quoted 1 line

Signed-off-by Christoph Lameter<[email protected]>

Acked-by David Rientjes<[email protected]>

Is this safe to be applied without the other hotplug parts?

Quoted 17 lines · Pekka Enberg

Quoted 17 lines

Quoted 17 lines

New patch that just covers the slub changes.

Subject: slub: Avoid use of slub_lock in show_slab_objects()

The purpose of the locking is to prevent removal and additions of nodes when statistics are gathered for a slab cache. So we need to avoid racing with memory hotplug functionality.

It is enough to take the memory hotplug locks there instead of the slub_lock.

online_pages() currently does not acquire the memory_hotplug lock. Another patch will be submitted by the memory hotplug authors to take the memory hotplug lock and describe the uses of the memory hotplug lock to protect against adding and removal of nodes from non hotplug data structures.

Quoted 3 lines

Quoted 2 lines

Quoted 1 line

Signed-off-by Christoph Lameter<[email protected]>

Acked-by David Rientjes<[email protected]>

Is this safe to be applied without the other hotplug parts?

It's safe, but not protecting anything since it can race with memory hot-add and cause inconsistent information to be displayed (since we iterate over N_NORMAL_MEMORY several times in show_slab_objects() and the memory hotplug code modifies it). I'm hoping Andrew can push Kame's patch to add lock_memory_hotplug() to online_pages() either during the merge window or during -rc1 (it has good justification -- it can race with memory hot-remove) and this can also be pushed during the rc series (to fix the lockdep warning).

 [ Should probably be seperated out into two patches, one for the memory hotplug locking addition and one for the slub fix, both should be pushed during this merge window.  If so, a comment describing the new semantics of lock_memory_hotplug() to protect data structures that may be modified in hotplug notifier callbacks would be appreciated. ]

Quoted 6 lines · David Rientjes

 [ Should probably be seperated out into two patches, one for the memory hotplug locking addition and one for the slub fix, both should be pushed during this merge window.  If so, a comment describing the new semantics of lock_memory_hotplug() to protect data structures that may be modified in hotplug notifier callbacks would be appreciated. ]

Agreed.

Quoted 8 lines · Pekka Enberg

Quoted 6 lines · David Rientjes

 [ Should probably be seperated out into two patches, one for the memory hotplug locking addition and one for the slub fix, both should be pushed during this merge window.  If so, a comment describing the new semantics of lock_memory_hotplug() to protect data structures that may be modified in hotplug notifier callbacks would be appreciated. ]

Quoted 1 line

Agreed.

Kame-san: Could you do the hotplug piece and the description of the use of the memory hotplug locks to protect extensions of data structures for new nodes?