Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 2/8] radix-tree: Use local_lock for protection
       [not found] <20200519201912.1564477-1-bigeasy@linutronix.de>
@ 2020-05-19 20:19 ` Sebastian Andrzej Siewior
  2020-05-19 20:45   ` Matthew Wilcox
  2020-05-20 10:21   ` Peter Zijlstra
  0 siblings, 2 replies; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-05-19 20:19 UTC (permalink / raw)
  To: linux-kernel
  Cc: Peter Zijlstra, Ingo Molnar, Steven Rostedt, Will Deacon,
	Thomas Gleixner, Paul E . McKenney, Linus Torvalds,
	Sebastian Andrzej Siewior, Matthew Wilcox, linux-fsdevel

The radix-tree and idr preload mechanisms use preempt_disable() to protect
the complete operation between xxx_preload() and xxx_preload_end().

As the code inside the preempt disabled section acquires regular spinlocks,
which are converted to 'sleeping' spinlocks on a PREEMPT_RT kernel and
eventually calls into a memory allocator, this conflicts with the RT
semantics.

Convert it to a local_lock which allows RT kernels to substitute them with
a real per CPU lock. On non RT kernels this maps to preempt_disable() as
before, but provides also lockdep coverage of the critical region.
No functional change.

Cc: Matthew Wilcox <willy@infradead.org>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/idr.h        |  5 +----
 include/linux/radix-tree.h |  6 +-----
 lib/radix-tree.c           | 25 +++++++++++++++++++------
 3 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/include/linux/idr.h b/include/linux/idr.h
index ac6e946b6767b..839da8f2f6f13 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -169,10 +169,7 @@ static inline bool idr_is_empty(const struct idr *idr)
  * Each idr_preload() should be matched with an invocation of this
  * function.  See idr_preload() for details.
  */
-static inline void idr_preload_end(void)
-{
-	preempt_enable();
-}
+void idr_preload_end(void);
 
 /**
  * idr_for_each_entry() - Iterate over an IDR's elements of a given type.
diff --git a/include/linux/radix-tree.h b/include/linux/radix-tree.h
index 63e62372443a5..040b1fd0ab940 100644
--- a/include/linux/radix-tree.h
+++ b/include/linux/radix-tree.h
@@ -226,6 +226,7 @@ unsigned int radix_tree_gang_lookup(const struct radix_tree_root *,
 			unsigned int max_items);
 int radix_tree_preload(gfp_t gfp_mask);
 int radix_tree_maybe_preload(gfp_t gfp_mask);
+void radix_tree_preload_end(void);
 void radix_tree_init(void);
 void *radix_tree_tag_set(struct radix_tree_root *,
 			unsigned long index, unsigned int tag);
@@ -243,11 +244,6 @@ unsigned int radix_tree_gang_lookup_tag_slot(const struct radix_tree_root *,
 		unsigned int max_items, unsigned int tag);
 int radix_tree_tagged(const struct radix_tree_root *, unsigned int tag);
 
-static inline void radix_tree_preload_end(void)
-{
-	preempt_enable();
-}
-
 void __rcu **idr_get_free(struct radix_tree_root *root,
 			      struct radix_tree_iter *iter, gfp_t gfp,
 			      unsigned long max);
diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index 2ee6ae3b0ade0..8a44f7b85dfdc 100644
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -20,6 +20,7 @@
 #include <linux/kernel.h>
 #include <linux/kmemleak.h>
 #include <linux/percpu.h>
+#include <linux/locallock.h>
 #include <linux/preempt.h>		/* in_interrupt() */
 #include <linux/radix-tree.h>
 #include <linux/rcupdate.h>
@@ -27,7 +28,6 @@
 #include <linux/string.h>
 #include <linux/xarray.h>
 
-
 /*
  * Radix tree node cache.
  */
@@ -64,6 +64,7 @@ struct radix_tree_preload {
 	struct radix_tree_node *nodes;
 };
 static DEFINE_PER_CPU(struct radix_tree_preload, radix_tree_preloads) = { 0, };
+static DEFINE_LOCAL_LOCK(radix_tree_preloads_lock);
 
 static inline struct radix_tree_node *entry_to_node(void *ptr)
 {
@@ -332,14 +333,14 @@ static __must_check int __radix_tree_preload(gfp_t gfp_mask, unsigned nr)
 	 */
 	gfp_mask &= ~__GFP_ACCOUNT;
 
-	preempt_disable();
+	local_lock(radix_tree_preloads_lock);
 	rtp = this_cpu_ptr(&radix_tree_preloads);
 	while (rtp->nr < nr) {
-		preempt_enable();
+		local_unlock(radix_tree_preloads_lock);
 		node = kmem_cache_alloc(radix_tree_node_cachep, gfp_mask);
 		if (node == NULL)
 			goto out;
-		preempt_disable();
+		local_lock(radix_tree_preloads_lock);
 		rtp = this_cpu_ptr(&radix_tree_preloads);
 		if (rtp->nr < nr) {
 			node->parent = rtp->nodes;
@@ -381,11 +382,17 @@ int radix_tree_maybe_preload(gfp_t gfp_mask)
 	if (gfpflags_allow_blocking(gfp_mask))
 		return __radix_tree_preload(gfp_mask, RADIX_TREE_PRELOAD_SIZE);
 	/* Preloading doesn't help anything with this gfp mask, skip it */
-	preempt_disable();
+	local_lock(radix_tree_preloads_lock);
 	return 0;
 }
 EXPORT_SYMBOL(radix_tree_maybe_preload);
 
+void radix_tree_preload_end(void)
+{
+	local_unlock(radix_tree_preloads_lock);
+}
+EXPORT_SYMBOL(radix_tree_preload_end);
+
 static unsigned radix_tree_load_root(const struct radix_tree_root *root,
 		struct radix_tree_node **nodep, unsigned long *maxindex)
 {
@@ -1470,10 +1477,16 @@ EXPORT_SYMBOL(radix_tree_tagged);
 void idr_preload(gfp_t gfp_mask)
 {
 	if (__radix_tree_preload(gfp_mask, IDR_PRELOAD_SIZE))
-		preempt_disable();
+		local_lock(radix_tree_preloads_lock);
 }
 EXPORT_SYMBOL(idr_preload);
 
+void idr_preload_end(void)
+{
+	local_unlock(radix_tree_preloads_lock);
+}
+EXPORT_SYMBOL(idr_preload_end);
+
 void __rcu **idr_get_free(struct radix_tree_root *root,
 			      struct radix_tree_iter *iter, gfp_t gfp,
 			      unsigned long max)
-- 
2.26.2


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/8] radix-tree: Use local_lock for protection
  2020-05-19 20:19 ` [PATCH 2/8] radix-tree: Use local_lock for protection Sebastian Andrzej Siewior
