On Fri, Dec 22, 2017 at 10:38:46AM +0800, Boqun Feng wrote:
> On Thu, Dec 21, 2017 at 05:42:12PM -0800, Paul E. McKenney wrote:
> > On Fri, Dec 22, 2017 at 09:17:04AM +0800, Boqun Feng wrote:
> > > Hi Shoaib,
> > >
> > > On Thu, Dec 21, 2017 at 02:32:50PM -0800, rao.shoaib@xxxxxxxxxx wrote:
> > > > From: Rao Shoaib <rao.shoaib@xxxxxxxxxx>
> > > >
> > > > This patch moves kfree_call_rcu() and related macros out of rcu code.
> > > > A new function call_rcu_lazy() is created for calling __call_rcu() with
> > > > the lazy flag. kfree_call_rcu() in the tiny implementation remains unchanged.
> > > >
> > >
> > > Mind to explain why you want to do this in the commit log?
> >
> > I am too close to this one, so I need you guys to hash this out. ;-)
>
> I think simply improving the modularity is OK, but it's better to have
> some words in the commit log ;-)
Improved modularity is indeed the big benefit from my point of view.
> > > > V2: Addresses the noise generated by checkpatch
> > > >
> > > > Signed-off-by: Rao Shoaib <rao.shoaib@xxxxxxxxxx>
> > > > ---
> > > > include/linux/rcupdate.h | 43 +++----------------------------------------
> > > > include/linux/rcutree.h | 2 --
> > > > include/linux/slab.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > > kernel/rcu/tree.c | 24 ++++++++++--------------
> > > > mm/slab_common.c | 10 ++++++++++
> > > > 5 files changed, 67 insertions(+), 56 deletions(-)
> > > >
> > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > > index a6ddc42..23ed728 100644
> > > > --- a/include/linux/rcupdate.h
> > > > +++ b/include/linux/rcupdate.h
> > > > @@ -55,6 +55,9 @@ void call_rcu(struct rcu_head *head, rcu_callback_t func);
> > > > #define call_rcu call_rcu_sched
> > > > #endif /* #else #ifdef CONFIG_PREEMPT_RCU */
> > > >
> > > > +/* only for use by kfree_call_rcu() */
> > > > +void call_rcu_lazy(struct rcu_head *head, rcu_callback_t func);
> > > > +
> > > > void call_rcu_bh(struct rcu_head *head, rcu_callback_t func);
> > > > void call_rcu_sched(struct rcu_head *head, rcu_callback_t func);
> > > > void synchronize_sched(void);
> > > > @@ -838,45 +841,6 @@ static inline notrace void rcu_read_unlock_sched_notrace(void)
> > > > #define __is_kfree_rcu_offset(offset) ((offset) < 4096)
> > > >
> > > > /*
> > > > - * Helper macro for kfree_rcu() to prevent argument-expansion eyestrain.
> > > > - */
> > > > -#define __kfree_rcu(head, offset) \
> > > > - do { \
> > > > - BUILD_BUG_ON(!__is_kfree_rcu_offset(offset)); \
> > > > - kfree_call_rcu(head, (rcu_callback_t)(unsigned long)(offset)); \
> > > > - } while (0)
> > > > -
> > > > -/**
> > > > - * kfree_rcu() - kfree an object after a grace period.
> > > > - * @ptr: pointer to kfree
> > > > - * @rcu_head: the name of the struct rcu_head within the type of @ptr.
> > > > - *
> > > > - * Many rcu callbacks functions just call kfree() on the base structure.
> > > > - * These functions are trivial, but their size adds up, and furthermore
> > > > - * when they are used in a kernel module, that module must invoke the
> > > > - * high-latency rcu_barrier() function at module-unload time.
> > > > - *
> > > > - * The kfree_rcu() function handles this issue. Rather than encoding a
> > > > - * function address in the embedded rcu_head structure, kfree_rcu() instead
> > > > - * encodes the offset of the rcu_head structure within the base structure.
> > > > - * Because the functions are not allowed in the low-order 4096 bytes of
> > > > - * kernel virtual memory, offsets up to 4095 bytes can be accommodated.
> > > > - * If the offset is larger than 4095 bytes, a compile-time error will
> > > > - * be generated in __kfree_rcu(). If this error is triggered, you can
> > > > - * either fall back to use of call_rcu() or rearrange the structure to
> > > > - * position the rcu_head structure into the first 4096 bytes.
> > > > - *
> > > > - * Note that the allowable offset might decrease in the future, for example,
> > > > - * to allow something like kmem_cache_free_rcu().
> > > > - *
> > > > - * The BUILD_BUG_ON check must not involve any function calls, hence the
> > > > - * checks are done in macros here.
> > > > - */
> > > > -#define kfree_rcu(ptr, rcu_head) \
> > > > - __kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))
> > > > -
> > > > -
> > > > -/*
> > > > * Place this after a lock-acquisition primitive to guarantee that
> > > > * an UNLOCK+LOCK pair acts as a full barrier. This guarantee applies
> > > > * if the UNLOCK and LOCK are executed by the same CPU or if the
> > > > @@ -888,5 +852,4 @@ static inline notrace void rcu_read_unlock_sched_notrace(void)
> > > > #define smp_mb__after_unlock_lock() do { } while (0)
> > > > #endif /* #else #ifdef CONFIG_ARCH_WEAK_RELEASE_ACQUIRE */
> > > >
> > > > -
> > > > #endif /* __LINUX_RCUPDATE_H */
> > > > diff --git a/include/linux/rcutree.h b/include/linux/rcutree.h
> > > > index 37d6fd3..7746b19 100644
> > > > --- a/include/linux/rcutree.h
> > > > +++ b/include/linux/rcutree.h
> > > > @@ -48,8 +48,6 @@ void synchronize_rcu_bh(void);
> > > > void synchronize_sched_expedited(void);
> > > > void synchronize_rcu_expedited(void);
> > > >
> > > > -void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func);
> > > > -
> > > > /**
> > > > * synchronize_rcu_bh_expedited - Brute-force RCU-bh grace period
> > > > *
> > > > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > > > index 50697a1..a71f6a78 100644
> > > > --- a/include/linux/slab.h
> > > > +++ b/include/linux/slab.h
> > > > @@ -342,6 +342,50 @@ void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __malloc;
> > > > void *kmem_cache_alloc(struct kmem_cache *, gfp_t flags) __assume_slab_alignment __malloc;
> > > > void kmem_cache_free(struct kmem_cache *, void *);
> > > >
> > > > +void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func);
> > > > +
> > > > +/* Helper macro for kfree_rcu() to prevent argument-expansion eyestrain. */
> > > > +#define __kfree_rcu(head, offset) \
> > > > + do { \
> > > > + unsigned long __o = (unsigned long)offset; \
> > > > + BUILD_BUG_ON(!__is_kfree_rcu_offset(__o)); \
> > > > + kfree_call_rcu(head, (rcu_callback_t)(__o)); \
> > > > + } while (0)
> > > > +
> > > > +/**
> > > > + * kfree_rcu() - kfree an object after a grace period.
> > > > + * @ptr: pointer to kfree
> > > > + * @rcu_head: the name of the struct rcu_head within the type of @ptr.
> > >
> > > So you already rename this parameter to 'rcu_head_name', and...
> > >
> > > > + *
> > > > + * Many rcu callbacks functions just call kfree() on the base structure.
> > > > + * These functions are trivial, but their size adds up, and furthermore
> > > > + * when they are used in a kernel module, that module must invoke the
> > > > + * high-latency rcu_barrier() function at module-unload time.
> > > > + *
> > > > + * The kfree_rcu() function handles this issue. Rather than encoding a
> > > > + * function address in the embedded rcu_head structure, kfree_rcu() instead
> > > > + * encodes the offset of the rcu_head structure within the base structure.
> > > > + * Because the functions are not allowed in the low-order 4096 bytes of
> > > > + * kernel virtual memory, offsets up to 4095 bytes can be accommodated.
> > > > + * If the offset is larger than 4095 bytes, a compile-time error will
> > > > + * be generated in __kfree_rcu(). If this error is triggered, you can
> > > > + * either fall back to use of call_rcu() or rearrange the structure to
> > > > + * position the rcu_head structure into the first 4096 bytes.
> > > > + *
> > > > + * Note that the allowable offset might decrease in the future, for example,
> > > > + * to allow something like kmem_cache_free_rcu().
> > > > + *
> > > > + * The BUILD_BUG_ON check must not involve any function calls, hence the
> > > > + * checks are done in macros here.
> > > > + */
> > > > +#define kfree_rcu(ptr, rcu_head_name) \
> > > > + do { \
> > > > + typeof(ptr) __ptr = ptr; \
> > > > + unsigned long __off = offsetof(typeof(*(__ptr)), \
> > > > + rcu_head_name); \
> > > > + struct rcu_head *__rptr = (void *)__ptr + __off; \
> > > > + __kfree_rcu(__rptr, __off); \
> > > > + } while (0)
> > >
> > > why do you want to open code this?
> >
> > He needs it to be a macro so that offsetof() will work properly.
>
> You lost me on this one, but it may be me who used the wrong phrase
> "open code"..
>
> So the kfree_rcu() used to be implemented as:
>
> #define kfree_rcu(ptr, rcu_head) \
> __kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))
>
> and Shoaib expanded as above, so I thought he must have a reason to do
> this, maybe simply for the readability?
Ah, this was in response to feedback about not repeating macro parameters.
A bit over the top, perhaps, but the added typechecking alone is probably
well worth the added lines.
Thanx, Paul
> Regards,
> Boqun
>
> > > > /*
> > > > * Bulk allocation and freeing operations. These are accelerated in an
> > > > * allocator specific way to avoid taking locks repeatedly or building
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index f9c0ca2..7d2830f 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -3180,6 +3180,16 @@ void call_rcu_sched(struct rcu_head *head, rcu_callback_t func)
> > > > }
> > > > EXPORT_SYMBOL_GPL(call_rcu_sched);
> > > >
> > > > +/* Queue an RCU callback for lazy invocation after a grace period.
> > > > + * Currently there is no way of tagging the lazy RCU callbacks in the
> > > > + * list of pending callbacks. Until then, this function may only be
> > > > + * called from kfree_call_rcu().
> > > > + */
> > > > +void call_rcu_lazy(struct rcu_head *head, rcu_callback_t func)
> > > > +{
> > > > + __call_rcu(head, func, &rcu_sched_state, -1, 1);
> > >
> > > Why do you switch this from rcu_state_p to rcu_sched_state? Have you
> > > checked your changes don't break PREEMPT=y kernel? Did I miss something
> > > subtle?
> >
> > Good catch, this would be a problem on PREEMPT=y kernels. I agree with
> > Boqun's implicit suggestion of using rcu_state_p.
> >
> > Thanx, Paul
> >
> > > Regards,
> > > Boqun
> > >
> > > > +}
> > > > +
> > > > /**
> > > > * call_rcu_bh() - Queue an RCU for invocation after a quicker grace period.
> > > > * @head: structure to be used for queueing the RCU updates.
> > > > @@ -3209,20 +3219,6 @@ void call_rcu_bh(struct rcu_head *head, rcu_callback_t func)
> > > > EXPORT_SYMBOL_GPL(call_rcu_bh);
> > > >
> > > > /*
> > > > - * Queue an RCU callback for lazy invocation after a grace period.
> > > > - * This will likely be later named something like "call_rcu_lazy()",
> > > > - * but this change will require some way of tagging the lazy RCU
> > > > - * callbacks in the list of pending callbacks. Until then, this
> > > > - * function may only be called from __kfree_rcu().
> > > > - */
> > > > -void kfree_call_rcu(struct rcu_head *head,
> > > > - rcu_callback_t func)
> > > > -{
> > > > - __call_rcu(head, func, rcu_state_p, -1, 1);
> > > > -}
> > > > -EXPORT_SYMBOL_GPL(kfree_call_rcu);
> > > > -
> > > > -/*
> > > > * Because a context switch is a grace period for RCU-sched and RCU-bh,
> > > > * any blocking grace-period wait automatically implies a grace period
> > > > * if there is only one CPU online at any point time during execution
> > > > diff --git a/mm/slab_common.c b/mm/slab_common.c
> > > > index c8cb367..0d8a63b 100644
> > > > --- a/mm/slab_common.c
> > > > +++ b/mm/slab_common.c
> > > > @@ -1483,6 +1483,16 @@ void kzfree(const void *p)
> > > > }
> > > > EXPORT_SYMBOL(kzfree);
> > > >
> > > > +/*
> > > > + * Queue Memory to be freed by RCU after a grace period.
> > > > + */
> > > > +void kfree_call_rcu(struct rcu_head *head,
> > > > + rcu_callback_t func)
> > > > +{
> > > > + call_rcu_lazy(head, func);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(kfree_call_rcu);
> > > > +
> > > > /* Tracepoints definitions. */
> > > > EXPORT_TRACEPOINT_SYMBOL(kmalloc);
> > > > EXPORT_TRACEPOINT_SYMBOL(kmem_cache_alloc);
> > > > --
> > > > 2.7.4
> > > >
> > > > --
> > > > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > > > the body to majordomo@xxxxxxxxx. For more info on Linux MM,
> > > > see: http://www.linux-mm.org/ .
> > > > Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
> >
> > >
> >
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
