Re: [PATCH] kasan: convert kasan/quarantine_lock to raw_spinlock

Re: [PATCH] kasan: convert kasan/quarantine_lock to raw_spinlock

[Sebastian Andrzej Siewior]

On 2018-09-18 10:29:31 [-0500], Clark Williams wrote:

So I received this from Clark:

> The static lock quarantine_lock is used in mm/kasan/quarantine.c to protect
> the quarantine queue datastructures. It is taken inside quarantine queue
> manipulation routines (quarantine_put(), quarantine_reduce() and quarantine_remove_cache()),
> with IRQs disabled. This is no problem on a stock kernel but is problematic
> on an RT kernel where spin locks are converted to rt_mutex_t, which can sleep.
> 
> Convert the quarantine_lock to a raw spinlock. The usage of quarantine_lock
> is confined to quarantine.c and the work performed while the lock is held is limited.
> 
> Signed-off-by: Clark Williams <williams@redhat.com>

This is the minimum to get this working on RT splat free. There is one
memory deallocation with irqs off which should work on RT in its current
way.
Once this and the on_each_cpu() invocation, I was wondering if…

> ---
>  mm/kasan/quarantine.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 3a8ddf8baf7d..b209dbaefde8 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -103,7 +103,7 @@ static int quarantine_head;
>  static int quarantine_tail;
>  /* Total size of all objects in global_quarantine across all batches. */
>  static unsigned long quarantine_size;
> -static DEFINE_SPINLOCK(quarantine_lock);
> +static DEFINE_RAW_SPINLOCK(quarantine_lock);
>  DEFINE_STATIC_SRCU(remove_cache_srcu);
>  
>  /* Maximum size of the global queue. */
> @@ -190,7 +190,7 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
>  	if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
>  		qlist_move_all(q, &temp);
>  
> -		spin_lock(&quarantine_lock);
> +		raw_spin_lock(&quarantine_lock);
>  		WRITE_ONCE(quarantine_size, quarantine_size + temp.bytes);
>  		qlist_move_all(&temp, &global_quarantine[quarantine_tail]);
>  		if (global_quarantine[quarantine_tail].bytes >=
> @@ -203,7 +203,7 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
>  			if (new_tail != quarantine_head)
>  				quarantine_tail = new_tail;
>  		}
> -		spin_unlock(&quarantine_lock);
> +		raw_spin_unlock(&quarantine_lock);
>  	}
>  
>  	local_irq_restore(flags);
> @@ -230,7 +230,7 @@ void quarantine_reduce(void)
>  	 * expected case).
>  	 */
>  	srcu_idx = srcu_read_lock(&remove_cache_srcu);
> -	spin_lock_irqsave(&quarantine_lock, flags);
> +	raw_spin_lock_irqsave(&quarantine_lock, flags);
>  
>  	/*
>  	 * Update quarantine size in case of hotplug. Allocate a fraction of
> @@ -254,7 +254,7 @@ void quarantine_reduce(void)
>  			quarantine_head = 0;
>  	}
>  
> -	spin_unlock_irqrestore(&quarantine_lock, flags);
> +	raw_spin_unlock_irqrestore(&quarantine_lock, flags);
>  
>  	qlist_free_all(&to_free, NULL);
>  	srcu_read_unlock(&remove_cache_srcu, srcu_idx);
> @@ -310,17 +310,17 @@ void quarantine_remove_cache(struct kmem_cache *cache)
>  	 */
>  	on_each_cpu(per_cpu_remove_cache, cache, 1);
>  
> -	spin_lock_irqsave(&quarantine_lock, flags);
> +	raw_spin_lock_irqsave(&quarantine_lock, flags);
>  	for (i = 0; i < QUARANTINE_BATCHES; i++) {
>  		if (qlist_empty(&global_quarantine[i]))
>  			continue;
>  		qlist_move_cache(&global_quarantine[i], &to_free, cache);
>  		/* Scanning whole quarantine can take a while. */
> -		spin_unlock_irqrestore(&quarantine_lock, flags);
> +		raw_spin_unlock_irqrestore(&quarantine_lock, flags);
>  		cond_resched();
> -		spin_lock_irqsave(&quarantine_lock, flags);
> +		raw_spin_lock_irqsave(&quarantine_lock, flags);
>  	}
> -	spin_unlock_irqrestore(&quarantine_lock, flags);
> +	raw_spin_unlock_irqrestore(&quarantine_lock, flags);
>  
>  	qlist_free_all(&to_free, cache);
>  
> -- 
> 2.17.1
> 

Sebastian

Re: [PATCH] kasan: convert kasan/quarantine_lock to raw_spinlock

[Sebastian Andrzej Siewior]

On 2018-10-05 18:30:18 [+0200], To Clark Williams wrote:
> This is the minimum to get this working on RT splat free. There is one
> memory deallocation with irqs off which should work on RT in its current
> way.
> Once this and the on_each_cpu() invocation, I was wondering if…

the patch at the bottom wouldn't work just fine for everyone.
It would have the beaty of annotating the locking scope a little and
avoiding the on_each_cpu() invocation. No local_irq_save() but actually
the proper locking primitives.
I haven't fully decoded the srcu part in the code.

Wouldn't that work for you?

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 mm/kasan/quarantine.c | 45 +++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 3a8ddf8baf7dc..8ed595960e3c1 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -39,12 +39,13 @@
  * objects inside of it.
  */
 struct qlist_head {
+	spinlock_t	lock;
 	struct qlist_node *head;
 	struct qlist_node *tail;
 	size_t bytes;
 };
 