@ 2020-05-19 20:45   ` Matthew Wilcox
  2020-05-19 20:54     ` Steven Rostedt
  2020-05-20 10:21   ` Peter Zijlstra
  1 sibling, 1 reply; 7+ messages in thread
From: Matthew Wilcox @ 2020-05-19 20:45 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Steven Rostedt,
	Will Deacon, Thomas Gleixner, Paul E . McKenney, Linus Torvalds,
	linux-fsdevel

On Tue, May 19, 2020 at 10:19:06PM +0200, Sebastian Andrzej Siewior wrote:
> The radix-tree and idr preload mechanisms use preempt_disable() to protect
> the complete operation between xxx_preload() and xxx_preload_end().
> 
> As the code inside the preempt disabled section acquires regular spinlocks,
> which are converted to 'sleeping' spinlocks on a PREEMPT_RT kernel and
> eventually calls into a memory allocator, this conflicts with the RT
> semantics.
> 
> Convert it to a local_lock which allows RT kernels to substitute them with
> a real per CPU lock. On non RT kernels this maps to preempt_disable() as
> before, but provides also lockdep coverage of the critical region.
> No functional change.

I don't seem to have a locallock.h in my tree.  Where can I find more
information about it?

> +++ b/lib/radix-tree.c
> @@ -20,6 +20,7 @@
>  #include <linux/kernel.h>
>  #include <linux/kmemleak.h>
>  #include <linux/percpu.h>
> +#include <linux/locallock.h>
>  #include <linux/preempt.h>		/* in_interrupt() */
>  #include <linux/radix-tree.h>
>  #include <linux/rcupdate.h>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/8] radix-tree: Use local_lock for protection
  2020-05-19 20:45   ` Matthew Wilcox
@ 2020-05-19 20:54     ` Steven Rostedt
  2020-05-20  2:05       ` Matthew Wilcox
  0 siblings, 1 reply; 7+ messages in thread
From: Steven Rostedt @ 2020-05-19 20:54 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Sebastian Andrzej Siewior, linux-kernel, Peter Zijlstra,
	Ingo Molnar, Will Deacon, Thomas Gleixner, Paul E . McKenney,
	Linus Torvalds, linux-fsdevel

