[PATCH v1 1/5] mm/vmalloc.c: Introduce vfree_bulk() interface

[PATCH v1 1/5] mm/vmalloc.c: Introduce vfree_bulk() interface

[Uladzislau Rezki (Sony)]

Provide vfree_bulk() function and make it public. Some users
need to vfree vmalloc ptrs. as one set of batch. For example
there is a kvfree_rcu() API that keeps pointers in array.

As of now start with a simple implementation of vmalloc-bulk
interface that can be improved step by step in the future.

To: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Suggested-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 include/linux/vmalloc.h |  1 +
 mm/vmalloc.c            | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index 3de7be6dd17c..bef1588c165b 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -112,6 +112,7 @@ void *__vmalloc_node(unsigned long size, unsigned long align, gfp_t gfp_mask,
 
 extern void vfree(const void *addr);
 extern void vfree_atomic(const void *addr);
+extern void vfree_bulk(size_t count, void **addrs);
 
 extern void *vmap(struct page **pages, unsigned int count,
 			unsigned long flags, pgprot_t prot);
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index d5f2a84e488a..a07aa336b05d 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2365,6 +2365,37 @@ void vfree(const void *addr)
 }
 EXPORT_SYMBOL(vfree);
 
+/**
+ * vfree_bulk - Release memory allocated by vmalloc()
+ * @count: Number of elements in array
+ * @addrs: Array of elements to be freed
+ *
+ * Free the virtually continuous memory area starting at @addrs[i], as obtained
+ * from one of the vmalloc() family of APIs.  This will usually also free the
+ * physical memory underlying the virtual allocation, but that memory is
+ * reference counted, so it will not be freed until the last user goes away.
+ *
+ * If @addrs[i] is NULL, no operation is performed.
+ *
+ * Context:
+ * Same as for vfree()
+ */
+void vfree_bulk(size_t count, void **addrs)
+{
+	unsigned int i;
+
+	BUG_ON(in_nmi());
+	might_sleep_if(!in_interrupt());
+
+	for (i = 0; i < count; i++) {
+		kmemleak_free(addrs[i]);
+
+		if (addrs[i])
+			__vfree(addrs[i]);
+	}
+}
+EXPORT_SYMBOL(vfree_bulk);
+
 /**
  * vunmap - release virtual mapping obtained by vmap()
  * @addr:   memory base address
-- 
2.20.1

[PATCH v1 2/5] kvfree_rcu: Switch to vfree_bulk() in kfree_rcu_work()

[Uladzislau Rezki (Sony)]

Replace calling a vfree() in a loop per object by the
vfree_bulk() variant. In that case we just need to pass
an array of vmalloc'ed pointers and number of elements
in it.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 kernel/rcu/tree.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 9ea1d4eef1ad..6bf170d01cd5 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3280,7 +3280,7 @@ static void kfree_rcu_work(struct work_struct *work)
 	struct rcu_head *head, *next;
 	struct kfree_rcu_cpu *krcp;
 	struct kfree_rcu_cpu_work *krwp;
-	int i, j;
+	int i;
 
 	krwp = container_of(to_rcu_work(work),
 			    struct kfree_rcu_cpu_work, rcu_work);
@@ -3313,13 +3313,7 @@ static void kfree_rcu_work(struct work_struct *work)
 				kfree_bulk(bkvhead[i]->nr_records,
 					bkvhead[i]->records);
 			} else { // vmalloc() / vfree().
-				for (j = 0; j < bkvhead[i]->nr_records; j++) {
-					trace_rcu_invoke_kvfree_callback(
-						rcu_state.name,
-						bkvhead[i]->records[j], 0);
-
-					vfree(bkvhead[i]->records[j]);
-				}
+				vfree_bulk(bkvhead[i]->nr_records, bkvhead[i]->records);
 			}
 			rcu_lock_release(&rcu_callback_map);
 
-- 
2.20.1

[PATCH v1 3/5] kvfree_rcu: Rename rcu_invoke_kfree_bulk_callback

[Uladzislau Rezki (Sony)]

The purpose of renaming is to have one common trace event
for APIs which use a special "bulk" form to release memory.

In our case we have kfree_bulk() and vfree_bulk(). It does
not make sense to implement two separate trace events which
would be identical expect for their names.

Rename it to "rcu_invoke_free_bulk_callback" and use it as
one trace event for both bulk API's.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 include/trace/events/rcu.h |  4 ++--
 kernel/rcu/tree.c          | 16 +++++++++-------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/include/trace/events/rcu.h b/include/trace/events/rcu.h
index 918585d85af9..0855cb35ae4e 100644
--- a/include/trace/events/rcu.h
+++ b/include/trace/events/rcu.h
@@ -681,11 +681,11 @@ TRACE_EVENT_RCU(rcu_invoke_kvfree_callback,
 
 /*
  * Tracepoint for the invocation of a single RCU callback of the special
- * kfree_bulk() form. The first argument is the RCU flavor, the second
+ * "free bulk" form. The first argument is the RCU flavor, the second
  * argument is a number of elements in array to free, the third is an
  * address of the array holding nr_records entries.
  */
