| Message ID | 20200524215739.551568-3-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
* Sebastian Andrzej Siewior <bigeasy@linutronix.de> 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. > > 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 | 29 ++++++++++++++++++++++------- > 3 files changed, 24 insertions(+), 16 deletions(-) > -static inline void idr_preload_end(void) > -{ > - preempt_enable(); > -} > +void idr_preload_end(void); > +void idr_preload_end(void) > +{ > + local_unlock(&radix_tree_preloads.lock); > +} > +EXPORT_SYMBOL(idr_preload_end); > +void radix_tree_preload_end(void); > -static inline void radix_tree_preload_end(void) > -{ > - preempt_enable(); > -} > +void radix_tree_preload_end(void) > +{ > + local_unlock(&radix_tree_preloads.lock); > +} > +EXPORT_SYMBOL(radix_tree_preload_end); Since upstream we are still mapping the local_lock primitives to preempt_disable()/preempt_enable(), I believe these uninlining changes should not be done in this patch, i.e. idr_preload_end() and radix_tree_preload_end() should stay inline. Thanks, Ingo
On Mon, May 25, 2020 at 08:29:54AM +0200, Ingo Molnar wrote: > > +void radix_tree_preload_end(void) > > +{ > > + local_unlock(&radix_tree_preloads.lock); > > +} > > +EXPORT_SYMBOL(radix_tree_preload_end); > > Since upstream we are still mapping the local_lock primitives to > preempt_disable()/preempt_enable(), I believe these uninlining changes should not be done > in this patch, i.e. idr_preload_end() and radix_tree_preload_end() should stay inline. But radix_tree_preloads is static, and I wouldn't be terribly happy to see that exported to modules.
On 2020-05-25 08:29:54 [+0200], Ingo Molnar wrote: > Since upstream we are still mapping the local_lock primitives to > preempt_disable()/preempt_enable(), I believe these uninlining changes should not be done > in this patch, i.e. idr_preload_end() and radix_tree_preload_end() should stay inline. That means we need to export the per-CPU struct radix_tree_preload in order to access the ::lock from an inline function. Something like this then: diff --git a/include/linux/idr.h b/include/linux/idr.h index ac6e946b6767b..3ade03e5c7af3 100644 --- a/include/linux/idr.h +++ b/include/linux/idr.h @@ -171,7 +171,7 @@ static inline bool idr_is_empty(const struct idr *idr) */ static inline void idr_preload_end(void) { - preempt_enable(); + local_unlock(&radix_tree_preloads.lock); } /** diff --git a/include/linux/radix-tree.h b/include/linux/radix-tree.h index 63e62372443a5..1dcc43ac75aed 100644 --- a/include/linux/radix-tree.h +++ b/include/linux/radix-tree.h @@ -16,11 +16,20 @@ #include <linux/spinlock.h> #include <linux/types.h> #include <linux/xarray.h> +#include <linux/locallock.h> /* Keep unconverted code working */ #define radix_tree_root xarray #define radix_tree_node xa_node +struct radix_tree_preload { + struct local_lock lock; + unsigned nr; + /* nodes->parent points to next preallocated node */ + struct radix_tree_node *nodes; +}; +DECLARE_PER_CPU(struct radix_tree_preload, radix_tree_preloads); + /* * The bottom two bits of the slot determine how the remaining bits in the * slot are interpreted: @@ -245,7 +254,7 @@ int radix_tree_tagged(const struct radix_tree_root *, unsigned int tag); static inline void radix_tree_preload_end(void) { - preempt_enable(); + local_unlock(&radix_tree_preloads.lock); } void __rcu **idr_get_free(struct radix_tree_root *root, diff --git a/lib/radix-tree.c b/lib/radix-tree.c index 2ee6ae3b0ade0..1c46840b4f1d3 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. */ @@ -58,12 +58,10 @@ struct kmem_cache *radix_tree_node_cachep; /* * Per-cpu pool of preloaded nodes */ -struct radix_tree_preload { - unsigned nr; - /* nodes->parent points to next preallocated node */ - struct radix_tree_node *nodes; +DEFINE_PER_CPU(struct radix_tree_preload, radix_tree_preloads) = { + .lock = INIT_LOCAL_LOCK(lock), }; -static DEFINE_PER_CPU(struct radix_tree_preload, radix_tree_preloads) = { 0, }; +EXPORT_PER_CPU_SYMBOL_GPL(radix_tree_preloads); static inline struct radix_tree_node *entry_to_node(void *ptr) { @@ -332,14 +330,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,7 +379,7 @@ 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); @@ -1470,7 +1468,7 @@ 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);
* Matthew Wilcox <willy@infradead.org> wrote: > On Mon, May 25, 2020 at 08:29:54AM +0200, Ingo Molnar wrote: > > > +void radix_tree_preload_end(void) > > > +{ > > > + local_unlock(&radix_tree_preloads.lock); > > > +} > > > +EXPORT_SYMBOL(radix_tree_preload_end); > > > > Since upstream we are still mapping the local_lock primitives to > > preempt_disable()/preempt_enable(), I believe these uninlining changes should not be done > > in this patch, i.e. idr_preload_end() and radix_tree_preload_end() should stay inline. > > But radix_tree_preloads is static, and I wouldn't be terribly happy to > see that exported to modules. Well, it seems a bit silly to make radix_tree_preload_end() a standalone function, on most distro kernels that don't have CONFIG_PREEMPT=y, preempt_enable() is a NOP: 0000000000002bf0 <radix_tree_preload_end>: 2bf0: c3 retq I.e. we'd be introducing a separate function call for no good reason. Thanks, Ingo
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..609aeb900b550 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. */ @@ -59,11 +59,14 @@ struct kmem_cache *radix_tree_node_cachep; * 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_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) { @@ -332,14 +335,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 +384,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 +1479,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 | 29 ++++++++++++++++++++++------- 3 files changed, 24 insertions(+), 16 deletions(-)