| Message ID | 20181010214945.5owshc3mlrh74z4b@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/kasan: make quarantine_lock a raw_spinlock_t | expand |
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); >
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.
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.
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?
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.
--- 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);