[PATCH] kasan: avoid unnecessary quarantine lock cycling

Hui Su posted 1 patch 1 month, 3 weeks ago
mm/kasan/quarantine.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
[PATCH] kasan: avoid unnecessary quarantine lock cycling
Posted by Hui Su 1 month, 3 weeks ago
kasan_quarantine_remove_cache() drops quarantine_lock and restores
interrupts after scanning each non-empty quarantine batch so that
cond_resched() can schedule during a long scan.

When no reschedule is pending, the unlock/IRQ restore and subsequent
IRQ save/relock cycle is unnecessary.

Check need_resched() before cycling quarantine_lock to avoid this
overhead when rescheduling is not needed.

Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Hui Su <sh_def@163.com>
---
 mm/kasan/quarantine.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 6958aa713c67..db7170f6f1b2 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -368,10 +368,12 @@ void kasan_quarantine_remove_cache(struct kmem_cache *cache)
 		if (qlist_empty(&global_quarantine[i]))
 			continue;
 		qlist_move_cache(&global_quarantine[i], &to_free, cache);
-		/* Scanning whole quarantine can take a while. */
-		raw_spin_unlock_irqrestore(&quarantine_lock, flags);
-		cond_resched();
-		raw_spin_lock_irqsave(&quarantine_lock, flags);
+		if (need_resched()) {
+			/* Scanning whole quarantine can take a while. */
+			raw_spin_unlock_irqrestore(&quarantine_lock, flags);
+			cond_resched();
+			raw_spin_lock_irqsave(&quarantine_lock, flags);
+		}
 	}
 	raw_spin_unlock_irqrestore(&quarantine_lock, flags);
 
-- 
2.43.0
Re: [PATCH] kasan: avoid unnecessary quarantine lock cycling
Posted by Andrey Ryabinin 1 month, 2 weeks ago
Hui Su <sh_def@163.com> writes:

> kasan_quarantine_remove_cache() drops quarantine_lock and restores
> interrupts after scanning each non-empty quarantine batch so that
> cond_resched() can schedule during a long scan.
>
> When no reschedule is pending, the unlock/IRQ restore and subsequent
> IRQ save/relock cycle is unnecessary.
>
> Check need_resched() before cycling quarantine_lock to avoid this
> overhead when rescheduling is not needed.
>
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Hui Su <sh_def@163.com>
> ---
>  mm/kasan/quarantine.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 6958aa713c67..db7170f6f1b2 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -368,10 +368,12 @@ void kasan_quarantine_remove_cache(struct kmem_cache *cache)
>  		if (qlist_empty(&global_quarantine[i]))
>  			continue;
>  		qlist_move_cache(&global_quarantine[i], &to_free, cache);
> -		/* Scanning whole quarantine can take a while. */
> -		raw_spin_unlock_irqrestore(&quarantine_lock, flags);
> -		cond_resched();
> -		raw_spin_lock_irqsave(&quarantine_lock, flags);
> +		if (need_resched()) {

I'm not sure about this one and inclined to agree with sashiko here.
See https://sashiko.dev/#/patchset/20260808191230.3345930-2-sh_def%40163.com

With local IRQs disabled, the local timer tick cannot run, so it cannot
set TIF_NEED_RESCHED due to time-slice expiration. Perhaps another CPU may set
the flag remotely, but AFAIU nothing guarantees that this will happen.

If need_resched() is initially false, it may remain false for the entire scan.
IRQs would then remain disabled throughout, potentially long enough to trigger
a HARDLOCKUP.

Current code should be fine, the bulk of the work is done by qlist_move_cache(),
and the unlock/reschedule/relock sequence occurs only after each
non-empty batch,
at most QUARANTINE_BATCHES times.

> +			/* Scanning whole quarantine can take a while. */
> +			raw_spin_unlock_irqrestore(&quarantine_lock, flags);
> +			cond_resched();
> +			raw_spin_lock_irqsave(&quarantine_lock, flags);
> +		}
>  	}
>  	raw_spin_unlock_irqrestore(&quarantine_lock, flags);
>
> --
> 2.43.0