-TRACE_EVENT_RCU(rcu_invoke_kfree_bulk_callback,
+TRACE_EVENT_RCU(rcu_invoke_free_bulk_callback,
 
 	TP_PROTO(const char *rcuname, unsigned long nr_records, void **p),
 
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 6bf170d01cd5..e44d6f8c56f0 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3305,16 +3305,18 @@ static void kfree_rcu_work(struct work_struct *work)
 			debug_rcu_bhead_unqueue(bkvhead[i]);
 
 			rcu_lock_acquire(&rcu_callback_map);
-			if (i == 0) { // kmalloc() / kfree().
-				trace_rcu_invoke_kfree_bulk_callback(
-					rcu_state.name, bkvhead[i]->nr_records,
-					bkvhead[i]->records);
 
+			trace_rcu_invoke_free_bulk_callback(
+				rcu_state.name, bkvhead[i]->nr_records,
+				bkvhead[i]->records);
+
+			if (i == 0) // kmalloc() / kfree().
 				kfree_bulk(bkvhead[i]->nr_records,
 					bkvhead[i]->records);
-			} else { // vmalloc() / vfree().
-				vfree_bulk(bkvhead[i]->nr_records, bkvhead[i]->records);
-			}
+			else // vmalloc() / vfree().
+				vfree_bulk(bkvhead[i]->nr_records,
+					bkvhead[i]->records);
+
 			rcu_lock_release(&rcu_callback_map);
 
 			raw_spin_lock_irqsave(&krcp->lock, flags);
-- 
2.20.1

[PATCH v1 4/5] kvfree_rcu: Refactor kfree_rcu_monitor() function

[Uladzislau Rezki (Sony)]

Rearm the monitor work directly from its own function that
is kfree_rcu_monitor(). So this patch puts the invocation
timing control in one place.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 kernel/rcu/tree.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index e44d6f8c56f0..229e909ad437 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3415,37 +3415,44 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
 	return !repeat;
 }
 
-static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp,
-					  unsigned long flags)
+/*
+ * This function queues a new batch. If success or nothing to
+ * drain it returns 1. Otherwise 0 is returned indicating that
+ * a reclaim kthread has not processed a previous batch.
+ */
+static inline int kfree_rcu_drain(struct kfree_rcu_cpu *krcp)
 {
+	unsigned long flags;
+	int ret;
+
+	raw_spin_lock_irqsave(&krcp->lock, flags);
+
 	// Attempt to start a new batch.
-	if (queue_kfree_rcu_work(krcp)) {
+	ret = queue_kfree_rcu_work(krcp);
+	if (ret)
 		// Success! Our job is done here.
 		krcp->monitor_todo = false;
-		raw_spin_unlock_irqrestore(&krcp->lock, flags);
-		return;
-	}
 
 	// Previous RCU batch still in progress, try again later.
-	schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES);
 	raw_spin_unlock_irqrestore(&krcp->lock, flags);
+	return ret;
 }
 
 /*
  * This function is invoked after the KFREE_DRAIN_JIFFIES timeout.
- * It invokes kfree_rcu_drain_unlock() to attempt to start another batch.
+ * It invokes kfree_rcu_drain() to attempt to start another batch.
  */
 static void kfree_rcu_monitor(struct work_struct *work)
 {
-	unsigned long flags;
 	struct kfree_rcu_cpu *krcp = container_of(work, struct kfree_rcu_cpu,
 						 monitor_work.work);
 
-	raw_spin_lock_irqsave(&krcp->lock, flags);
-	if (krcp->monitor_todo)
-		kfree_rcu_drain_unlock(krcp, flags);
-	else
-		raw_spin_unlock_irqrestore(&krcp->lock, flags);
+	if (kfree_rcu_drain(krcp))
+		return;
+
+	// Not success. A previous batch is still in progress.
+	// Rearm a work to repeat an attempt of starting another batch.
+	schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES);
 }
 
 static enum hrtimer_restart
-- 
2.20.1

[PATCH v1 5/5] kvfree_rcu: Fix comments according to current code

[Uladzislau Rezki (Sony)]

We used to have an "emergency path" and comments related
to it. According to current kvfree_rcu() design that path
is not considered as emergency anymore.