On Tue, 19 May 2020 13:45:45 -0700
Matthew Wilcox <willy@infradead.org> wrote:

> On Tue, May 19, 2020 at 10:19:06PM +0200, Sebastian Andrzej Siewior wrote:
> > The radix-tree and idr preload mechanisms use preempt_disable() to protect
> > the complete operation between xxx_preload() and xxx_preload_end().
> > 
> > As the code inside the preempt disabled section acquires regular spinlocks,
> > which are converted to 'sleeping' spinlocks on a PREEMPT_RT kernel and
> > eventually calls into a memory allocator, this conflicts with the RT
> > semantics.
> > 
> > Convert it to a local_lock which allows RT kernels to substitute them with
> > a real per CPU lock. On non RT kernels this maps to preempt_disable() as
> > before, but provides also lockdep coverage of the critical region.
> > No functional change.  
> 
> I don't seem to have a locallock.h in my tree.  Where can I find more
> information about it?

PATCH 1 ;-)

 https://lore.kernel.org/r/20200519201912.1564477-1-bigeasy@linutronix.de

With lore and b4, it should now be easy to get full patch series.

-- Steve

> 
> > +++ b/lib/radix-tree.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/kmemleak.h>
> >  #include <linux/percpu.h>
> > +#include <linux/locallock.h>
> >  #include <linux/preempt.h>		/* in_interrupt() */
> >  #include <linux/radix-tree.h>
> >  #include <linux/rcupdate.h>  


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/8] radix-tree: Use local_lock for protection
  2020-05-19 20:54     ` Steven Rostedt
@ 2020-05-20  2:05       ` Matthew Wilcox
  2020-05-20 10:13         ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 7+ messages in thread
From: Matthew Wilcox @ 2020-05-20  2:05 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Sebastian Andrzej Siewior, linux-kernel, Peter Zijlstra,
	Ingo Molnar, Will Deacon, Thomas Gleixner, Paul E . McKenney,
	Linus Torvalds, linux-fsdevel

On Tue, May 19, 2020 at 04:54:53PM -0400, Steven Rostedt wrote:
> On Tue, 19 May 2020 13:45:45 -0700
> Matthew Wilcox <willy@infradead.org> wrote:
> 
> > On Tue, May 19, 2020 at 10:19:06PM +0200, Sebastian Andrzej Siewior wrote:
> > > The radix-tree and idr preload mechanisms use preempt_disable() to protect
> > > the complete operation between xxx_preload() and xxx_preload_end().
> > > 
> > > As the code inside the preempt disabled section acquires regular spinlocks,
> > > which are converted to 'sleeping' spinlocks on a PREEMPT_RT kernel and
> > > eventually calls into a memory allocator, this conflicts with the RT
> > > semantics.
> > > 
> > > Convert it to a local_lock which allows RT kernels to substitute them with
> > > a real per CPU lock. On non RT kernels this maps to preempt_disable() as
> > > before, but provides also lockdep coverage of the critical region.
> > > No functional change.  
> > 
> > I don't seem to have a locallock.h in my tree.  Where can I find more
> > information about it?
> 
> PATCH 1 ;-)

... this is why we have the convention to cc everybody on all the patches.

>  https://lore.kernel.org/r/20200519201912.1564477-1-bigeasy@linutronix.de
> 
> With lore and b4, it should now be easy to get full patch series.

