diff mbox series

mm/kmemleak: Prevent soft lockup in kmemleak_scan()'s object iteration loops

Message ID 20221020175619.366317-1-longman@redhat.com
State New
Headers
Series mm/kmemleak: Prevent soft lockup in kmemleak_scan()'s object iteration loops |

Commit Message

Waiman Long Oct. 20, 2022, 5:56 p.m. UTC 
  Commit 6edda04ccc7c ("mm/kmemleak: prevent soft lockup in first object
iteration loop of kmemleak_scan()") adds cond_resched() in the first
object iteration loop of kmemleak_scan(). However, it turns that the
2nd objection iteration loop can still cause soft lockup to happen in
some cases. So add a cond_resched() call in the 2nd and 3rd loops as
well to prevent that and for completeness.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 mm/kmemleak.c | 61 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 19 deletions(-)

Comments

Andrew Morton Oct. 21, 2022, 12:58 a.m. UTC | #1 
On Thu, 20 Oct 2022 13:56:19 -0400 Waiman Long <longman@redhat.com> wrote:

> Commit 6edda04ccc7c ("mm/kmemleak: prevent soft lockup in first object
> iteration loop of kmemleak_scan()") adds cond_resched() in the first
> object iteration loop of kmemleak_scan(). However, it turns that the
> 2nd objection iteration loop can still cause soft lockup to happen in
> some cases. So add a cond_resched() call in the 2nd and 3rd loops as
> well to prevent that and for completeness.
> 

Seems reasonable, although not an object of beauty.

We didn't include a Fixes: or cc:stable in 6edda04ccc7c, even though it
addresses softlockups.  I think I'll add a cc:stable to this, with a
Fixes:6edda04ccc7c.  So any kenrel which included 6edda04ccc7c will
hopefully add this as well.
Waiman Long Oct. 21, 2022, 1:22 a.m. UTC | #2 
On 10/20/22 20:58, Andrew Morton wrote:
> On Thu, 20 Oct 2022 13:56:19 -0400 Waiman Long <longman@redhat.com> wrote:
>
>> Commit 6edda04ccc7c ("mm/kmemleak: prevent soft lockup in first object
>> iteration loop of kmemleak_scan()") adds cond_resched() in the first
>> object iteration loop of kmemleak_scan(). However, it turns that the
>> 2nd objection iteration loop can still cause soft lockup to happen in
>> some cases. So add a cond_resched() call in the 2nd and 3rd loops as
>> well to prevent that and for completeness.
>>
> Seems reasonable, although not an object of beauty.
>
> We didn't include a Fixes: or cc:stable in 6edda04ccc7c, even though it
> addresses softlockups.  I think I'll add a cc:stable to this, with a
> Fixes:6edda04ccc7c.  So any kenrel which included 6edda04ccc7c will
> hopefully add this as well.

That sounds good to me. I consider commit 6edda04ccc7c incomplete and 
this patch complete it. Technically it can be considered a fix.

Thanks,
Longman
Catalin Marinas Oct. 28, 2022, 1:59 p.m. UTC | #3 
On Thu, Oct 20, 2022 at 01:56:19PM -0400, Waiman Long wrote:
> Commit 6edda04ccc7c ("mm/kmemleak: prevent soft lockup in first object
> iteration loop of kmemleak_scan()") adds cond_resched() in the first
> object iteration loop of kmemleak_scan(). However, it turns that the
> 2nd objection iteration loop can still cause soft lockup to happen in
> some cases. So add a cond_resched() call in the 2nd and 3rd loops as
> well to prevent that and for completeness.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
diff mbox series

Patch

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 1eddc0132f7f..613d34b57c5d 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1463,6 +1463,27 @@  static void scan_gray_list(void)
 	WARN_ON(!list_empty(&gray_list));
 }
 
+/*
+ * Conditionally call resched() in a object iteration loop while making sure
+ * that the given object won't go away without RCU read lock by performing a
+ * get_object() if !pinned.
+ *
+ * Return: false if can't do a cond_resched() due to get_object() failure
+ *	   true otherwise
+ */
+static bool kmemleak_cond_resched(struct kmemleak_object *object, bool pinned)
+{
+	if (!pinned && !get_object(object))
+		return false;
+
+	rcu_read_unlock();
+	cond_resched();
+	rcu_read_lock();
+	if (!pinned)
+		put_object(object);
+	return true;
+}
+
 /*
  * Scan data sections and all the referenced memory blocks allocated via the
  * kernel's standard allocators. This function must be called with the
@@ -1474,7 +1495,7 @@  static void kmemleak_scan(void)
 	struct zone *zone;
 	int __maybe_unused i;
 	int new_leaks = 0;
-	int loop1_cnt = 0;
+	int loop_cnt = 0;
 
 	jiffies_last_scan = jiffies;
 
@@ -1483,7 +1504,6 @@  static void kmemleak_scan(void)
 	list_for_each_entry_rcu(object, &object_list, object_list) {
 		bool obj_pinned = false;
 
-		loop1_cnt++;
 		raw_spin_lock_irq(&object->lock);
 #ifdef DEBUG
 		/*
@@ -1517,24 +1537,11 @@  static void kmemleak_scan(void)
 		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.
+		 * Do a cond_resched() every 64k objects to avoid soft lockup.
 		 */
-		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);
-		}
+		if (!(++loop_cnt & 0xffff) &&
+		    !kmemleak_cond_resched(object, obj_pinned))
+			loop_cnt--; /* Try again on next object */
 	}
 	rcu_read_unlock();
 
@@ -1601,7 +1608,15 @@  static void kmemleak_scan(void)
 	 * scan and color them gray until the next scan.
 	 */
 	rcu_read_lock();
+	loop_cnt = 0;
 	list_for_each_entry_rcu(object, &object_list, object_list) {
+		/*
+		 * Do a cond_resched() every 64k objects to avoid soft lockup.
+		 */
+		if (!(++loop_cnt & 0xffff) &&
+		    !kmemleak_cond_resched(object, false))
+			loop_cnt--;	/* Try again on next object */
+
 		/*
 		 * This is racy but we can save the overhead of lock/unlock
 		 * calls. The missed objects, if any, should be caught in
@@ -1635,7 +1650,15 @@  static void kmemleak_scan(void)
 	 * Scanning result reporting.
 	 */
 	rcu_read_lock();
+	loop_cnt = 0;
 	list_for_each_entry_rcu(object, &object_list, object_list) {
+		/*
+		 * Do a cond_resched() every 64k objects to avoid soft lockup.
+		 */
+		if (!(++loop_cnt & 0xffff) &&
+		    !kmemleak_cond_resched(object, false))
+			loop_cnt--;	/* Try again on next object */
+
 		/*
 		 * This is racy but we can save the overhead of lock/unlock
 		 * calls. The missed objects, if any, should be caught in