This patch rephrases and updates comments regarding this.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 kernel/rcu/tree.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 229e909ad437..91b978e90c3b 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3332,9 +3332,11 @@ static void kfree_rcu_work(struct work_struct *work)
 	}
 
 	/*
-	 * Emergency case only. It can happen under low memory
-	 * condition when an allocation gets failed, so the "bulk"
-	 * path can not be temporary maintained.
+	 * This is used when the "bulk" path can not be temporary
+	 * maintained for the double-argument of kvfree_rcu(). It
+	 * happens because a page-cache is empty, thus objects are
+	 * queued to the linked-list instead. This path is named
+	 * as "Channel 3".
 	 */
 	for (; head; head = next) {
 		unsigned long offset = (unsigned long)head->func;
@@ -3380,8 +3382,8 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
 		if ((krcp->bkvhead[0] && !krwp->bkvhead_free[0]) ||
 			(krcp->bkvhead[1] && !krwp->bkvhead_free[1]) ||
 				(krcp->head && !krwp->head_free)) {
-			// Channel 1 corresponds to SLAB ptrs.
-			// Channel 2 corresponds to vmalloc ptrs.
+			// Channel 1 corresponds to SLAB ptrs. bulk path.
+			// Channel 2 corresponds to vmalloc ptrs. bulk path.
 			for (j = 0; j < FREE_N_CHANNELS; j++) {
 				if (!krwp->bkvhead_free[j]) {
 					krwp->bkvhead_free[j] = krcp->bkvhead[j];
@@ -3389,7 +3391,8 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
 				}
 			}
 
-			// Channel 3 corresponds to emergency path.
+			// Channel 3 corresponds to objects queued to the
+			// linked-list for both SLAB and vmalloc pointers.
 			if (!krwp->head_free) {
 				krwp->head_free = krcp->head;
 				krcp->head = NULL;
-- 
2.20.1

Re: [PATCH v1 5/5] kvfree_rcu: Fix comments according to current code

[Paul E. McKenney]

On Wed, Apr 28, 2021 at 03:44:22PM +0200, Uladzislau Rezki (Sony) wrote:
> We used to have an "emergency path" and comments related
> to it. According to current kvfree_rcu() design that path
> is not considered as emergency anymore.
> 
> This patch rephrases and updates comments regarding this.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

Queued as follows, thank you!  Please check my usual wordsmithing to
make sure that I did not mess anything up.

							Thanx, Paul

------------------------------------------------------------------------

commit 06c2113c4b1ce9ded69cd0ac4da9a00ed6be8834
Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
Date:   Wed Apr 28 15:44:22 2021 +0200

    kvfree_rcu: Fix comments according to current code
    
    The kvfree_rcu() function now defers allocations in the common
    case due to the fact that there is no lockless access to the
    memory-allocator caches/pools.  In addition, in CONFIG_PREEMPT_NONE=y
    and in CONFIG_PREEMPT_VOLUNTARY=y kernels, there is no reliable way to
    determine if spinlocks are held.  As a result, allocation is deferred in
    the common case, and the two-argument form of kvfree_rcu() thus uses the
    "channel 3" queue through all the rcu_head structures.  This channel
    is called referred to as the emergency case in comments, and these
    comments are now obsolete.
    
    This commit therefore updates these comments to reflect the new
    common-case nature of such emergencies.
    
    Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 9089c23e80dc..4d93c6985d3f 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3338,9 +3338,11 @@ static void kfree_rcu_work(struct work_struct *work)
 	}
 
 	/*
-	 * Emergency case only. It can happen under low memory
-	 * condition when an allocation gets failed, so the "bulk"
-	 * path can not be temporary maintained.
+	 * This is used when the "bulk" path can not be used for the
+	 * double-argument of kvfree_rcu().  This happens when the
+	 * page-cache is empty, which means that objects are instead
+	 * queued on a linked list through their rcu_head structures.
+	 * This list is named "Channel 3".
 	 */
 	for (; head; head = next) {
 		unsigned long offset = (unsigned long)head->func;
@@ -3386,8 +3388,8 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
 		if ((krcp->bkvhead[0] && !krwp->bkvhead_free[0]) ||
 			(krcp->bkvhead[1] && !krwp->bkvhead_free[1]) ||
 				(krcp->head && !krwp->head_free)) {
-			// Channel 1 corresponds to SLAB ptrs.
-			// Channel 2 corresponds to vmalloc ptrs.
+			// Channel 1 corresponds to the SLAB-pointer bulk path.
+			// Channel 2 corresponds to vmalloc-pointer bulk path.
 			for (j = 0; j < FREE_N_CHANNELS; j++) {
 				if (!krwp->bkvhead_free[j]) {
 					krwp->bkvhead_free[j] = krcp->bkvhead[j];
@@ -3395,7 +3397,8 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
 				}
 			}
 
-			// Channel 3 corresponds to emergency path.
+			// Channel 3 corresponds to both SLAB and vmalloc
+			// objects queued on the linked list.
 			if (!krwp->head_free) {
 				krwp->head_free = krcp->head;
 				krcp->head = NULL;

Re: [PATCH v1 4/5] kvfree_rcu: Refactor kfree_rcu_monitor() function

[Uladzislau Rezki]

Hello, Paul.

> Rearm the monitor work directly from its own function that
> is kfree_rcu_monitor(). So this patch puts the invocation
> timing control in one place.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> ---
>  kernel/rcu/tree.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index e44d6f8c56f0..229e909ad437 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3415,37 +3415,44 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
>  	return !repeat;
>  }
>  
> -static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp,
> -					  unsigned long flags)
> +/*
> + * This function queues a new batch. If success or nothing to
> + * drain it returns 1. Otherwise 0 is returned indicating that
> + * a reclaim kthread has not processed a previous batch.
> + */
> +static inline int kfree_rcu_drain(struct kfree_rcu_cpu *krcp)
>  {
> +	unsigned long flags;
> +	int ret;
> +
> +	raw_spin_lock_irqsave(&krcp->lock, flags);
> +
>  	// Attempt to start a new batch.
> -	if (queue_kfree_rcu_work(krcp)) {
> +	ret = queue_kfree_rcu_work(krcp);
> +	if (ret)
>  		// Success! Our job is done here.
>  		krcp->monitor_todo = false;
> -		raw_spin_unlock_irqrestore(&krcp->lock, flags);
> -		return;
> -	}
>  
>  	// Previous RCU batch still in progress, try again later.
> -	schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES);
>  	raw_spin_unlock_irqrestore(&krcp->lock, flags);
> +	return ret;
>  }
>  
>  /*
>   * This function is invoked after the KFREE_DRAIN_JIFFIES timeout.
> - * It invokes kfree_rcu_drain_unlock() to attempt to start another batch.
> + * It invokes kfree_rcu_drain() to attempt to start another batch.
>   */
>  static void kfree_rcu_monitor(struct work_struct *work)
>  {
> -	unsigned long flags;
>  	struct kfree_rcu_cpu *krcp = container_of(work, struct kfree_rcu_cpu,
>  						 monitor_work.work);
>  
> -	raw_spin_lock_irqsave(&krcp->lock, flags);
> -	if (krcp->monitor_todo)
> -		kfree_rcu_drain_unlock(krcp, flags);
> -	else
> -		raw_spin_unlock_irqrestore(&krcp->lock, flags);
> +	if (kfree_rcu_drain(krcp))
> +		return;
> +
> +	// Not success. A previous batch is still in progress.
> +	// Rearm a work to repeat an attempt of starting another batch.
> +	schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES);
>  }
>  
>  static enum hrtimer_restart
> -- 
> 2.20.1
> 

Please see below a v2 of this patch. The main difference between v1
is that, the monitor work now is open-coded, thus some extra inline
functions were eliminated:

From 7d153a640a4f791cbfd9b546e32f90fb2c60c480 Mon Sep 17 00:00:00 2001
From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
Date: Wed, 21 Apr 2021 13:22:52 +0200
Subject: [PATCH v2] kvfree_rcu: Refactor kfree_rcu_monitor()

Currently we have three functions which depend on each other.
Two of them are quite tiny and the last one where the most
work is done. All of them are related to queuing RCU batches
to reclaim objects after a GP.

1. kfree_rcu_monitor(). It consist of few lines. It acquires
   the spin-lock and calls "drain" function.

2. kfree_rcu_drain_unlock(). It also consists of few lines of
   code. It calls a func. to queue the batch. If not success
   rearm the monitor work to repeat an attempt one more time.

3. queue_kfree_rcu_work(). Main core part is implemented here.
   In short, it attempts to start a new batch to free objects
   after a GP.

Since there are no external users of [2] and [3] functions we
can eliminate both by moving all logic directly into [1]. That
makes the kfree_rcu_monitor() to be open-coded what is easier
to follow thus becomes less complicated.

Apart of that, replace comments which start with "/*" to "//"
format to make it unified across the file.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 kernel/rcu/tree.c | 84 +++++++++++++++--------------------------------
 1 file changed, 26 insertions(+), 58 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index e44d6f8c56f0..d6bf2d4e6e8b 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3354,29 +3354,26 @@ static void kfree_rcu_work(struct work_struct *work)
 }
 
 /*
- * Schedule the kfree batch RCU work to run in workqueue context after a GP.
- *
- * This function is invoked by kfree_rcu_monitor() when the KFREE_DRAIN_JIFFIES
- * timeout has been reached.
+ * This function is invoked after the KFREE_DRAIN_JIFFIES timeout.
  */
