[PATCH v2 1/1] Move kfree_call_rcu() to slab_common.c

[PATCH v2 1/1] Move kfree_call_rcu() to slab_common.c

[rao.shoaib]

From: Rao Shoaib <rao.shoaib@oracle.com>

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.

V2: Addresses the noise generated by checkpatch

Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com>
---
 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.
+ *
+ * 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)
 /*
  * 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);
+}
+
 /**
  * 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/1] Move kfree_call_rcu() to slab_common.c

[Boqun Feng]

[-- Attachment #1: Type: text/plain, Size: 10002 bytes --]

Hi Shoaib,

On Thu, Dec 21, 2017 at 02:32:50PM -0800, rao.shoaib@oracle.com wrote:
> From: Rao Shoaib <rao.shoaib@oracle.com>
> 
> 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?

> V2: Addresses the noise generated by checkpatch
> 
> Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com>
> ---
>  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?

>  /*
>   * 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?

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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/1] Move kfree_call_rcu() to slab_common.c

[Paul E. McKenney]

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 <rao.shoaib@oracle.com>
> > 
> > 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 <rao.shoaib@oracle.com>
> > ---
> >  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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

> 

--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/1] Move kfree_call_rcu() to slab_common.c

[Boqun Feng]

[-- Attachment #1: Type: text/plain, Size: 11910 bytes --]

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@oracle.com wrote:
> > > From: Rao Shoaib <rao.shoaib@oracle.com>
> > > 
> > > 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 ;-)

> > > V2: Addresses the noise generated by checkpatch
> > > 
> > > Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com>
> > > ---
> > >  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?

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@kvack.org.  For more info on Linux MM,
> > > see: http://www.linux-mm.org/ .
> > > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 
> > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/1] Move kfree_call_rcu() to slab_common.c

[Boqun Feng]

[-- Attachment #1: Type: text/plain, Size: 1250 bytes --]

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@oracle.com wrote:
> > > > From: Rao Shoaib <rao.shoaib@oracle.com>
> > > > 
> > > > 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 ;-)
> 

Hmm.. seems like this is first part of the split version for:

	https://lkml.kernel.org/r/1513705948-31072-1-git-send-email-rao.shoaib@oracle.com

In that case, Shoaib, I think you'd better send the whole thing as a
proper patchset(which is a set of patches) and write a cover letter.

Feel free to ask questions ;-)

Regards,
Boqun

[...]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/1] Move kfree_call_rcu() to slab_common.c

[Paul E. McKenney]

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@oracle.com wrote:
> > > > From: Rao Shoaib <rao.shoaib@oracle.com>
> > > > 
> > > > 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@oracle.com>
> > > > ---
> > > >  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@kvack.org.  For more info on Linux MM,
> > > > see: http://www.linux-mm.org/ .
> > > > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> > 
> > > 
> > 


--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>