mm/kmemleak: Avoid soft lockup in kmemleak_scan()
Subsystems (2)
cover mm/kmemleak: Avoid soft lockup in kmemleak_scan()
There are 3 RCU-based object iteration loops in kmemleak_scan(). Because of the need to take RCU read lock, we can't insert cond_resched() into the loop like other parts of the function. As there can be millions of objects to be scanned, it takes a while to iterate all of them. The kmemleak functionality is usually enabled in a debug kernel which is much slower than a non-debug kernel. With sufficient number of kmemleak objects, the time to iterate them all may exceed 22s causing soft lockup.
watchdog: BUG: soft lockup - CPU#3 stuck for 22s! [kmemleak:625]
This patch series make changes to the 3 object iteration loops in kmemleak_scan() to prevent them from causing soft lockup.
Waiman Long (3):
mm/kmemleak: Use _irq lock/unlock variants in kmemleak_scan/_clear() mm/kmemleak: Skip unlikely objects in kmemleak_scan() without taking
lock
mm/kmemleak: Prevent soft lockup in first object iteration loop of
kmemleak_scan()
mm/kmemleak.c | 52 ++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 11 deletions(-)
Signature · 1 line
2.31.1
PATCH 1/3 mm/kmemleak: Use _irq lock/unlock variants in kmemleak_scan/_clear()
The kmemleak_scan() function is called only from the kmemleak scan thread or from write to the kmemleak debugfs file. Both are in task context and so we can directly use the simpler _irq() lock/unlock calls instead of the more complex _irqsave/_irqrestore variants.
Similarly, kmemleak_clear() is called only from write to the kmemleak debugfs file. The same change can be applied.
--- mm/kmemleak.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)
mm/kmemleak.c +8 -10
@@ -1413,7 +1413,6 @@ static void scan_gray_list(void)*/ static void kmemleak_scan (void ) { -unsigned long flags;struct kmemleak_object *object;struct zone *zone;int __maybe_unused i;
@@ -1424,7 +1423,7 @@ static void kmemleak_scan(void)/* prepare the kmemleak_object's */ rcu_read_lock ();list_for_each_entry_rcu (object, &object_list, object_list) { -raw_spin_lock_irqsave (&object->lock, flags); +raw_spin_lock_irq (&object->lock);#ifdef DEBUG /* * With a few exceptions there should be a maximum of
@@ -1441,7 +1440,7 @@ static void kmemleak_scan(void)if (color_gray (object) &&get_object (object))list_add_tail (&object->gray_list, &gray_list); -raw_spin_unlock_irqrestore (&object->lock, flags); +raw_spin_unlock_irq (&object->lock); }rcu_read_unlock ();
@@ -1509,14 +1508,14 @@ static void kmemleak_scan(void)*/ rcu_read_lock ();list_for_each_entry_rcu (object, &object_list, object_list) { -raw_spin_lock_irqsave (&object->lock, flags); +raw_spin_lock_irq (&object->lock);if (color_white (object) && (object->flags &OBJECT_ALLOCATED ) &&update_checksum (object) &&get_object (object)) {/* color it gray temporarily */ object->count = object->min_count;list_add_tail (&object->gray_list, &gray_list); } -raw_spin_unlock_irqrestore (&object->lock, flags); +raw_spin_unlock_irq (&object->lock); }rcu_read_unlock ();
@@ -1536,7 +1535,7 @@ static void kmemleak_scan(void)*/ rcu_read_lock ();list_for_each_entry_rcu (object, &object_list, object_list) { -raw_spin_lock_irqsave (&object->lock, flags); +raw_spin_lock_irq (&object->lock);if (unreferenced_object (object) && !(object->flags &OBJECT_REPORTED )) { object->flags |=OBJECT_REPORTED ;
@@ -1546,7 +1545,7 @@ static void kmemleak_scan(void) new_leaks++; } -raw_spin_unlock_irqrestore (&object->lock, flags); +raw_spin_unlock_irq (&object->lock); }rcu_read_unlock ();
@@ -1748,15 +1747,14 @@ static int dump_str_object_info(const char *str)static void kmemleak_clear (void ) {struct kmemleak_object *object; -unsigned long flags;rcu_read_lock ();list_for_each_entry_rcu (object, &object_list, object_list) { -raw_spin_lock_irqsave (&object->lock, flags); +raw_spin_lock_irq (&object->lock);if ((object->flags &OBJECT_REPORTED ) &&unreferenced_object (object))__paint_it (object,KMEMLEAK_GREY ); -raw_spin_unlock_irqrestore (&object->lock, flags); +raw_spin_unlock_irq (&object->lock); }rcu_read_unlock ();
Signature · 1 line
2.31.1
Re: [PATCH 1/3] mm/kmemleak: Use _irq lock/unlock variants in kmemleak_scan/_clear()
Quoted 9 lines · Waiman Long
The
kmemleak_scan()function is called only from the kmemleak scan thread or from write to the kmemleak debugfs file. Both are in task context and so we can directly use the simpler_irq()lock/unlock calls instead of the more complex _irqsave/_irqrestore variants.Similarly,
kmemleak_clear()is called only from write to the kmemleak debugfs file. The same change can be applied.Signed-off-by Waiman Long <[email protected]>
Thanks.
Re: [PATCH 1/3] mm/kmemleak: Use _irq lock/unlock variants in kmemleak_scan/_clear()
In reply to Waiman LongQuoted 9 lines · Waiman Long
The
kmemleak_scan()function is called only from the kmemleak scan thread or from write to the kmemleak debugfs file. Both are in task context and so we can directly use the simpler_irq()lock/unlock calls instead of the more complex _irqsave/_irqrestore variants.Similarly,
kmemleak_clear()is called only from write to the kmemleak debugfs file. The same change can be applied.Signed-off-by Waiman Long <[email protected]>
PATCH 2/3 mm/kmemleak: Skip unlikely objects in kmemleak_scan() without taking lock
In reply to Waiman LongThere are 3 RCU-based object iteration loops in kmemleak_scan(). Because of the need to take RCU read lock, we can't insert cond_resched() into the loop like other parts of the function. As there can be millions of objects to be scanned, it takes a while to iterate all of them. The kmemleak functionality is usually enabled in a debug kernel which is much slower than a non-debug kernel. With sufficient number of kmemleak objects, the time to iterate them all may exceed 22s causing soft lockup.
watchdog: BUG: soft lockup - CPU#3 stuck for 22s! [kmemleak:625]
In this particular bug report, the soft lockup happen in the 2nd iteration loop.
In the 2nd and 3rd loops, most of the objects are checked and then skipped under the object lock. Only a selected fews are modified. Those objects certainly need lock protection. However, the lock/unlock operation is slow especially with interrupt disabling and enabling included.
We can actually do some basic check like color_white() without taking the lock and skip the object accordingly. Of course, this kind of check is racy and may miss objects that are being modified concurrently. The cost of missed objects, however, is just that they will be discovered in the next scan instead. The advantage of doing so is that iteration can be done much faster especially with LOCKDEP enabled in a debug kernel.
With a debug kernel running on a 2-socket 96-thread x86-64 system (HZ=1000), the 2nd and 3rd iteration loops speedup with this patch on the first kmemleak_scan() call after bootup is shown in the table below.
Before patch After patch
Loop # # of objects Elapsed time # of objects Elapsed time
------ ------------ ------------ ------------ ------------
2 2,599,850 2.392s 2,596,364 0.266s
3 2,600,176 2.171s 2,597,061 0.260sThis patch reduces loop iteration times by about 88%. This will greatly reduce the chance of a soft lockup happening in the 2nd or 3rd iteration loops.
--- mm/kmemleak.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
mm/kmemleak.c +14 -0
@@ -1508,6 +1508,13 @@ static void kmemleak_scan(void)*/ rcu_read_lock ();list_for_each_entry_rcu (object, &object_list, object_list) { +/* +* This is racy but we can save the overhead of lock/unlock +* calls. The missed objects, if any, should be caught in +* the next scan. +*/ +if (!color_white (object)) +continue ;raw_spin_lock_irq (&object->lock);if (color_white (object) && (object->flags &OBJECT_ALLOCATED ) &&update_checksum (object) &&get_object (object)) {
@@ -1535,6 +1542,13 @@ static void kmemleak_scan(void)*/ rcu_read_lock ();list_for_each_entry_rcu (object, &object_list, object_list) { +/* +* This is racy but we can save the overhead of lock/unlock +* calls. The missed objects, if any, should be caught in +* the next scan. +*/ +if (!color_white (object)) +continue ;raw_spin_lock_irq (&object->lock);if (unreferenced_object (object) && !(object->flags &OBJECT_REPORTED )) {
Signature · 1 line
2.31.1
Re: [PATCH 2/3] mm/kmemleak: Skip unlikely objects in kmemleak_scan() without taking lock
Quoted 5 lines · Waiman Long
With a debug kernel running on a 2-socket 96-thread x86-64 system (HZ=1000), the 2nd and 3rd iteration loops speedup with this patch on the first
kmemleak_scan()call after bootup is shown in the table below.Before patch After patch
Quoted 8 lines
Loop # # of objects Elapsed time # of objects Elapsed time ------ ------------ ------------ ------------ ------------ 2 2,599,850 2.392s 2,596,364 0.266s 3 2,600,176 2.171s 2,597,061 0.260sThis patch reduces loop iteration times by about 88%. This will greatly reduce the chance of a soft lockup happening in the 2nd or 3rd iteration loops.
Nice numbers, thanks for digging into this.
But I'm slightly surprised that the first loop doesn't cause any problems.
Quoted 8 lines
mm/kmemleak.c +0
-0@@ -1508,6 +1508,13 @@ static void kmemleak_scan(void)*/ rcu_read_lock ();list_for_each_entry_rcu (object, &object_list, object_list) {
mm/kmemleak.c static void kmemleak_scan(void) · line 1520
Quoted 10 lines
mm/kmemleak.c +7
-0+/* +* This is racy but we can save the overhead of lock/unlock +* calls. The missed objects, if any, should be caught in +* the next scan. +*/ +if (!color_white (object)) +continue ;raw_spin_lock_irq (&object->lock);if (color_white (object) && (object->flags &OBJECT_ALLOCATED ) &&update_checksum (object) &&get_object (object)) {
It's not actually scanning (like tree look-ups) but only updating the checksum of the potentially orphan objects. If the problem is caused by object->lock, we should have seen it with the first loop as well.
It is possible that some large list is occasionally missed if there are concurrent updates and a significant number of objects turn up "white", forcing the checksum update. Otherwise this shouldn't be much different from the first loop if there are no massive (false) leaks.
I think the race on color_white() can only be with a kmemleak_ignore() or kmemleak_not_leak() call, otherwise the object colour shouldn't be changed. So such objects can only turn from white to gray or black, so the race I think is safe.
Quoted 4 lines
mm/kmemleak.c +0
-0@@ -1535,6 +1542,13 @@ static void kmemleak_scan(void)*/ rcu_read_lock ();list_for_each_entry_rcu (object, &object_list, object_list) {
mm/kmemleak.c static void kmemleak_scan(void) · line 1554
Quoted 10 lines
mm/kmemleak.c +7
-0+/* +* This is racy but we can save the overhead of lock/unlock +* calls. The missed objects, if any, should be caught in +* the next scan. +*/ +if (!color_white (object)) +continue ;raw_spin_lock_irq (&object->lock);if (unreferenced_object (object) && !(object->flags &OBJECT_REPORTED )) {
Same here.
I did wonder whether it's worth keeping object->lock around, I even have a stashed patch lying around from 2019. Instead we'd have the big kmemleak_lock held for longer, though released periodically during scanning. We can then move the lock outside the loop and traversal would be faster but with an increased latency on slab allocation/freeing on other CPUs. Right now we take the kmemleak_lock when scanning a single block (e.g. object) to protect the rb-tree and rely on object->lock to ensure the object isn't freed. Other concurrent allocs/frees would only be blocked during single object scanning.
Anyway, I'm not entirely sure it's the lock causing the issue as we don't see it with the first loop. I'm more inclined to think it's the checksum and the skipping if !color_white() would do the trick.
Unless there's a better idea:
Re: [PATCH 2/3] mm/kmemleak: Skip unlikely objects in kmemleak_scan() without taking lock
Quoted 11 lines · Catalin Marinas
Quoted 10 lines · Waiman Long
With a debug kernel running on a 2-socket 96-thread x86-64 system (HZ=1000), the 2nd and 3rd iteration loops speedup with this patch on the first
kmemleak_scan()call after bootup is shown in the table below.Before patch After patch
Loop # # of objects Elapsed time # of objects Elapsed time ------ ------------ ------------ ------------ ------------ 2 2,599,850 2.392s 2,596,364 0.266s 3 2,600,176 2.171s 2,597,061 0.260s
Quoted 7 lines
Quoted 3 lines
This patch reduces loop iteration times by about 88%. This will greatly reduce the chance of a soft lockup happening in the 2nd or 3rd iteration loops.
Nice numbers, thanks for digging into this.
But I'm slightly surprised that the first loop doesn't cause any problems.
The first loop is still problematic. It is just a bit faster with the same number of objects. The corresponding elapsed time is about 1.7s. The heuristics used in this patch cannot be applied to the first loop. See patch 3 on how to avoid soft lockup in the first loop.
Quoted 14 lines
Quoted 14 lines
mm/kmemleak.c +5
-0@@ -1508,6 +1508,13 @@ static void kmemleak_scan(void)*/ rcu_read_lock ();list_for_each_entry_rcu (object, &object_list, object_list) { +/* +* This is racy but we can save the overhead of lock/unlock +* calls. The missed objects, if any, should be caught in +* the next scan. +*/
Quoted 8 lines
mm/kmemleak.c static void kmemleak_scan(void) · line 1520
Quoted 5 lines
mm/kmemleak.c +2
-0+if (!color_white (object)) +continue ;raw_spin_lock_irq (&object->lock);if (color_white (object) && (object->flags &OBJECT_ALLOCATED ) &&update_checksum (object) &&get_object (object)) {It's not actually scanning (like tree look-ups) but only updating the checksum of the potentially orphan objects. If the problem is caused by
object->lock, we should have seen it with the first loop as well.
See above. Maybe I should clarify in the patch description that similar change cannot be applied to the first loop.
Quoted 37 lines
It is possible that some large list is occasionally missed if there are concurrent updates and a significant number of objects turn up "white", forcing the checksum update. Otherwise this shouldn't be much different from the first loop if there are no massive (false) leaks.
I think the race on
color_white()can only be with akmemleak_ignore()orkmemleak_not_leak()call, otherwise the object colour shouldn't be changed. So such objects can only turn from white to gray or black, so the race I think is safe.Quoted 4 lines
mm/kmemleak.c +0
-0@@ -1535,6 +1542,13 @@ static void kmemleak_scan(void)*/ rcu_read_lock ();list_for_each_entry_rcu (object, &object_list, object_list) {mm/kmemleak.c static void kmemleak_scan(void) · line 1554
Quoted 10 lines
mm/kmemleak.c +7
-0+/* +* This is racy but we can save the overhead of lock/unlock +* calls. The missed objects, if any, should be caught in +* the next scan. +*/ +if (!color_white (object)) +continue ;raw_spin_lock_irq (&object->lock);if (unreferenced_object (object) && !(object->flags &OBJECT_REPORTED )) {Same here.
I did wonder whether it's worth keeping
object->lockaround, I even have a stashed patch lying around from 2019. Instead we'd have the bigkmemleak_lockheld for longer, though released periodically during scanning. We can then move the lock outside the loop and traversal would be faster but with an increased latency on slab allocation/freeing on other CPUs. Right now we take thekmemleak_lockwhen scanning a single block (e.g. object) to protect the rb-tree and rely onobject->lockto ensure the object isn't freed. Other concurrent allocs/frees would only be blocked during single object scanning.
Quoted 7 lines
Anyway, I'm not entirely sure it's the lock causing the issue as we don't see it with the first loop. I'm more inclined to think it's the checksum and the skipping if !color_white() would do the trick.
Unless there's a better idea:
Reviewed-by Catalin Marinas <[email protected]>
The lock is a problem because of lockdep. Once I disable lockdep, the elapsed time can drop to about 0.7s. However, lockdep is normally enabled in a debug kernel. I will try to investigate if there is a way to optimize lockdep or such repeated lock/unlock loop.
Thanks,
Longman
Quoted 1 line
PATCH 3/3 mm/kmemleak: Prevent soft lockup in first object iteration loop of kmemleak_scan()
In reply to Waiman LongThe first RCU-based object iteration loop has to put almost all the objects into the gray list and so cannot skip taking the object lock.
One way to avoid soft lockup is to insert occasional cond_resched() into the loop. This cannot be done while holding the RCU read lock which is to protect objects from removal. However, putting an object into the gray list means getting a reference to the object. That will prevent the object from removal as well without the need to hold the RCU read lock. So insert a cond_resched() call after every 64k objects are put into the gray list.
--- mm/kmemleak.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
mm/kmemleak.c +19 -1
@@ -1417,12 +1417,15 @@ static void kmemleak_scan(void)struct zone *zone;int __maybe_unused i;int new_leaks =0 ; +int gray_list_cnt =0 ; jiffies_last_scan = jiffies;/* prepare the kmemleak_object's */ rcu_read_lock ();list_for_each_entry_rcu (object, &object_list, object_list) { +bool object_pinned = false; +raw_spin_lock_irq (&object->lock);#ifdef DEBUG /*
@@ -1437,10 +1440,25 @@ static void kmemleak_scan(void)#endif /* reset the reference count (whiten the object) */ object->count =0 ; -if (color_gray (object) &&get_object (object)) +if (color_gray (object) &&get_object (object)) {list_add_tail (&object->gray_list, &gray_list); + gray_list_cnt++; + object_pinned = true; + }raw_spin_unlock_irq (&object->lock); + +/* +* With object pinned by a positive reference count, it +* won't go away and we can safely release the RCU read +* lock and do a cond_resched() to avoid soft lockup every +* 64k objects. +*/ +if (object_pinned && !(gray_list_cnt &0xffff )) { +rcu_read_unlock (); +cond_resched (); +rcu_read_lock (); + } }rcu_read_unlock ();
Signature · 1 line
2.31.1
Catalin Marinas
maintainerRe: [PATCH 3/3] mm/kmemleak: Prevent soft lockup in first object iteration loop of kmemleak_scan()
Quoted 14 lines · Waiman Long
mm/kmemleak.c +6
-1@@ -1437,10 +1440,25 @@ static void kmemleak_scan(void)#endif /* reset the reference count (whiten the object) */ object->count =0 ; -if (color_gray (object) &&get_object (object)) +if (color_gray (object) &&get_object (object)) {list_add_tail (&object->gray_list, &gray_list); + gray_list_cnt++; + object_pinned = true; + }raw_spin_unlock_irq (&object->lock); + +/*
mm/kmemleak.c static void kmemleak_scan(void) · line 1461
Quoted 10 lines
mm/kmemleak.c +10
-0+* With object pinned by a positive reference count, it +* won't go away and we can safely release the RCU read +* lock and do a cond_resched() to avoid soft lockup every +* 64k objects. +*/ +if (object_pinned && !(gray_list_cnt &0xffff )) { +rcu_read_unlock (); +cond_resched (); +rcu_read_lock (); + }
I'm not sure this gains much. There should be very few gray objects initially (those passed to kmemleak_not_leak() for example). The majority should be white objects.
If we drop the fine-grained object->lock, we could instead take kmemleak_lock outside the loop with a cond_resched_lock(&kmemleak_lock) within the loop. I think we can get away with not having an rcu_read_lock() at all for list traversal with the big lock outside the loop.
The reason I added it in the first kmemleak incarnation was to defer kmemleak_object freeing as it was causing a re-entrant call into the slab allocator. I later went for fine-grained locking and RCU list traversal but I may have overdone it ;).
Signature · 1 line
Catalin
Catalin Marinas
maintainerRe: [PATCH 3/3] mm/kmemleak: Prevent soft lockup in first object iteration loop of kmemleak_scan()
Quoted 30 lines · Catalin Marinas
Quoted 14 lines · Waiman Long
mm/kmemleak.c +6
-1@@ -1437,10 +1440,25 @@ static void kmemleak_scan(void)#endif /* reset the reference count (whiten the object) */ object->count =0 ; -if (color_gray (object) &&get_object (object)) +if (color_gray (object) &&get_object (object)) {list_add_tail (&object->gray_list, &gray_list); + gray_list_cnt++; + object_pinned = true; + }raw_spin_unlock_irq (&object->lock); + +/* mm/kmemleak.c static void kmemleak_scan(void) · line 1461
Quoted 10 lines
mm/kmemleak.c +10
-0+* With object pinned by a positive reference count, it +* won't go away and we can safely release the RCU read +* lock and do a cond_resched() to avoid soft lockup every +* 64k objects. +*/ +if (object_pinned && !(gray_list_cnt &0xffff )) { +rcu_read_unlock (); +cond_resched (); +rcu_read_lock (); + }I'm not sure this gains much. There should be very few gray objects initially (those passed to
kmemleak_not_leak()for example). The majority should be white objects.
Quoted 5 lines
If we drop the fine-grained
object->lock, we could instead takekmemleak_lockoutside the loop with acond_resched_lock(&kmemleak_lock)within the loop. I think we can get away with not having anrcu_read_lock()at all for list traversal with the big lock outside the loop.
Actually this doesn't work is the current object in the iteration is freed. Does list_for_each_rcu_safe() help?
Signature · 1 line
Catalin
Re: [PATCH 3/3] mm/kmemleak: Prevent soft lockup in first object iteration loop of kmemleak_scan()
Quoted 13 lines · Catalin Marinas
Quoted 12 lines · Catalin Marinas
Review static void kmemleak_scan(void) · line 1448
Quoted 11 lines · Waiman Long
(quoted diff) +4
-1@@ -1437,10 +1440,25 @@ static void kmemleak_scan(void) #endif /* reset the reference count (whiten the object) */ object->count = 0; - if (color_gray(object) && get_object(object)) + if (color_gray(object) && get_object(object)) { list_add_tail(&object->gray_list, &gray_list); + gray_list_cnt++; + object_pinned = true; + }
I may have the mistaken belief that setting count to 0 will make most object gray. Apparently, that may not be the case here.
Quoted 17 lines
Quoted 17 lines
Review static void kmemleak_scan(void) · line 1461
Quoted 13 lines
(quoted diff) +12
-0raw_spin_unlock_irq(&object->lock); + + /* + * With object pinned by a positive reference count, it + * won't go away and we can safely release the RCU read + * lock and do a cond_resched() to avoid soft lockup every + * 64k objects. + */ + if (object_pinned && !(gray_list_cnt & 0xffff)) { + rcu_read_unlock(); + cond_resched(); + rcu_read_lock(); + }I'm not sure this gains much. There should be very few gray objects initially (those passed to
kmemleak_not_leak()for example). The majority should be white objects.
Quoted 7 lines
Quoted 5 lines
If we drop the fine-grained
object->lock, we could instead takekmemleak_lockoutside the loop with acond_resched_lock(&kmemleak_lock)within the loop. I think we can get away with not having anrcu_read_lock()at all for list traversal with the big lock outside the loop.Actually this doesn't work is the current object in the iteration is freed. Does
list_for_each_rcu_safe()help?
list_for_each_rcu_safe() helps if we are worrying about object being freed. However, it won't help if object->next is freed instead.
How about something like:
mm/kmemleak.c +0 -0
@@ -1417,12 +1417,16 @@ static void kmemleak_scan(void)
struct zone *zone;
int __maybe_unused i;
int new_leaks = 0;
+ int loop1_cnt = 0;
jiffies_last_scan = jiffies;
/* prepare the kmemleak_object's */ rcu_read_lock();
list_for_each_entry_rcu(object, &object_list, object_list) { + bool obj_pinned = false;
+
+ loop1_cnt++; raw_spin_lock_irq(&object->lock);
#ifdef DEBUG
/*
@@ -1437,10 +1441,32 @@ static void kmemleak_scan(void) #endif
/* reset the reference count (whiten the object) */ object->count = 0;
- if (color_gray(object) && get_object(object)) + if (color_gray(object) && get_object(object)) { list_add_tail(&object->gray_list, &gray_list); + obj_pinned = true;
+ }
raw_spin_unlock_irq(&object->lock);
+
+ /*
+ * Do a cond_resched() to avoid soft lockup every 64k objects. + * Make sure a reference has been taken so that the object + * won't go away without RCU read lock.
+ */
+ if (loop1_cnt & 0xffff) {
+ if (!obj_pinned && !get_object(object)) { + /* Try the next object instead */ + loop1_cnt--;
+ continue;
+ }
+
+ rcu_read_unlock();
+ cond_resched();
+ rcu_read_lock();
+
+ if (!obj_pinned)
+ put_object(object);
+ }
}
rcu_read_unlock();
Cheers,
Longman
Re: [PATCH 3/3] mm/kmemleak: Prevent soft lockup in first object iteration loop of kmemleak_scan()
Quoted 66 lines · Waiman Long
Quoted 17 lines · Catalin Marinas
Quoted 17 lines
Quoted 5 lines
raw_spin_unlock_irq(&object->lock);
+
+ /*
+ * With object pinned by a positive reference count, it + * won't go away and we can safely release the RCU readQuoted 8 lines
+ * lock and do a
cond_resched()to avoid soft lockup every + * 64k objects.
+ */
+ if (object_pinned&& !(gray_list_cnt &0xffff)) { +rcu_read_unlock();
+cond_resched();
+rcu_read_lock();
+ }I'm not sure this gains much. There should be very few gray objects initially (those passed to
kmemleak_not_leak()for example). The majority should be white objects.Quoted 7 lines
Quoted 5 lines
If we drop the fine-grained
object->lock, we could instead takekmemleak_lockoutside the loop with acond_resched_lock(&kmemleak_lock)within the loop. I think we can get away with not having anrcu_read_lock()at all for list traversal with the big lock outside the loop.Actually this doesn't work is the current object in the iteration is freed. Does
list_for_each_rcu_safe()help?
list_for_each_rcu_safe()helps if we are worrying about object being freed. However, it won't help ifobject->nextis freed instead.How about something like:
mm/kmemleak.c +10
-1@@ -1417,12 +1417,16 @@ static void kmemleak_scan(void) struct zone *zone; int __maybe_unused i; int new_leaks = 0; + int loop1_cnt = 0; jiffies_last_scan = jiffies; /* prepare the kmemleak_object's */ rcu_read_lock(); list_for_each_entry_rcu(object, &object_list, object_list) { + bool obj_pinned = false; + + loop1_cnt++; raw_spin_lock_irq(&object->lock); #ifdef DEBUG /*@@ -1437,10 +1441,32 @@ static void kmemleak_scan(void) #endif /* reset the reference count (whiten the object) */ object->count = 0; - if (color_gray(object) && get_object(object)) + if (color_gray(object) && get_object(object)) { list_add_tail(&object->gray_list, &gray_list); + obj_pinned = true; + } raw_spin_unlock_irq(&object->lock); + + /* +* Do a cond_resched() to avoid soft lockup every 64k
Quoted 6 lines
objects.
+ * Make sure a reference has been taken so that the object
+ * won't go away without RCU read lock. + */
+ if (loop1_cnt&0xffff) {
Sorry, should be "(!(loop1_cnt & 0xffff))".
Quoted 13 lines
+ if (!obj_pinned && !get_object(object)) {
+ /* Try the next object instead */ + loop1_cnt--;
+ continue;
+ }mm/kmemleak.c +8
-0+ + rcu_read_unlock(); + cond_resched(); + rcu_read_lock(); + + if (!obj_pinned) + put_object(object); + }
Quoted 3 lines
}
rcu_read_unlock();
Cheers,
Longman