-static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
+static void kfree_rcu_monitor(struct work_struct *work)
 {
-	struct kfree_rcu_cpu_work *krwp;
-	bool repeat = false;
+	struct kfree_rcu_cpu *krcp = container_of(work,
+		struct kfree_rcu_cpu, monitor_work.work);
+	unsigned long flags;
 	int i, j;
 
-	lockdep_assert_held(&krcp->lock);
+	raw_spin_lock_irqsave(&krcp->lock, flags);
 
+	// Attempt to start a new batch.
 	for (i = 0; i < KFREE_N_BATCHES; i++) {
-		krwp = &(krcp->krw_arr[i]);
+		struct kfree_rcu_cpu_work *krwp = &(krcp->krw_arr[i]);
 
-		/*
-		 * Try to detach bkvhead or head and attach it over any
-		 * available corresponding free channel. It can be that
-		 * a previous RCU batch is in progress, it means that
-		 * immediately to queue another one is not possible so
-		 * return false to tell caller to retry.
-		 */
+		// Try to detach bkvhead or head and attach it over any
+		// available corresponding free channel. It can be that
+		// a previous RCU batch is in progress, it means that
+		// immediately to queue another one is not possible so
+		// in that case the monitor work is rearmed.
 		if ((krcp->bkvhead[0] && !krwp->bkvhead_free[0]) ||
 			(krcp->bkvhead[1] && !krwp->bkvhead_free[1]) ||
 				(krcp->head && !krwp->head_free)) {
@@ -3397,57 +3394,28 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
 
 			WRITE_ONCE(krcp->count, 0);
 
-			/*
-			 * One work is per one batch, so there are three
-			 * "free channels", the batch can handle. It can
-			 * be that the work is in the pending state when
-			 * channels have been detached following by each
-			 * other.
-			 */
+			// One work is per one batch, so there are three
+			// "free channels", the batch can handle. It can
+			// be that the work is in the pending state when
+			// channels have been detached following by each
+			// other.
 			queue_rcu_work(system_wq, &krwp->rcu_work);
 		}
-
-		// Repeat if any "free" corresponding channel is still busy.
-		if (krcp->bkvhead[0] || krcp->bkvhead[1] || krcp->head)
-			repeat = true;
 	}
 
-	return !repeat;
-}
-
-static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp,
-					  unsigned long flags)
-{
-	// Attempt to start a new batch.
-	if (queue_kfree_rcu_work(krcp)) {
-		// Success! Our job is done here.
+	// If there is nothing to detach, it means that our job is
+	// successfully done here. In case of having at least one
+	// of the channels that is still busy we should rearm the
+	// work to repeat an attempt. Because previous batches are
+	// still in progress.
+	if (!krcp->bkvhead[0] && !krcp->bkvhead[1] && !krcp->head)
 		krcp->monitor_todo = false;
-		raw_spin_unlock_irqrestore(&krcp->lock, flags);
-		return;
-	}
+	else
+		schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES);
 
-	// Previous RCU batch still in progress, try again later.
-	schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES);
 	raw_spin_unlock_irqrestore(&krcp->lock, flags);
 }
 
