From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt0-f198.google.com (mail-qt0-f198.google.com [209.85.216.198]) by kanga.kvack.org (Postfix) with ESMTP id AA6346B0038 for ; Thu, 21 Dec 2017 20:42:02 -0500 (EST) Received: by mail-qt0-f198.google.com with SMTP id 18so20471550qtt.10 for ; Thu, 21 Dec 2017 17:42:02 -0800 (PST) Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com. [148.163.158.5]) by mx.google.com with ESMTPS id z3si1141734qti.272.2017.12.21.17.42.01 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 21 Dec 2017 17:42:01 -0800 (PST) Received: from pps.filterd (m0098416.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id vBM1Xijq154580 for ; Thu, 21 Dec 2017 20:42:01 -0500 Received: from e18.ny.us.ibm.com (e18.ny.us.ibm.com [129.33.205.208]) by mx0b-001b2d01.pphosted.com with ESMTP id 2f0qqnt939-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 21 Dec 2017 20:42:00 -0500 Received: from localhost by e18.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 21 Dec 2017 20:42:00 -0500 Date: Thu, 21 Dec 2017 17:42:12 -0800 From: "Paul E. McKenney" Subject: Re: [PATCH v2 1/1] Move kfree_call_rcu() to slab_common.c Reply-To: paulmck@linux.vnet.ibm.com References: <1513895570-28640-1-git-send-email-rao.shoaib@oracle.com> <20171222011704.GD1044@tardis> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171222011704.GD1044@tardis> Message-Id: <20171222014212.GB7829@linux.vnet.ibm.com> Sender: owner-linux-mm@kvack.org List-ID: To: Boqun Feng Cc: rao.shoaib@oracle.com, brouer@redhat.com, linux-mm@kvack.org 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@oracle.com wrote: > > From: Rao Shoaib > > > > 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. ;-) > > V2: Addresses the noise generated by checkpatch > > > > Signed-off-by: Rao Shoaib > > --- > > 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. > > /* > > * 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@kvack.org. For more info on Linux MM, > > see: http://www.linux-mm.org/ . > > Don't email: email@kvack.org > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org