Thats asking too much of the random people cc'd on random patches.
What is b4 anyway?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/8] radix-tree: Use local_lock for protection
  2020-05-20  2:05       ` Matthew Wilcox
@ 2020-05-20 10:13         ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-05-20 10:13 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Steven Rostedt, linux-kernel, Peter Zijlstra, Ingo Molnar,
	Will Deacon, Thomas Gleixner, Paul E . McKenney, Linus Torvalds,
	linux-fsdevel

On 2020-05-19 19:05:16 [-0700], Matthew Wilcox wrote:
> >  https://lore.kernel.org/r/20200519201912.1564477-1-bigeasy@linutronix.de
> > 
> > With lore and b4, it should now be easy to get full patch series.
> 
> Thats asking too much of the random people cc'd on random patches.

Well, other complain that they don't care about the other 20 patches
just because one patch affects them. And they can look it up if needed
so you can't make everyone happy.

> What is b4 anyway?

  git://git.kernel.org/pub/scm/utils/b4/b4.git

It is a tool written by Konstantin which can grab a whole series giving
the message-id of one patch in series, save the series as mbox, patch
series and collect all the tags (like replies with acked/tested/…-by)
and fold those tags it into the right patches.

Sebastian

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/8] radix-tree: Use local_lock for protection
  2020-05-19 20:19 ` [PATCH 2/8] radix-tree: Use local_lock for protection Sebastian Andrzej Siewior
  2020-05-19 20:45   ` Matthew Wilcox
@ 2020-05-20 10:21   ` Peter Zijlstra
  2020-05-20 12:28     ` Thomas Gleixner
  1 sibling, 1 reply; 7+ messages in thread
From: Peter Zijlstra @ 2020-05-20 10:21 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-kernel, Ingo Molnar, Steven Rostedt, Will Deacon,
	Thomas Gleixner, Paul E . McKenney, Linus Torvalds,
	Matthew Wilcox, linux-fsdevel

On Tue, May 19, 2020 at 10:19:06PM +0200, Sebastian Andrzej Siewior wrote:
> @@ -64,6 +64,7 @@ struct radix_tree_preload {
>  	struct radix_tree_node *nodes;
>  };
>  static DEFINE_PER_CPU(struct radix_tree_preload, radix_tree_preloads) = { 0, };
> +static DEFINE_LOCAL_LOCK(radix_tree_preloads_lock);
>  
>  static inline struct radix_tree_node *entry_to_node(void *ptr)
>  {

So I'm all with Andrew on the naming and pass-by-pointer thing, but
also, the above is pretty crap. You want the local_lock to be in the
structure you're actually protecting, and there really isn't anything
stopping you from doing that.

The below builds just fine and is ever so much more sensible.

--- a/include/linux/locallock_internal.h
+++ b/include/linux/locallock_internal.h
@@ -19,7 +19,7 @@ struct local_lock {
 # define LL_DEP_MAP_INIT(lockname)
 #endif
 
-#define INIT_LOCAL_LOCK(lockname)	LL_DEP_MAP_INIT(lockname)
+#define INIT_LOCAL_LOCK(lockname)	{ LL_DEP_MAP_INIT(lockname) }
 
 #define local_lock_lockdep_init(lock)				\
 do {								\
@@ -63,35 +63,35 @@ static inline void local_lock_release(st
 #define __local_lock(lock)					\
 	do {							\
 		preempt_disable();				\
-		local_lock_acquire(this_cpu_ptr(&(lock)));	\
+		local_lock_acquire(this_cpu_ptr(lock));		\
 	} while (0)
 
 #define __local_lock_irq(lock)					\
 	do {							\
 		local_irq_disable();				\
-		local_lock_acquire(this_cpu_ptr(&(lock)));	\
+		local_lock_acquire(this_cpu_ptr(lock));		\
 	} while (0)
 
 #define __local_lock_irqsave(lock, flags)			\
 	do {							\
 		local_irq_save(flags);				\
-		local_lock_acquire(this_cpu_ptr(&(lock)));	\
+		local_lock_acquire(this_cpu_ptr(lock));		\
 	} while (0)
 
 #define __local_unlock(lock)					\
 	do {							\
-		local_lock_release(this_cpu_ptr(&lock));	\
+		local_lock_release(this_cpu_ptr(lock));		\
 		preempt_enable();				\
 	} while (0)
 
 #define __local_unlock_irq(lock)				\
 	do {							\
-		local_lock_release(this_cpu_ptr(&lock));	\
+		local_lock_release(this_cpu_ptr(lock));		\
 		local_irq_enable();				\
 	} while (0)
 
 #define __local_unlock_irqrestore(lock, flags)			\
 	do {							\
-		local_lock_release(this_cpu_ptr(&lock));	\
+		local_lock_release(this_cpu_ptr(lock));		\
 		local_irq_restore(flags);			\
 	} while (0)
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -59,12 +59,14 @@ struct kmem_cache *radix_tree_node_cache
  * Per-cpu pool of preloaded nodes
  */
 struct radix_tree_preload {
+	struct local_lock lock;
 	unsigned nr;
 	/* nodes->parent points to next preallocated node */
 	struct radix_tree_node *nodes;
 };
-static DEFINE_PER_CPU(struct radix_tree_preload, radix_tree_preloads) = { 0, };
-static DEFINE_LOCAL_LOCK(radix_tree_preloads_lock);
+static DEFINE_PER_CPU(struct radix_tree_preload, radix_tree_preloads) =
+	{ .lock = INIT_LOCAL_LOCK(lock),
+	  .nr = 0, };
 
 static inline struct radix_tree_node *entry_to_node(void *ptr)
 {
@@ -333,14 +335,14 @@ static __must_check int __radix_tree_pre
 	 */
 	gfp_mask &= ~__GFP_ACCOUNT;
 
-	local_lock(radix_tree_preloads_lock);
+	local_lock(&radix_tree_preloads.lock);
 	rtp = this_cpu_ptr(&radix_tree_preloads);
 	while (rtp->nr < nr) {
-		local_unlock(radix_tree_preloads_lock);
+		local_unlock(&radix_tree_preloads.lock);
 		node = kmem_cache_alloc(radix_tree_node_cachep, gfp_mask);
 		if (node == NULL)
 			goto out;
-		local_lock(radix_tree_preloads_lock);
+		local_lock(&radix_tree_preloads.lock);
 		rtp = this_cpu_ptr(&radix_tree_preloads);
 		if (rtp->nr < nr) {
 			node->parent = rtp->nodes;
@@ -382,14 +384,14 @@ int radix_tree_maybe_preload(gfp_t gfp_m
 	if (gfpflags_allow_blocking(gfp_mask))
 		return __radix_tree_preload(gfp_mask, RADIX_TREE_PRELOAD_SIZE);
 	/* Preloading doesn't help anything with this gfp mask, skip it */
-	local_lock(radix_tree_preloads_lock);
+	local_lock(&radix_tree_preloads.lock);
 	return 0;
 }
 EXPORT_SYMBOL(radix_tree_maybe_preload);
 
 void radix_tree_preload_end(void)
 {
-	local_unlock(radix_tree_preloads_lock);
+	local_unlock(&radix_tree_preloads.lock);
 }
 EXPORT_SYMBOL(radix_tree_preload_end);
 
@@ -1477,13 +1479,13 @@ EXPORT_SYMBOL(radix_tree_tagged);
 void idr_preload(gfp_t gfp_mask)
 {
 	if (__radix_tree_preload(gfp_mask, IDR_PRELOAD_SIZE))
-		local_lock(radix_tree_preloads_lock);
+		local_lock(&radix_tree_preloads.lock);
 }
 EXPORT_SYMBOL(idr_preload);
 
 void idr_preload_end(void)
 {
-	local_unlock(radix_tree_preloads_lock);
+	local_unlock(&radix_tree_preloads.lock);
 }
 EXPORT_SYMBOL(idr_preload_end);
 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/8] radix-tree: Use local_lock for protection
  2020-05-20 10:21   ` Peter Zijlstra