-/*
- * This function is invoked after the KFREE_DRAIN_JIFFIES timeout.
- * It invokes kfree_rcu_drain_unlock() to attempt to start another batch.
- */
-static void kfree_rcu_monitor(struct work_struct *work)
-{
-	unsigned long flags;
-	struct kfree_rcu_cpu *krcp = container_of(work, struct kfree_rcu_cpu,
-						 monitor_work.work);
-
-	raw_spin_lock_irqsave(&krcp->lock, flags);
-	if (krcp->monitor_todo)
-		kfree_rcu_drain_unlock(krcp, flags);
-	else
-		raw_spin_unlock_irqrestore(&krcp->lock, flags);
-}
-
 static enum hrtimer_restart
 schedule_page_work_fn(struct hrtimer *t)
 {
-- 
2.20.1

--
Vlad Rezki

Re: [PATCH v1 5/5] kvfree_rcu: Fix comments according to current code

[Uladzislau Rezki]

> On Wed, Apr 28, 2021 at 03:44:22PM +0200, Uladzislau Rezki (Sony) wrote:
> > We used to have an "emergency path" and comments related
> > to it. According to current kvfree_rcu() design that path
> > is not considered as emergency anymore.
> > 
> > This patch rephrases and updates comments regarding this.
> > 
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> 
> Queued as follows, thank you!  Please check my usual wordsmithing to
> make sure that I did not mess anything up.
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit 06c2113c4b1ce9ded69cd0ac4da9a00ed6be8834
> Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
> Date:   Wed Apr 28 15:44:22 2021 +0200
> 
>     kvfree_rcu: Fix comments according to current code
>     
>     The kvfree_rcu() function now defers allocations in the common
>     case due to the fact that there is no lockless access to the
>     memory-allocator caches/pools.  In addition, in CONFIG_PREEMPT_NONE=y
>     and in CONFIG_PREEMPT_VOLUNTARY=y kernels, there is no reliable way to
>     determine if spinlocks are held.  As a result, allocation is deferred in
>     the common case, and the two-argument form of kvfree_rcu() thus uses the
>     "channel 3" queue through all the rcu_head structures.  This channel
>     is called referred to as the emergency case in comments, and these
>     comments are now obsolete.
>     
>     This commit therefore updates these comments to reflect the new
>     common-case nature of such emergencies.
>     
>     Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
>     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> 
Looks good to me.

--
Vlad Rezki

Re: [PATCH v1 4/5] kvfree_rcu: Refactor kfree_rcu_monitor() function

[Paul E. McKenney]

On Mon, May 03, 2021 at 08:12:14PM +0200, Uladzislau Rezki wrote:
> Hello, Paul.
> 
> > Rearm the monitor work directly from its own function that
> > is kfree_rcu_monitor(). So this patch puts the invocation
> > timing control in one place.
> > 
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > ---
> >  kernel/rcu/tree.c | 35 +++++++++++++++++++++--------------
> >  1 file changed, 21 insertions(+), 14 deletions(-)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index e44d6f8c56f0..229e909ad437 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -3415,37 +3415,44 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
> >  	return !repeat;
> >  }
> >  
> > -static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp,
> > -					  unsigned long flags)
> > +/*
> > + * This function queues a new batch. If success or nothing to
> > + * drain it returns 1. Otherwise 0 is returned indicating that
> > + * a reclaim kthread has not processed a previous batch.
> > + */
> > +static inline int kfree_rcu_drain(struct kfree_rcu_cpu *krcp)
> >  {
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	raw_spin_lock_irqsave(&krcp->lock, flags);
> > +
> >  	// Attempt to start a new batch.
> > -	if (queue_kfree_rcu_work(krcp)) {
> > +	ret = queue_kfree_rcu_work(krcp);
> > +	if (ret)
> >  		// Success! Our job is done here.
> >  		krcp->monitor_todo = false;
> > -		raw_spin_unlock_irqrestore(&krcp->lock, flags);
> > -		return;
> > -	}
> >  
> >  	// Previous RCU batch still in progress, try again later.
> > -	schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES);
> >  	raw_spin_unlock_irqrestore(&krcp->lock, flags);
> > +	return ret;
> >  }
> >  
> >  /*
> >   * This function is invoked after the KFREE_DRAIN_JIFFIES timeout.
> > - * It invokes kfree_rcu_drain_unlock() to attempt to start another batch.
> > + * It invokes kfree_rcu_drain() to attempt to start another batch.
> >   */
> >  static void kfree_rcu_monitor(struct work_struct *work)
> >  {
> > -	unsigned long flags;
> >  	struct kfree_rcu_cpu *krcp = container_of(work, struct kfree_rcu_cpu,
> >  						 monitor_work.work);
> >  
> > -	raw_spin_lock_irqsave(&krcp->lock, flags);
> > -	if (krcp->monitor_todo)
> > -		kfree_rcu_drain_unlock(krcp, flags);
> > -	else
> > -		raw_spin_unlock_irqrestore(&krcp->lock, flags);
> > +	if (kfree_rcu_drain(krcp))
> > +		return;
> > +
> > +	// Not success. A previous batch is still in progress.
> > +	// Rearm a work to repeat an attempt of starting another batch.
> > +	schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES);
> >  }
> >  
> >  static enum hrtimer_restart
> > -- 
> > 2.20.1
> > 
> 
> Please see below a v2 of this patch. The main difference between v1
> is that, the monitor work now is open-coded, thus some extra inline
> functions were eliminated:
> 
> >From 7d153a640a4f791cbfd9b546e32f90fb2c60c480 Mon Sep 17 00:00:00 2001
> From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
> Date: Wed, 21 Apr 2021 13:22:52 +0200
> Subject: [PATCH v2] kvfree_rcu: Refactor kfree_rcu_monitor()
> 
> Currently we have three functions which depend on each other.
> Two of them are quite tiny and the last one where the most
> work is done. All of them are related to queuing RCU batches
> to reclaim objects after a GP.
> 
> 1. kfree_rcu_monitor(). It consist of few lines. It acquires
>    the spin-lock and calls "drain" function.
> 
> 2. kfree_rcu_drain_unlock(). It also consists of few lines of
>    code. It calls a func. to queue the batch. If not success
>    rearm the monitor work to repeat an attempt one more time.
> 
> 3. queue_kfree_rcu_work(). Main core part is implemented here.
>    In short, it attempts to start a new batch to free objects
>    after a GP.
> 
> Since there are no external users of [2] and [3] functions we
> can eliminate both by moving all logic directly into [1]. That
> makes the kfree_rcu_monitor() to be open-coded what is easier
> to follow thus becomes less complicated.
> 
> Apart of that, replace comments which start with "/*" to "//"
> format to make it unified across the file.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

Queued and pushed as shown below.  Nice diffstat!  ;-)

						Thanx, Paul

------------------------------------------------------------------------

 tree.c |   84 ++++++++++++++++++++---------------------------------------------
 1 file changed, 26 insertions(+), 58 deletions(-)

commit c47687cb4cd09422c93a1a7dd7562e10439861b6
Author: Uladzislau Rezki (Sony) <urezki@gmail.com>
Date:   Wed Apr 21 13:22:52 2021 +0200

    kvfree_rcu: Refactor kfree_rcu_monitor()
    
    Currently we have three functions which depend on each other.
    Two of them are quite tiny and the last one where the most
    work is done. All of them are related to queuing RCU batches
    to reclaim objects after a GP.
    
    1. kfree_rcu_monitor(). It consist of few lines. It acquires a spin-lock
       and calls kfree_rcu_drain_unlock().
    
    2. kfree_rcu_drain_unlock(). It also consists of few lines of code. It
       calls queue_kfree_rcu_work() to queue the batch.  If this fails,
       it rearms the monitor work to try again later.
    
    3. queue_kfree_rcu_work(). This provides the bulk of the functionality,
       attempting to start a new batch to free objects after a GP.
    
    Since there are no external users of functions [2] and [3], both
    can eliminated by moving all logic directly into [1], which both
    shrinks and simplifies the code.
    
    Also replace comments which start with "/*" to "//" format to make it
    unified across the file.
    
    Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 4d93c6985d3f..3a5fef9fc934 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3362,29 +3362,26 @@ static void kfree_rcu_work(struct work_struct *work)
 }
 
 /*
- * Schedule the kfree batch RCU work to run in workqueue context after a GP.
- *
- * This function is invoked by kfree_rcu_monitor() when the KFREE_DRAIN_JIFFIES
- * timeout has been reached.
+ * This function is invoked after the KFREE_DRAIN_JIFFIES timeout.
  */
