| Message ID | 20200519201912.1564477-3-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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>
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>
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?
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
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);
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.
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)
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(-)