@ 2020-05-20 12:28     ` Thomas Gleixner
  0 siblings, 0 replies; 7+ messages in thread
From: Thomas Gleixner @ 2020-05-20 12:28 UTC (permalink / raw)
  To: Peter Zijlstra, Sebastian Andrzej Siewior
  Cc: linux-kernel, Ingo Molnar, Steven Rostedt, Will Deacon,
	Paul E . McKenney, Linus Torvalds, Matthew Wilcox, linux-fsdevel

Peter Zijlstra <peterz@infradead.org> writes:
> On Tue, May 19, 2020 at 10:19:06PM +0200, Sebastian Andrzej Siewior wrote:
>> @@ -64,6 +64,7 @@ struct radix_tree_preload {
>>  	struct radix_tree_node *nodes;
>>  };
>>  static DEFINE_PER_CPU(struct radix_tree_preload, radix_tree_preloads) = { 0, };
>> +static DEFINE_LOCAL_LOCK(radix_tree_preloads_lock);
>>  
>>  static inline struct radix_tree_node *entry_to_node(void *ptr)
>>  {
>
> So I'm all with Andrew on the naming and pass-by-pointer thing, but
> also, the above is pretty crap. You want the local_lock to be in the
> structure you're actually protecting, and there really isn't anything
> stopping you from doing that.
>
> The below builds just fine and is ever so much more sensible.

Right you are. It's pretty obvious now that you hit me over the head
with it.

Note to self: Remove the brown paperbag _before_ touching code.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-05-20 12:28 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200519201912.1564477-1-bigeasy@linutronix.de>
2020-05-19 20:19 ` [PATCH 2/8] radix-tree: Use local_lock for protection Sebastian Andrzej Siewior
2020-05-19 20:45   ` Matthew Wilcox
2020-05-19 20:54     ` Steven Rostedt
2020-05-20  2:05       ` Matthew Wilcox
2020-05-20 10:13         ` Sebastian Andrzej Siewior
2020-05-20 10:21   ` Peter Zijlstra
2020-05-20 12:28     ` Thomas Gleixner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).