-#define QLIST_INIT { NULL, NULL, 0 }
+#define QLIST_INIT {.head = NULL, .tail = NULL, .bytes = 0 }
 
 static bool qlist_empty(struct qlist_head *q)
 {
@@ -95,7 +96,9 @@ static void qlist_move_all(struct qlist_head *from, struct qlist_head *to)
  * The object quarantine consists of per-cpu queues and a global queue,
  * guarded by quarantine_lock.
  */
-static DEFINE_PER_CPU(struct qlist_head, cpu_quarantine);
+static DEFINE_PER_CPU(struct qlist_head, cpu_quarantine) = {
+	.lock = __SPIN_LOCK_UNLOCKED(cpu_quarantine.lock),
+};
 
 /* Round-robin FIFO array of batches. */
 static struct qlist_head global_quarantine[QUARANTINE_BATCHES];
@@ -183,12 +186,13 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
 	 * beginning which ensures that it either sees the objects in per-cpu
 	 * lists or in the global quarantine.
 	 */
-	local_irq_save(flags);
+	q = raw_cpu_ptr(&cpu_quarantine);
+	spin_lock_irqsave(&q->lock, flags);
 
-	q = this_cpu_ptr(&cpu_quarantine);
 	qlist_put(q, &info->quarantine_link, cache->size);
 	if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
 		qlist_move_all(q, &temp);
+		spin_unlock(&q->lock);
 
 		spin_lock(&quarantine_lock);
 		WRITE_ONCE(quarantine_size, quarantine_size + temp.bytes);
@@ -203,10 +207,10 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
 			if (new_tail != quarantine_head)
 				quarantine_tail = new_tail;
 		}
-		spin_unlock(&quarantine_lock);
+		spin_unlock_irqrestore(&quarantine_lock, flags);
+	} else {
+		spin_unlock_irqrestore(&q->lock, flags);
 	}
-
-	local_irq_restore(flags);
 }
 
 void quarantine_reduce(void)
@@ -284,21 +288,11 @@ static void qlist_move_cache(struct qlist_head *from,
 	}
 }
 