-static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
+static void kfree_rcu_monitor(struct work_struct *work)
 {
-	struct kfree_rcu_cpu_work *krwp;
-	bool repeat = false;
+	struct kfree_rcu_cpu *krcp = container_of(work,
+		struct kfree_rcu_cpu, monitor_work.work);
+	unsigned long flags;
 	int i, j;
 
-	lockdep_assert_held(&krcp->lock);
+	raw_spin_lock_irqsave(&krcp->lock, flags);
 
+	// Attempt to start a new batch.
 	for (i = 0; i < KFREE_N_BATCHES; i++) {
-		krwp = &(krcp->krw_arr[i]);
+		struct kfree_rcu_cpu_work *krwp = &(krcp->krw_arr[i]);
 
-		/*
-		 * Try to detach bkvhead or head and attach it over any
-		 * available corresponding free channel. It can be that
-		 * a previous RCU batch is in progress, it means that
-		 * immediately to queue another one is not possible so
-		 * return false to tell caller to retry.
-		 */
+		// Try to detach bkvhead or head and attach it over any
+		// available corresponding free channel. It can be that
+		// a previous RCU batch is in progress, it means that
+		// immediately to queue another one is not possible so
+		// in that case the monitor work is rearmed.
 		if ((krcp->bkvhead[0] && !krwp->bkvhead_free[0]) ||
 			(krcp->bkvhead[1] && !krwp->bkvhead_free[1]) ||
 				(krcp->head && !krwp->head_free)) {
@@ -3406,57 +3403,28 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
 
 			WRITE_ONCE(krcp->count, 0);
 
-			/*
-			 * One work is per one batch, so there are three
-			 * "free channels", the batch can handle. It can
-			 * be that the work is in the pending state when
-			 * channels have been detached following by each
-			 * other.
-			 */
+			// One work is per one batch, so there are three
+			// "free channels", the batch can handle. It can
+			// be that the work is in the pending state when
+			// channels have been detached following by each
+			// other.
 			queue_rcu_work(system_wq, &krwp->rcu_work);
 		}
-
-		// Repeat if any "free" corresponding channel is still busy.
-		if (krcp->bkvhead[0] || krcp->bkvhead[1] || krcp->head)
-			repeat = true;
 	}
 
-	return !repeat;
-}
-
-static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp,
-					  unsigned long flags)
-{
-	// Attempt to start a new batch.
-	if (queue_kfree_rcu_work(krcp)) {
-		// Success! Our job is done here.
+	// If there is nothing to detach, it means that our job is
+	// successfully done here. In case of having at least one
+	// of the channels that is still busy we should rearm the
+	// work to repeat an attempt. Because previous batches are
+	// still in progress.
+	if (!krcp->bkvhead[0] && !krcp->bkvhead[1] && !krcp->head)
 		krcp->monitor_todo = false;
-		raw_spin_unlock_irqrestore(&krcp->lock, flags);
-		return;
-	}
+	else
+		schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES);
 
-	// Previous RCU batch still in progress, try again later.
-	schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES);
 	raw_spin_unlock_irqrestore(&krcp->lock, flags);
 }
 
-/*
- * This function is invoked after the KFREE_DRAIN_JIFFIES timeout.
- * It invokes kfree_rcu_drain_unlock() to attempt to start another batch.
- */
-static void kfree_rcu_monitor(struct work_struct *work)
-{
-	unsigned long flags;
-	struct kfree_rcu_cpu *krcp = container_of(work, struct kfree_rcu_cpu,
-						 monitor_work.work);
-
-	raw_spin_lock_irqsave(&krcp->lock, flags);
-	if (krcp->monitor_todo)
-		kfree_rcu_drain_unlock(krcp, flags);
-	else
-		raw_spin_unlock_irqrestore(&krcp->lock, flags);
-}
-
 static enum hrtimer_restart
 schedule_page_work_fn(struct hrtimer *t)
 {

Re: [PATCH v1 4/5] kvfree_rcu: Refactor kfree_rcu_monitor() function

[Uladzislau Rezki]

On Mon, May 03, 2021 at 03:52:13PM -0700, Paul E. McKenney wrote:
> On Mon, May 03, 2021 at 08:12:14PM +0200, Uladzislau Rezki wrote:
> > Hello, Paul.
> > 
> > > Rearm the monitor work directly from its own function that
> > > is kfree_rcu_monitor(). So this patch puts the invocation
> > > timing control in one place.
> > > 
> > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > > ---
> > >  kernel/rcu/tree.c | 35 +++++++++++++++++++++--------------
> > >  1 file changed, 21 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index e44d6f8c56f0..229e909ad437 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -3415,37 +3415,44 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
> > >  	return !repeat;
> > >  }
> > >  
> > > -static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp,
> > > -					  unsigned long flags)
> > > +/*
> > > + * This function queues a new batch. If success or nothing to
> > > + * drain it returns 1. Otherwise 0 is returned indicating that
> > > + * a reclaim kthread has not processed a previous batch.
> > > + */
> > > +static inline int kfree_rcu_drain(struct kfree_rcu_cpu *krcp)
> > >  {
> > > +	unsigned long flags;
> > > +	int ret;
> > > +
> > > +	raw_spin_lock_irqsave(&krcp->lock, flags);
> > > +
> > >  	// Attempt to start a new batch.
> > > -	if (queue_kfree_rcu_work(krcp)) {
> > > +	ret = queue_kfree_rcu_work(krcp);
> > > +	if (ret)
> > >  		// Success! Our job is done here.
> > >  		krcp->monitor_todo = false;
> > > -		raw_spin_unlock_irqrestore(&krcp->lock, flags);
> > > -		return;
> > > -	}
> > >  
> > >  	// Previous RCU batch still in progress, try again later.
> > > -	schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES);
> > >  	raw_spin_unlock_irqrestore(&krcp->lock, flags);
> > > +	return ret;
> > >  }
> > >  
> > >  /*
> > >   * This function is invoked after the KFREE_DRAIN_JIFFIES timeout.
> > > - * It invokes kfree_rcu_drain_unlock() to attempt to start another batch.
> > > + * It invokes kfree_rcu_drain() to attempt to start another batch.
> > >   */
> > >  static void kfree_rcu_monitor(struct work_struct *work)
> > >  {
> > > -	unsigned long flags;
> > >  	struct kfree_rcu_cpu *krcp = container_of(work, struct kfree_rcu_cpu,
> > >  						 monitor_work.work);
> > >  
> > > -	raw_spin_lock_irqsave(&krcp->lock, flags);
> > > -	if (krcp->monitor_todo)
> > > -		kfree_rcu_drain_unlock(krcp, flags);
> > > -	else
> > > -		raw_spin_unlock_irqrestore(&krcp->lock, flags);
> > > +	if (kfree_rcu_drain(krcp))
> > > +		return;
> > > +
> > > +	// Not success. A previous batch is still in progress.
> > > +	// Rearm a work to repeat an attempt of starting another batch.
> > > +	schedule_delayed_work(&krcp->monitor_work, KFREE_DRAIN_JIFFIES);
> > >  }
> > >  
> > >  static enum hrtimer_restart
> > > -- 
> > > 2.20.1
> > > 
> > 
> > Please see below a v2 of this patch. The main difference between v1
> > is that, the monitor work now is open-coded, thus some extra inline
> > functions were eliminated:
> > 
> > >From 7d153a640a4f791cbfd9b546e32f90fb2c60c480 Mon Sep 17 00:00:00 2001
> > From: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
> > Date: Wed, 21 Apr 2021 13:22:52 +0200
> > Subject: [PATCH v2] kvfree_rcu: Refactor kfree_rcu_monitor()
> > 
> > Currently we have three functions which depend on each other.
> > Two of them are quite tiny and the last one where the most
> > work is done. All of them are related to queuing RCU batches
> > to reclaim objects after a GP.
> > 
> > 1. kfree_rcu_monitor(). It consist of few lines. It acquires
> >    the spin-lock and calls "drain" function.
> > 
> > 2. kfree_rcu_drain_unlock(). It also consists of few lines of
> >    code. It calls a func. to queue the batch. If not success
> >    rearm the monitor work to repeat an attempt one more time.
> > 
> > 3. queue_kfree_rcu_work(). Main core part is implemented here.
> >    In short, it attempts to start a new batch to free objects
> >    after a GP.
> > 
> > Since there are no external users of [2] and [3] functions we
> > can eliminate both by moving all logic directly into [1]. That
> > makes the kfree_rcu_monitor() to be open-coded what is easier
> > to follow thus becomes less complicated.
> > 
> > Apart of that, replace comments which start with "/*" to "//"
> > format to make it unified across the file.
> > 
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> 
> Queued and pushed as shown below.  Nice diffstat!  ;-)
> 
After such refactoring everything seems has been settled :)

Thanks.

--
Vlad Rezki

Re: [PATCH v1 4/5] kvfree_rcu: Refactor kfree_rcu_monitor() function

[Andrew Morton]

On Wed, 28 Apr 2021 15:44:21 +0200 "Uladzislau Rezki (Sony)" <urezki@gmail.com> wrote:

> Rearm the monitor work directly from its own function that
> is kfree_rcu_monitor(). So this patch puts the invocation
> timing control in one place.
>
> ...
>
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3415,37 +3415,44 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
>  	return !repeat;
>  }
>  
> -static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp,
> -					  unsigned long flags)
> +/*
> + * This function queues a new batch. If success or nothing to
> + * drain it returns 1. Otherwise 0 is returned indicating that
> + * a reclaim kthread has not processed a previous batch.
> + */
> +static inline int kfree_rcu_drain(struct kfree_rcu_cpu *krcp)
>  {
> +	unsigned long flags;
> +	int ret;
> +
> +	raw_spin_lock_irqsave(&krcp->lock, flags);
> +
>  	// Attempt to start a new batch.
> -	if (queue_kfree_rcu_work(krcp)) {
> +	ret = queue_kfree_rcu_work(krcp);

This code has changed slightly in mainline.  Can you please redo,
retest and resend?

> +	if (ret)
>  		// Success! Our job is done here.
>  		krcp->monitor_todo = false;
> -		raw_spin_unlock_irqrestore(&krcp->lock, flags);
> -		return;
> -	}

It's conventional to retain the braces here, otherwise the code looks
weird.  Unless you're a python programmer ;)

Re: [PATCH v1 4/5] kvfree_rcu: Refactor kfree_rcu_monitor() function

[Uladzislau Rezki]

On Sun, May 09, 2021 at 04:59:54PM -0700, Andrew Morton wrote:
> On Wed, 28 Apr 2021 15:44:21 +0200 "Uladzislau Rezki (Sony)" <urezki@gmail.com> wrote:
> 
> > Rearm the monitor work directly from its own function that
> > is kfree_rcu_monitor(). So this patch puts the invocation
> > timing control in one place.
> >
> > ...
> >
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -3415,37 +3415,44 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
> >  	return !repeat;
> >  }
> >  
> > -static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp,
> > -					  unsigned long flags)
> > +/*
> > + * This function queues a new batch. If success or nothing to
> > + * drain it returns 1. Otherwise 0 is returned indicating that
> > + * a reclaim kthread has not processed a previous batch.
> > + */
> > +static inline int kfree_rcu_drain(struct kfree_rcu_cpu *krcp)
> >  {
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	raw_spin_lock_irqsave(&krcp->lock, flags);
> > +
> >  	// Attempt to start a new batch.
> > -	if (queue_kfree_rcu_work(krcp)) {
> > +	ret = queue_kfree_rcu_work(krcp);
> 
> This code has changed slightly in mainline.  Can you please redo,
> retest and resend?
> 
> > +	if (ret)
> >  		// Success! Our job is done here.
> >  		krcp->monitor_todo = false;
> > -		raw_spin_unlock_irqrestore(&krcp->lock, flags);
> > -		return;
> > -	}
> 
> It's conventional to retain the braces here, otherwise the code looks
> weird.  Unless you're a python programmer ;)
> 
> 
Hello, Anrew.

This refactoring is not up to date and is obsolete, instead we have done 
bigger rework of kfree_rcu_monitor(). It is located here:

https://kernel.googlesource.com/pub/scm/linux/kernel/git/paulmck/linux-rcu/+/2349a35d39e7af5eef9064cbd0e42309040551da%5E%21/#F0

--
Vlad Rezki

Re: [PATCH v1 4/5] kvfree_rcu: Refactor kfree_rcu_monitor() function

[Paul E. McKenney]

On Mon, May 10, 2021 at 12:09:01PM +0200, Uladzislau Rezki wrote:
> On Sun, May 09, 2021 at 04:59:54PM -0700, Andrew Morton wrote:
> > On Wed, 28 Apr 2021 15:44:21 +0200 "Uladzislau Rezki (Sony)" <urezki@gmail.com> wrote:
> > 
> > > Rearm the monitor work directly from its own function that
> > > is kfree_rcu_monitor(). So this patch puts the invocation
> > > timing control in one place.
> > >
> > > ...
> > >
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -3415,37 +3415,44 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
> > >  	return !repeat;
> > >  }
> > >  
> > > -static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp,
> > > -					  unsigned long flags)
> > > +/*
> > > + * This function queues a new batch. If success or nothing to
> > > + * drain it returns 1. Otherwise 0 is returned indicating that
> > > + * a reclaim kthread has not processed a previous batch.
> > > + */
> > > +static inline int kfree_rcu_drain(struct kfree_rcu_cpu *krcp)
> > >  {
> > > +	unsigned long flags;
> > > +	int ret;
> > > +
> > > +	raw_spin_lock_irqsave(&krcp->lock, flags);
> > > +
> > >  	// Attempt to start a new batch.
> > > -	if (queue_kfree_rcu_work(krcp)) {
> > > +	ret = queue_kfree_rcu_work(krcp);
> > 
> > This code has changed slightly in mainline.  Can you please redo,
> > retest and resend?
> > 
> > > +	if (ret)
> > >  		// Success! Our job is done here.
> > >  		krcp->monitor_todo = false;
> > > -		raw_spin_unlock_irqrestore(&krcp->lock, flags);
> > > -		return;
> > > -	}
> > 
> > It's conventional to retain the braces here, otherwise the code looks
> > weird.  Unless you're a python programmer ;)
> > 
> > 
> Hello, Anrew.
> 
> This refactoring is not up to date and is obsolete, instead we have done 
> bigger rework of kfree_rcu_monitor(). It is located here:
> 
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/paulmck/linux-rcu/+/2349a35d39e7af5eef9064cbd0e42309040551da%5E%21/#F0

If Andrew would like to start taking these kvfree_rcu() patches,
that would be all to the good.  For example, there is likely much
more opportunity for optimization by bringing them closer to the
sl*b allocators.  Yes, they will need some privileged access to RCU
internals, but not that much.  And at some point, they should move from
their current home in kernel/rcu/tree.c to somewhere in mm.

To that end, here is the list in -rcu against current mainline, from
youngest to oldest:

b5691dd1cd7a kvfree_rcu: Fix comments according to current code
2349a35d39e7 kvfree_rcu: Refactor kfree_rcu_monitor()
bfa15885893f kvfree_rcu: Release a page cache under memory pressure
de9d86c3b0b7 kvfree_rcu: Use [READ/WRITE]_ONCE() macros to access to nr_bkv_objs
54a0393340f7 kvfree_rcu: Add a bulk-list check when a scheduler is run
7490789de1ac kvfree_rcu: Update "monitor_todo" once a batch is started
28e690ce0347 kvfree_rcu: Use kfree_rcu_monitor() instead of open-coded variant

Please let me know how you would like to proceed.

							Thanx, Paul

Re: [PATCH v1 4/5] kvfree_rcu: Refactor kfree_rcu_monitor() function

[Uladzislau Rezki]

On Mon, May 10, 2021 at 07:01:43AM -0700, Paul E. McKenney wrote:
> On Mon, May 10, 2021 at 12:09:01PM +0200, Uladzislau Rezki wrote:
> > On Sun, May 09, 2021 at 04:59:54PM -0700, Andrew Morton wrote:
> > > On Wed, 28 Apr 2021 15:44:21 +0200 "Uladzislau Rezki (Sony)" <urezki@gmail.com> wrote:
> > > 
> > > > Rearm the monitor work directly from its own function that
> > > > is kfree_rcu_monitor(). So this patch puts the invocation
> > > > timing control in one place.
> > > >
> > > > ...
> > > >
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -3415,37 +3415,44 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
> > > >  	return !repeat;
> > > >  }
> > > >  
> > > > -static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp,
> > > > -					  unsigned long flags)
> > > > +/*
> > > > + * This function queues a new batch. If success or nothing to
> > > > + * drain it returns 1. Otherwise 0 is returned indicating that
> > > > + * a reclaim kthread has not processed a previous batch.
> > > > + */
> > > > +static inline int kfree_rcu_drain(struct kfree_rcu_cpu *krcp)
> > > >  {
> > > > +	unsigned long flags;
> > > > +	int ret;
> > > > +
> > > > +	raw_spin_lock_irqsave(&krcp->lock, flags);
> > > > +
> > > >  	// Attempt to start a new batch.
> > > > -	if (queue_kfree_rcu_work(krcp)) {
> > > > +	ret = queue_kfree_rcu_work(krcp);
> > > 
> > > This code has changed slightly in mainline.  Can you please redo,
> > > retest and resend?
> > > 
> > > > +	if (ret)
> > > >  		// Success! Our job is done here.
> > > >  		krcp->monitor_todo = false;
> > > > -		raw_spin_unlock_irqrestore(&krcp->lock, flags);
> > > > -		return;
> > > > -	}
> > > 
> > > It's conventional to retain the braces here, otherwise the code looks
> > > weird.  Unless you're a python programmer ;)
> > > 
> > > 
> > Hello, Anrew.
> > 
> > This refactoring is not up to date and is obsolete, instead we have done 
> > bigger rework of kfree_rcu_monitor(). It is located here:
> > 
> > https://kernel.googlesource.com/pub/scm/linux/kernel/git/paulmck/linux-rcu/+/2349a35d39e7af5eef9064cbd0e42309040551da%5E%21/#F0
> 
> If Andrew would like to start taking these kvfree_rcu() patches,
> that would be all to the good.  For example, there is likely much
> more opportunity for optimization by bringing them closer to the
> sl*b allocators.  Yes, they will need some privileged access to RCU
> internals, but not that much.  And at some point, they should move from
> their current home in kernel/rcu/tree.c to somewhere in mm.
> 
That is the plan to change the home :)


> To that end, here is the list in -rcu against current mainline, from
> youngest to oldest:
> 
> b5691dd1cd7a kvfree_rcu: Fix comments according to current code
> 2349a35d39e7 kvfree_rcu: Refactor kfree_rcu_monitor()
> bfa15885893f kvfree_rcu: Release a page cache under memory pressure
> de9d86c3b0b7 kvfree_rcu: Use [READ/WRITE]_ONCE() macros to access to nr_bkv_objs
> 54a0393340f7 kvfree_rcu: Add a bulk-list check when a scheduler is run
> 7490789de1ac kvfree_rcu: Update "monitor_todo" once a batch is started
> 28e690ce0347 kvfree_rcu: Use kfree_rcu_monitor() instead of open-coded variant
> 
> Please let me know how you would like to proceed.
> 
> 							Thanx, Paul