-static void per_cpu_remove_cache(void *arg)
-{
-	struct kmem_cache *cache = arg;
-	struct qlist_head to_free = QLIST_INIT;
-	struct qlist_head *q;
-
-	q = this_cpu_ptr(&cpu_quarantine);
-	qlist_move_cache(q, &to_free, cache);
-	qlist_free_all(&to_free, cache);
-}
-
 /* Free all quarantined objects belonging to cache. */
 void quarantine_remove_cache(struct kmem_cache *cache)
 {
 	unsigned long flags, i;
+	unsigned int cpu;
 	struct qlist_head to_free = QLIST_INIT;
 
 	/*
@@ -308,7 +302,20 @@ void quarantine_remove_cache(struct kmem_cache *cache)
 	 * achieves the first goal, while synchronize_srcu() achieves the
 	 * second.
 	 */
-	on_each_cpu(per_cpu_remove_cache, cache, 1);
+	/* get_online_cpus() invoked by caller */
+	for_each_online_cpu(cpu) {
+		struct qlist_head *q;
+		unsigned long flags;
+		struct qlist_head to_free = QLIST_INIT;
+
+		q = per_cpu_ptr(&cpu_quarantine, cpu);
+		spin_lock_irqsave(&q->lock, flags);
+		qlist_move_cache(q, &to_free, cache);
+		spin_unlock_irqrestore(&q->lock, flags);
+
+		qlist_free_all(&to_free, cache);
+
+	}
 
 	spin_lock_irqsave(&quarantine_lock, flags);
 	for (i = 0; i < QUARANTINE_BATCHES; i++) {
-- 
2.19.0

Re: [PATCH] kasan: convert kasan/quarantine_lock to raw_spinlock

[Clark Williams]

I applied this patch to a fairly stock 4.18-rt5 kernel and booted with no complaints, then
ran rteval for 12h with no stack splats reported. I'll keep banging on it but preliminary
reports look good. 

Clark

On Fri, 5 Oct 2018 18:33:20 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> On 2018-10-05 18:30:18 [+0200], To Clark Williams wrote:
> > This is the minimum to get this working on RT splat free. There is one
> > memory deallocation with irqs off which should work on RT in its current
> > way.
> > Once this and the on_each_cpu() invocation, I was wondering if…  
> 
> the patch at the bottom wouldn't work just fine for everyone.
> It would have the beaty of annotating the locking scope a little and
> avoiding the on_each_cpu() invocation. No local_irq_save() but actually
> the proper locking primitives.
> I haven't fully decoded the srcu part in the code.
> 
> Wouldn't that work for you?
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  mm/kasan/quarantine.c | 45 +++++++++++++++++++++++++------------------
>  1 file changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 3a8ddf8baf7dc..8ed595960e3c1 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -39,12 +39,13 @@
>   * objects inside of it.
>   */
>  struct qlist_head {
> +	spinlock_t	lock;
>  	struct qlist_node *head;
>  	struct qlist_node *tail;
>  	size_t bytes;
>  };
>  
> -#define QLIST_INIT { NULL, NULL, 0 }
> +#define QLIST_INIT {.head = NULL, .tail = NULL, .bytes = 0 }
>  
>  static bool qlist_empty(struct qlist_head *q)
>  {
> @@ -95,7 +96,9 @@ static void qlist_move_all(struct qlist_head *from, struct qlist_head *to)
>   * The object quarantine consists of per-cpu queues and a global queue,
>   * guarded by quarantine_lock.
>   */
> -static DEFINE_PER_CPU(struct qlist_head, cpu_quarantine);
> +static DEFINE_PER_CPU(struct qlist_head, cpu_quarantine) = {
> +	.lock = __SPIN_LOCK_UNLOCKED(cpu_quarantine.lock),
> +};
>  
>  /* Round-robin FIFO array of batches. */
>  static struct qlist_head global_quarantine[QUARANTINE_BATCHES];
> @@ -183,12 +186,13 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
>  	 * beginning which ensures that it either sees the objects in per-cpu
>  	 * lists or in the global quarantine.
>  	 */
> -	local_irq_save(flags);
> +	q = raw_cpu_ptr(&cpu_quarantine);
> +	spin_lock_irqsave(&q->lock, flags);
>  
> -	q = this_cpu_ptr(&cpu_quarantine);
>  	qlist_put(q, &info->quarantine_link, cache->size);
>  	if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
>  		qlist_move_all(q, &temp);
> +		spin_unlock(&q->lock);
>  
>  		spin_lock(&quarantine_lock);
>  		WRITE_ONCE(quarantine_size, quarantine_size + temp.bytes);
> @@ -203,10 +207,10 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
>  			if (new_tail != quarantine_head)
>  				quarantine_tail = new_tail;
>  		}
> -		spin_unlock(&quarantine_lock);
> +		spin_unlock_irqrestore(&quarantine_lock, flags);
> +	} else {
> +		spin_unlock_irqrestore(&q->lock, flags);
>  	}
> -
> -	local_irq_restore(flags);
>  }
>  
>  void quarantine_reduce(void)
> @@ -284,21 +288,11 @@ static void qlist_move_cache(struct qlist_head *from,
>  	}
>  }
>  
> -static void per_cpu_remove_cache(void *arg)
> -{
> -	struct kmem_cache *cache = arg;
> -	struct qlist_head to_free = QLIST_INIT;
> -	struct qlist_head *q;
> -
> -	q = this_cpu_ptr(&cpu_quarantine);
> -	qlist_move_cache(q, &to_free, cache);
> -	qlist_free_all(&to_free, cache);
> -}
> -
>  /* Free all quarantined objects belonging to cache. */
>  void quarantine_remove_cache(struct kmem_cache *cache)
>  {
>  	unsigned long flags, i;
> +	unsigned int cpu;
>  	struct qlist_head to_free = QLIST_INIT;
>  
>  	/*
> @@ -308,7 +302,20 @@ void quarantine_remove_cache(struct kmem_cache *cache)
>  	 * achieves the first goal, while synchronize_srcu() achieves the
>  	 * second.
>  	 */
> -	on_each_cpu(per_cpu_remove_cache, cache, 1);
> +	/* get_online_cpus() invoked by caller */
> +	for_each_online_cpu(cpu) {
> +		struct qlist_head *q;
> +		unsigned long flags;
> +		struct qlist_head to_free = QLIST_INIT;
> +
> +		q = per_cpu_ptr(&cpu_quarantine, cpu);
> +		spin_lock_irqsave(&q->lock, flags);
> +		qlist_move_cache(q, &to_free, cache);
> +		spin_unlock_irqrestore(&q->lock, flags);
> +
> +		qlist_free_all(&to_free, cache);
> +
> +	}
>  
>  	spin_lock_irqsave(&quarantine_lock, flags);
>  	for (i = 0; i < QUARANTINE_BATCHES; i++) {
> -- 
> 2.19.0
> 


-- 
The United States Coast Guard
Ruining Natural Selection since 1790

Re: [PATCH] kasan: convert kasan/quarantine_lock to raw_spinlock

[Dmitry Vyukov]

On Fri, Oct 5, 2018 at 6:33 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 2018-10-05 18:30:18 [+0200], To Clark Williams wrote:
>> This is the minimum to get this working on RT splat free. There is one
>> memory deallocation with irqs off which should work on RT in its current
>> way.
>> Once this and the on_each_cpu() invocation, I was wondering if…
>
> the patch at the bottom wouldn't work just fine for everyone.
> It would have the beaty of annotating the locking scope a little and
> avoiding the on_each_cpu() invocation. No local_irq_save() but actually
> the proper locking primitives.
> I haven't fully decoded the srcu part in the code.
>
> Wouldn't that work for you?

Hi Sebastian,

This seems to beak quarantine_remove_cache( ) in the sense that some
object from the cache may still be in quarantine when
quarantine_remove_cache() returns. When quarantine_remove_cache()
returns all objects from the cache must be purged from quarantine.
That srcu and irq trickery is there for a reason.
This code is also on hot path of kmallock/kfree, an additional
lock/unlock per operation is expensive. Adding 2 locked RMW per
kmalloc is not something that should be done only out of refactoring
reasons.

The original message from Clark mentions that the problem can be fixed
by just changing type of spinlock. This looks like a better and
simpler way to resolve the problem to me.


> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  mm/kasan/quarantine.c | 45 +++++++++++++++++++++++++------------------
>  1 file changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 3a8ddf8baf7dc..8ed595960e3c1 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -39,12 +39,13 @@
>   * objects inside of it.
>   */
>  struct qlist_head {
> +       spinlock_t      lock;
>         struct qlist_node *head;
>         struct qlist_node *tail;
>         size_t bytes;
>  };
>
> -#define QLIST_INIT { NULL, NULL, 0 }
> +#define QLIST_INIT {.head = NULL, .tail = NULL, .bytes = 0 }
>
>  static bool qlist_empty(struct qlist_head *q)
>  {
> @@ -95,7 +96,9 @@ static void qlist_move_all(struct qlist_head *from, struct qlist_head *to)
>   * The object quarantine consists of per-cpu queues and a global queue,
>   * guarded by quarantine_lock.
>   */
> -static DEFINE_PER_CPU(struct qlist_head, cpu_quarantine);
> +static DEFINE_PER_CPU(struct qlist_head, cpu_quarantine) = {
> +       .lock = __SPIN_LOCK_UNLOCKED(cpu_quarantine.lock),
> +};
>
>  /* Round-robin FIFO array of batches. */
>  static struct qlist_head global_quarantine[QUARANTINE_BATCHES];
> @@ -183,12 +186,13 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
>          * beginning which ensures that it either sees the objects in per-cpu
>          * lists or in the global quarantine.
>          */
> -       local_irq_save(flags);
> +       q = raw_cpu_ptr(&cpu_quarantine);
> +       spin_lock_irqsave(&q->lock, flags);
>
> -       q = this_cpu_ptr(&cpu_quarantine);
>         qlist_put(q, &info->quarantine_link, cache->size);
>         if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
>                 qlist_move_all(q, &temp);
> +               spin_unlock(&q->lock);
>
>                 spin_lock(&quarantine_lock);
>                 WRITE_ONCE(quarantine_size, quarantine_size + temp.bytes);
> @@ -203,10 +207,10 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
>                         if (new_tail != quarantine_head)
>                                 quarantine_tail = new_tail;
>                 }
> -               spin_unlock(&quarantine_lock);
> +               spin_unlock_irqrestore(&quarantine_lock, flags);
> +       } else {
> +               spin_unlock_irqrestore(&q->lock, flags);
>         }
> -
> -       local_irq_restore(flags);
>  }
>
>  void quarantine_reduce(void)
> @@ -284,21 +288,11 @@ static void qlist_move_cache(struct qlist_head *from,
>         }
>  }
>
> -static void per_cpu_remove_cache(void *arg)
> -{
> -       struct kmem_cache *cache = arg;
> -       struct qlist_head to_free = QLIST_INIT;
> -       struct qlist_head *q;
> -
> -       q = this_cpu_ptr(&cpu_quarantine);
> -       qlist_move_cache(q, &to_free, cache);
> -       qlist_free_all(&to_free, cache);
> -}
> -
>  /* Free all quarantined objects belonging to cache. */
>  void quarantine_remove_cache(struct kmem_cache *cache)
>  {
>         unsigned long flags, i;
> +       unsigned int cpu;
>         struct qlist_head to_free = QLIST_INIT;
>
>         /*
> @@ -308,7 +302,20 @@ void quarantine_remove_cache(struct kmem_cache *cache)
>          * achieves the first goal, while synchronize_srcu() achieves the
>          * second.
>          */
> -       on_each_cpu(per_cpu_remove_cache, cache, 1);
> +       /* get_online_cpus() invoked by caller */
> +       for_each_online_cpu(cpu) {
> +               struct qlist_head *q;
> +               unsigned long flags;
> +               struct qlist_head to_free = QLIST_INIT;
> +
> +               q = per_cpu_ptr(&cpu_quarantine, cpu);
> +               spin_lock_irqsave(&q->lock, flags);
> +               qlist_move_cache(q, &to_free, cache);
> +               spin_unlock_irqrestore(&q->lock, flags);
> +
> +               qlist_free_all(&to_free, cache);
> +
> +       }
>
>         spin_lock_irqsave(&quarantine_lock, flags);
>         for (i = 0; i < QUARANTINE_BATCHES; i++) {
> --
> 2.19.0
>

Re: [PATCH] kasan: convert kasan/quarantine_lock to raw_spinlock

[Sebastian Andrzej Siewior]

On 2018-10-08 11:15:57 [+0200], Dmitry Vyukov wrote:
> Hi Sebastian,
Hi Dmitry,

> This seems to beak quarantine_remove_cache( ) in the sense that some
> object from the cache may still be in quarantine when
> quarantine_remove_cache() returns. When quarantine_remove_cache()
> returns all objects from the cache must be purged from quarantine.
> That srcu and irq trickery is there for a reason.

That loop should behave like your on_each_cpu() except it does not
involve the remote CPU.

> This code is also on hot path of kmallock/kfree, an additional
> lock/unlock per operation is expensive. Adding 2 locked RMW per
> kmalloc is not something that should be done only out of refactoring
> reasons.
But this is debug code anyway, right? And it is highly complex imho.
Well, maybe only for me after I looked at it for the first time…

> The original message from Clark mentions that the problem can be fixed
> by just changing type of spinlock. This looks like a better and
> simpler way to resolve the problem to me.

I usually prefer to avoid adding raw_locks everywhere if it can be
avoided. However given that this is debug code and a few additional us
shouldn't matter here, I have no problem with Clark's initial patch
(also the mem-free in irq-off region works in this scenario).
Can you take it as-is or should I repost it with an acked-by?

Sebastian

Re: [PATCH] kasan: convert kasan/quarantine_lock to raw_spinlock

[Dmitry Vyukov]

On Tue, Oct 9, 2018 at 4:27 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 2018-10-08 11:15:57 [+0200], Dmitry Vyukov wrote:
>> Hi Sebastian,
> Hi Dmitry,
>
>> This seems to beak quarantine_remove_cache( ) in the sense that some
>> object from the cache may still be in quarantine when
>> quarantine_remove_cache() returns. When quarantine_remove_cache()
>> returns all objects from the cache must be purged from quarantine.
>> That srcu and irq trickery is there for a reason.
>
> That loop should behave like your on_each_cpu() except it does not
> involve the remote CPU.


The problem is that it can squeeze in between:

+               spin_unlock(&q->lock);

                spin_lock(&quarantine_lock);

as far as I see. And then some objects can be left in the quarantine.


>> This code is also on hot path of kmallock/kfree, an additional
>> lock/unlock per operation is expensive. Adding 2 locked RMW per
>> kmalloc is not something that should be done only out of refactoring
>> reasons.
> But this is debug code anyway, right? And it is highly complex imho.
> Well, maybe only for me after I looked at it for the first time…

It is debug code - yes.
Nothing about its performance matters - no.

That's the way to produce unusable debug tools.
With too much overhead timeouts start to fire and code behaves not the
way it behaves in production.
The tool is used in continuous integration and developers wait for
test results before merging code.
The tool is used on canary devices and directly contributes to usage experience.

We of course don't want to trade a page of assembly code for cutting
few cycles here (something that could make sense for some networking
code maybe). But otherwise let's not introduce spinlocks on fast paths
just for refactoring reasons.


>> The original message from Clark mentions that the problem can be fixed
>> by just changing type of spinlock. This looks like a better and
>> simpler way to resolve the problem to me.
>
> I usually prefer to avoid adding raw_locks everywhere if it can be
> avoided. However given that this is debug code and a few additional us
> shouldn't matter here, I have no problem with Clark's initial patch
> (also the mem-free in irq-off region works in this scenario).
> Can you take it as-is or should I repost it with an acked-by?

Perhaps it's the problem with the way RT kernel changes things then?
This is not specific to quarantine, right? Should that mutex detect
that IRQs are disabled and not try to schedule? If this would happen
in some networking code, what would we do?

Re: [PATCH] kasan: convert kasan/quarantine_lock to raw_spinlock

[Sebastian Andrzej Siewior]

On 2018-10-10 10:25:42 [+0200], Dmitry Vyukov wrote:
> > That loop should behave like your on_each_cpu() except it does not
> > involve the remote CPU.
> 
> 
> The problem is that it can squeeze in between:
> 
> +               spin_unlock(&q->lock);
> 
>                 spin_lock(&quarantine_lock);
> 
> as far as I see. And then some objects can be left in the quarantine.

Okay. But then once you are at CPU10 (in the on_each_cpu() loop) there
can be objects which are added to CPU0, right? So based on that, I
assumed that this would be okay to drop the lock here. 

> > But this is debug code anyway, right? And it is highly complex imho.
> > Well, maybe only for me after I looked at it for the first time…
> 
> It is debug code - yes.
> Nothing about its performance matters - no.
> 
> That's the way to produce unusable debug tools.
> With too much overhead timeouts start to fire and code behaves not the
> way it behaves in production.
> The tool is used in continuous integration and developers wait for
> test results before merging code.
> The tool is used on canary devices and directly contributes to usage experience.

Completely understood. What I meant is that debug code in general (from
RT perspective) increases latency to a level where the device can not
operate. Take lockdep for instance. Debug facility which is required
for RT as it spots locking problems early. It increases the latency
(depending on the workload) by 50ms+ and can't be used in production.
Same goes for SLUB debug and most other.

> We of course don't want to trade a page of assembly code for cutting
> few cycles here (something that could make sense for some networking
> code maybe). But otherwise let's not introduce spinlocks on fast paths
> just for refactoring reasons.

Sure. As I said. I'm fine with patch Clark initially proposed. I assumed
the refactoring would make things simpler and avoiding the cross-CPU
call a good thing.

> > Can you take it as-is or should I repost it with an acked-by?
> 
> Perhaps it's the problem with the way RT kernel changes things then?
> This is not specific to quarantine, right? 

We got rid of _a lot_ of local_irq_disable/save() + spin_lock() combos
which were there for reasons which are no longer true or due to lack of
the API. And this kasan thing is just something Clark stumbled upon
recently. And I try to negotiate something where everyone can agree on.

> Should that mutex detect
> that IRQs are disabled and not try to schedule? If this would happen
> in some networking code, what would we do?

It is not only that it is supposed not to schedule. Assuming the "mutex"
is not owned you could acquire it right away. No scheduling. However,
you would record current() as the owner of the lock which is wrong and
you get into other trouble later on. The list goes on :)
However, networking. If there is something that breaks then it will be
addressed. It will be forwarded upstream if this something where it
is likely to assume that RT won't change. So networking isn't special.

Should I repost Clark's patch?

Sebastian

Re: [PATCH] kasan: convert kasan/quarantine_lock to raw_spinlock

[Dmitry Vyukov]

On Wed, Oct 10, 2018 at 11:29 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 2018-10-10 10:25:42 [+0200], Dmitry Vyukov wrote:
>> > That loop should behave like your on_each_cpu() except it does not
>> > involve the remote CPU.
>>
>>
>> The problem is that it can squeeze in between:
>>
>> +               spin_unlock(&q->lock);
>>
>>                 spin_lock(&quarantine_lock);
>>
>> as far as I see. And then some objects can be left in the quarantine.
>
> Okay. But then once you are at CPU10 (in the on_each_cpu() loop) there
> can be objects which are added to CPU0, right? So based on that, I
> assumed that this would be okay to drop the lock here.

What happens here is a bit tricky.

When a kmem cache is freed, all objects from the cache must be already
free. That is kmem_cache_free has returned for all objects (otherwise
it's a user bug), these calls could have have happened on other CPUs.
So is we are freeing a cache on CPU10, it is not possible that CPU0
frees an object from this cache right now/concurrently, the objects
were already freed but they still can be sitting in per-cpu quarantine
caches.
Now a free on an unrelated object on CPU0 can decide to drain CPU
cache concurrently, it grabs whole CPU cache, unlocks the cache
spinlock (with your patch), and now proceeds to pushing the batch to
global quarantine list. At this point CPU10 drains quarantine to evict
all objects related to the cache, but it misses the batch that CPU0
transfers from cpu cache to global quarantine, because at this point
it's referenced from just local stack variable temp in quarantine_put.
Wrapping the whole transfer sequence with irq disable/enable, ensures
that the transfer happens atomically wrt quarantine_remove_cache. That
is, that quarantine_remove_cache will see the object either in cpu
cache or in global quarantine, but not somewhere in the flight.


>> > But this is debug code anyway, right? And it is highly complex imho.
>> > Well, maybe only for me after I looked at it for the first time…
>>
>> It is debug code - yes.
>> Nothing about its performance matters - no.
>>
>> That's the way to produce unusable debug tools.
>> With too much overhead timeouts start to fire and code behaves not the
>> way it behaves in production.
>> The tool is used in continuous integration and developers wait for
>> test results before merging code.
>> The tool is used on canary devices and directly contributes to usage experience.
>
> Completely understood. What I meant is that debug code in general (from
> RT perspective) increases latency to a level where the device can not
> operate. Take lockdep for instance. Debug facility which is required
> for RT as it spots locking problems early. It increases the latency
> (depending on the workload) by 50ms+ and can't be used in production.
> Same goes for SLUB debug and most other.
>
>> We of course don't want to trade a page of assembly code for cutting
>> few cycles here (something that could make sense for some networking
>> code maybe). But otherwise let's not introduce spinlocks on fast paths
>> just for refactoring reasons.
>
> Sure. As I said. I'm fine with patch Clark initially proposed. I assumed
> the refactoring would make things simpler and avoiding the cross-CPU
> call a good thing.
>
>> > Can you take it as-is or should I repost it with an acked-by?
>>
>> Perhaps it's the problem with the way RT kernel changes things then?
>> This is not specific to quarantine, right?
>
> We got rid of _a lot_ of local_irq_disable/save() + spin_lock() combos
> which were there for reasons which are no longer true or due to lack of
> the API. And this kasan thing is just something Clark stumbled upon
> recently. And I try to negotiate something where everyone can agree on.
>
>> Should that mutex detect
>> that IRQs are disabled and not try to schedule? If this would happen
>> in some networking code, what would we do?
>
> It is not only that it is supposed not to schedule. Assuming the "mutex"
> is not owned you could acquire it right away. No scheduling. However,
> you would record current() as the owner of the lock which is wrong and
> you get into other trouble later on. The list goes on :)
> However, networking. If there is something that breaks then it will be
> addressed. It will be forwarded upstream if this something where it
> is likely to assume that RT won't change. So networking isn't special.
>
> Should I repost Clark's patch?


I am much more comfortable with just changing the type of the lock.
What are the bad implications of using the raw spinlock? Will it help
to do something along the following lines:

// Because of ...
#if CONFIG_RT
#define quarantine_spinlock_t raw_spinlock_t
#else
#define quarantine_spinlock_t spinlock_t
#endif

?

Re: [PATCH] kasan: convert kasan/quarantine_lock to raw_spinlock

[Sebastian Andrzej Siewior]

On 2018-10-10 11:45:32 [+0200], Dmitry Vyukov wrote:
> > Should I repost Clark's patch?
> 
> 
> I am much more comfortable with just changing the type of the lock.

Yes, that is what Clark's patch does. Should I resent it?

> What are the bad implications of using the raw spinlock? Will it help
> to do something along the following lines:
> 
> // Because of ...
> #if CONFIG_RT
> #define quarantine_spinlock_t raw_spinlock_t
> #else
> #define quarantine_spinlock_t spinlock_t
> #endif

no. For !RT spinlock_t and raw_spinlock_t are the same. For RT
spinlock_t does not disable interrupts or preemption while
raw_spinlock_t does.
Therefore holding a raw_spinlock_t might increase your latency.

Sebastian

Re: [PATCH] kasan: convert kasan/quarantine_lock to raw_spinlock

[Dmitry Vyukov]

On Wed, Oct 10, 2018 at 11:53 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 2018-10-10 11:45:32 [+0200], Dmitry Vyukov wrote:
>> > Should I repost Clark's patch?
>>
>>
>> I am much more comfortable with just changing the type of the lock.
>
> Yes, that is what Clark's patch does. Should I resent it?


Yes. Clark's patch looks good to me. Probably would be useful to add a
comment as to why raw spinlock is used (otherwise somebody may
refactor it back later).


>> What are the bad implications of using the raw spinlock? Will it help
>> to do something along the following lines:
>>
>> // Because of ...
>> #if CONFIG_RT
>> #define quarantine_spinlock_t raw_spinlock_t
>> #else
>> #define quarantine_spinlock_t spinlock_t
>> #endif
>
> no. For !RT spinlock_t and raw_spinlock_t are the same. For RT
> spinlock_t does not disable interrupts or preemption while
> raw_spinlock_t does.
> Therefore holding a raw_spinlock_t might increase your latency.

Ack.

[PATCH] mm/kasan: make quarantine_lock a raw_spinlock_t

[Sebastian Andrzej Siewior]

From: Clark Williams <williams@redhat.com>
Date: Tue, 18 Sep 2018 10:29:31 -0500

The static lock quarantine_lock is used in quarantine.c to protect the
quarantine queue datastructures. It is taken inside quarantine queue
manipulation routines (quarantine_put(), quarantine_reduce() and
quarantine_remove_cache()), with IRQs disabled.
This is not a problem on a stock kernel but is problematic on an RT
kernel where spin locks are sleeping spinlocks, which can sleep and can
not be acquired with disabled interrupts.

Convert the quarantine_lock to a raw spinlock_t. The usage of
quarantine_lock is confined to quarantine.c and the work performed while
the lock is held is used for debug purpose.

Signed-off-by: Clark Williams <williams@redhat.com>
Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
[bigeasy: slightly altered the commit message]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
On 2018-10-10 11:57:41 [+0200], Dmitry Vyukov wrote:
> Yes. Clark's patch looks good to me. Probably would be useful to add a
> comment as to why raw spinlock is used (otherwise somebody may
> refactor it back later).

If you really insist, I could add something but this didn't happen so
far. git's changelog should provide enough information why to why it was
changed.

 mm/kasan/quarantine.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -103,7 +103,7 @@ static int quarantine_head;
 static int quarantine_tail;
 /* Total size of all objects in global_quarantine across all batches. */
 static unsigned long quarantine_size;
-static DEFINE_SPINLOCK(quarantine_lock);
+static DEFINE_RAW_SPINLOCK(quarantine_lock);
 DEFINE_STATIC_SRCU(remove_cache_srcu);
 
 /* Maximum size of the global queue. */
@@ -190,7 +190,7 @@ void quarantine_put(struct kasan_free_me
 	if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
 		qlist_move_all(q, &temp);
 
-		spin_lock(&quarantine_lock);
+		raw_spin_lock(&quarantine_lock);
 		WRITE_ONCE(quarantine_size, quarantine_size + temp.bytes);
 		qlist_move_all(&temp, &global_quarantine[quarantine_tail]);
 		if (global_quarantine[quarantine_tail].bytes >=
@@ -203,7 +203,7 @@ void quarantine_put(struct kasan_free_me
 			if (new_tail != quarantine_head)
 				quarantine_tail = new_tail;
 		}
-		spin_unlock(&quarantine_lock);
+		raw_spin_unlock(&quarantine_lock);
 	}
 
 	local_irq_restore(flags);
@@ -230,7 +230,7 @@ void quarantine_reduce(void)
 	 * expected case).
 	 */
 	srcu_idx = srcu_read_lock(&remove_cache_srcu);
-	spin_lock_irqsave(&quarantine_lock, flags);
+	raw_spin_lock_irqsave(&quarantine_lock, flags);
 
 	/*
 	 * Update quarantine size in case of hotplug. Allocate a fraction of
@@ -254,7 +254,7 @@ void quarantine_reduce(void)
 			quarantine_head = 0;
 	}
 
-	spin_unlock_irqrestore(&quarantine_lock, flags);
+	raw_spin_unlock_irqrestore(&quarantine_lock, flags);
 
 	qlist_free_all(&to_free, NULL);
 	srcu_read_unlock(&remove_cache_srcu, srcu_idx);
@@ -310,17 +310,17 @@ void quarantine_remove_cache(struct kmem
 	 */
 	on_each_cpu(per_cpu_remove_cache, cache, 1);
 
-	spin_lock_irqsave(&quarantine_lock, flags);
+	raw_spin_lock_irqsave(&quarantine_lock, flags);
 	for (i = 0; i < QUARANTINE_BATCHES; i++) {
 		if (qlist_empty(&global_quarantine[i]))
 			continue;
 		qlist_move_cache(&global_quarantine[i], &to_free, cache);
 		/* Scanning whole quarantine can take a while. */
-		spin_unlock_irqrestore(&quarantine_lock, flags);
+		raw_spin_unlock_irqrestore(&quarantine_lock, flags);
 		cond_resched();
-		spin_lock_irqsave(&quarantine_lock, flags);
+		raw_spin_lock_irqsave(&quarantine_lock, flags);
 	}
-	spin_unlock_irqrestore(&quarantine_lock, flags);
+	raw_spin_unlock_irqrestore(&quarantine_lock, flags);
 
 	qlist_free_all(&to_free, cache);
 

Re: [PATCH] mm/kasan: make quarantine_lock a raw_spinlock_t

[Dmitry Vyukov]

On Wed, Oct 10, 2018 at 11:49 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> From: Clark Williams <williams@redhat.com>
> Date: Tue, 18 Sep 2018 10:29:31 -0500
>
> The static lock quarantine_lock is used in quarantine.c to protect the
> quarantine queue datastructures. It is taken inside quarantine queue
> manipulation routines (quarantine_put(), quarantine_reduce() and
> quarantine_remove_cache()), with IRQs disabled.
> This is not a problem on a stock kernel but is problematic on an RT
> kernel where spin locks are sleeping spinlocks, which can sleep and can
> not be acquired with disabled interrupts.
>
> Convert the quarantine_lock to a raw spinlock_t. The usage of
> quarantine_lock is confined to quarantine.c and the work performed while
> the lock is held is used for debug purpose.
>
> Signed-off-by: Clark Williams <williams@redhat.com>
> Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> [bigeasy: slightly altered the commit message]
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Dmitry Vyukov <dvyukov@google.com>

> ---
> On 2018-10-10 11:57:41 [+0200], Dmitry Vyukov wrote:
>> Yes. Clark's patch looks good to me. Probably would be useful to add a
>> comment as to why raw spinlock is used (otherwise somebody may
>> refactor it back later).
>
> If you really insist, I could add something but this didn't happen so
> far. git's changelog should provide enough information why to why it was
> changed.
>
>  mm/kasan/quarantine.c |   18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -103,7 +103,7 @@ static int quarantine_head;
>  static int quarantine_tail;
>  /* Total size of all objects in global_quarantine across all batches. */
>  static unsigned long quarantine_size;
> -static DEFINE_SPINLOCK(quarantine_lock);
> +static DEFINE_RAW_SPINLOCK(quarantine_lock);
>  DEFINE_STATIC_SRCU(remove_cache_srcu);
>
>  /* Maximum size of the global queue. */
> @@ -190,7 +190,7 @@ void quarantine_put(struct kasan_free_me
>         if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
>                 qlist_move_all(q, &temp);
>
> -               spin_lock(&quarantine_lock);
> +               raw_spin_lock(&quarantine_lock);
>                 WRITE_ONCE(quarantine_size, quarantine_size + temp.bytes);
>                 qlist_move_all(&temp, &global_quarantine[quarantine_tail]);
>                 if (global_quarantine[quarantine_tail].bytes >=
> @@ -203,7 +203,7 @@ void quarantine_put(struct kasan_free_me
>                         if (new_tail != quarantine_head)
>                                 quarantine_tail = new_tail;
>                 }
> -               spin_unlock(&quarantine_lock);
> +               raw_spin_unlock(&quarantine_lock);
>         }
>
>         local_irq_restore(flags);
> @@ -230,7 +230,7 @@ void quarantine_reduce(void)
>          * expected case).
>          */
>         srcu_idx = srcu_read_lock(&remove_cache_srcu);
> -       spin_lock_irqsave(&quarantine_lock, flags);
> +       raw_spin_lock_irqsave(&quarantine_lock, flags);
>
>         /*
>          * Update quarantine size in case of hotplug. Allocate a fraction of
> @@ -254,7 +254,7 @@ void quarantine_reduce(void)
>                         quarantine_head = 0;
>         }
>
> -       spin_unlock_irqrestore(&quarantine_lock, flags);
> +       raw_spin_unlock_irqrestore(&quarantine_lock, flags);
>
>         qlist_free_all(&to_free, NULL);
>         srcu_read_unlock(&remove_cache_srcu, srcu_idx);
> @@ -310,17 +310,17 @@ void quarantine_remove_cache(struct kmem
>          */
>         on_each_cpu(per_cpu_remove_cache, cache, 1);
>
> -       spin_lock_irqsave(&quarantine_lock, flags);
> +       raw_spin_lock_irqsave(&quarantine_lock, flags);
>         for (i = 0; i < QUARANTINE_BATCHES; i++) {
>                 if (qlist_empty(&global_quarantine[i]))
>                         continue;
>                 qlist_move_cache(&global_quarantine[i], &to_free, cache);
>                 /* Scanning whole quarantine can take a while. */
> -               spin_unlock_irqrestore(&quarantine_lock, flags);
> +               raw_spin_unlock_irqrestore(&quarantine_lock, flags);
>                 cond_resched();
> -               spin_lock_irqsave(&quarantine_lock, flags);
> +               raw_spin_lock_irqsave(&quarantine_lock, flags);
>         }
> -       spin_unlock_irqrestore(&quarantine_lock, flags);
> +       raw_spin_unlock_irqrestore(&quarantine_lock, flags);
>
>         qlist_free_all(&to_free, cache);
>

Re: [PATCH] mm/kasan: make quarantine_lock a raw_spinlock_t

[Andrew Morton]

On Wed, 10 Oct 2018 23:49:45 +0200 Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> On 2018-10-10 11:57:41 [+0200], Dmitry Vyukov wrote:
> > Yes. Clark's patch looks good to me. Probably would be useful to add a
> > comment as to why raw spinlock is used (otherwise somebody may
> > refactor it back later).
> 
> If you really insist, I could add something but this didn't happen so
> far. git's changelog should provide enough information why to why it was
> changed.

Requiring code readers to look up changelogs in git is rather user-hostile.
There are several reasons for using raw_*, so an explanatory comment at
each site is called for.

However it would be smarter to stop "using raw_* for several reasons". 
Instead, create a differently named variant for each such reason.  ie, do

/*
 * Nice comment goes here.  It explains all the possible reasons why -rt
 * might use a raw_spin_lock when a spin_lock could otherwise be used.
 */
#define raw_spin_lock_for_rt	raw_spinlock

Then use raw_spin_lock_for_rt() at all such sites.

Re: [PATCH] mm/kasan: make quarantine_lock a raw_spinlock_t

[Peter Zijlstra]

On Fri, Oct 12, 2018 at 04:56:55PM -0700, Andrew Morton wrote:
> There are several reasons for using raw_*, so an explanatory comment at
> each site is called for.
> 
> However it would be smarter to stop "using raw_* for several reasons". 
> Instead, create a differently named variant for each such reason.  ie, do
> 
> /*
>  * Nice comment goes here.  It explains all the possible reasons why -rt
>  * might use a raw_spin_lock when a spin_lock could otherwise be used.
>  */
> #define raw_spin_lock_for_rt	raw_spinlock
> 
> Then use raw_spin_lock_for_rt() at all such sites.

The whole raw_spinlock_t is for RT, no other reason. It is the one true
spinlock.

>From this, it naturally follows that:

 - nesting order: raw_spinlock_t < spinlock_t < mutex_t
 - raw_spinlock_t sections must be bounded

The patch under discussion is the result of the nesting order rule; and
is allowed to violate the second rule, by virtue of it being debug code.

There are no other reasons; and I'm somewhat confused by what you
propose.

Re: [PATCH] mm/kasan: make quarantine_lock a raw_spinlock_t

[Andrew Morton]

On Sat, 13 Oct 2018 15:50:58 +0200 Peter Zijlstra <peterz@infradead.org> wrote:

> The whole raw_spinlock_t is for RT, no other reason.

Oh.  I never realised that.

Is this documented anywhere?  Do there exist guidelines which tell
non-rt developers and reviewers when it should be used?

Re: [PATCH] mm/kasan: make quarantine_lock a raw_spinlock_t

[Peter Zijlstra]

On Mon, Oct 15, 2018 at 04:35:29PM -0700, Andrew Morton wrote:
> On Sat, 13 Oct 2018 15:50:58 +0200 Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > The whole raw_spinlock_t is for RT, no other reason.
> 
> Oh.  I never realised that.
> 
> Is this documented anywhere?  Do there exist guidelines which tell
> non-rt developers and reviewers when it should be used?

I'm afraid not; I'll put it on the todo list ... I've also been working
on some lockdep validation for the lock order stuff.