[PATCH 1/4] rcu/kvfree: Support dynamic rcu_head for single argument objects

[PATCH 1/4] rcu/kvfree: Support dynamic rcu_head for single argument objects

[Uladzislau Rezki (Sony)]

Add a support of dynamically attaching an rcu_head to an object
which gets freed via the single argument of kvfree_rcu(). This is
used in the path, when a page allocation fails due to a high memory
pressure.

The basic idea behind of this is to minimize a hit of slow path
which requires a caller to wait until a grace period is passed.

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

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index be00aac5f4e7..0124411fecfb 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3425,6 +3425,11 @@ kvfree_rcu_bulk(struct kfree_rcu_cpu *krcp,
 	cond_resched_tasks_rcu_qs();
 }
 
+struct dyn_rcu_head {
+	unsigned long *ptr;
+	struct rcu_head rh;
+};
+
 static void
 kvfree_rcu_list(struct rcu_head *head)
 {
@@ -3433,15 +3438,32 @@ kvfree_rcu_list(struct rcu_head *head)
 	for (; head; head = next) {
 		void *ptr = (void *) head->func;
 		unsigned long offset = (void *) head - ptr;
+		struct dyn_rcu_head *drhp = NULL;
+
+		/*
+		 * For dynamically attached rcu_head, a ->func field
+		 * points to _offset_, i.e. not to a pointer which has
+		 * to be freed. For such objects, adjust an offset and
+		 * pointer.
+		 */
+		if (__is_kvfree_rcu_offset((unsigned long) ptr)) {
+			drhp = container_of(head, struct dyn_rcu_head, rh);
+			offset = (unsigned long) drhp->rh.func;
+			ptr = drhp->ptr;
+		}
 
 		next = head->next;
 		debug_rcu_head_unqueue((struct rcu_head *)ptr);
 		rcu_lock_acquire(&rcu_callback_map);
 		trace_rcu_invoke_kvfree_callback(rcu_state.name, head, offset);
 
-		if (!WARN_ON_ONCE(!__is_kvfree_rcu_offset(offset)))
+		if (!WARN_ON_ONCE(!__is_kvfree_rcu_offset(offset))) {
 			kvfree(ptr);
 
+			if (drhp)
+				kvfree(drhp);
+		}
+
 		rcu_lock_release(&rcu_callback_map);
 		cond_resched_tasks_rcu_qs();
 	}
@@ -3787,6 +3809,21 @@ add_ptr_to_bulk_krc_lock(struct kfree_rcu_cpu **krcp,
 	return true;
 }
 
+static struct rcu_head *
+attach_rcu_head_to_object(void *obj)
+{
+	struct dyn_rcu_head *rhp;
+
+	rhp = kmalloc(sizeof(struct dyn_rcu_head), GFP_KERNEL |
+		__GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
+
+	if (!rhp)
+		return NULL;
+
+	rhp->ptr = obj;
+	return &rhp->rh;
+}
+
 /*
  * Queue a request for lazy invocation of the appropriate free routine
  * after a grace period.  Please note that three paths are maintained,
@@ -3830,9 +3867,17 @@ void kvfree_call_rcu(struct rcu_head *head, void *ptr)
 	if (!success) {
 		run_page_cache_worker(krcp);
 
-		if (head == NULL)
-			// Inline if kvfree_rcu(one_arg) call.
-			goto unlock_return;
+		if (!head) {
+			krc_this_cpu_unlock(krcp, flags);
+			head = attach_rcu_head_to_object(ptr);
+			krcp = krc_this_cpu_lock(&flags);
+
+			if (!head)
+				// Inline if kvfree_rcu(one_arg) call.
+				goto unlock_return;
+
+			ptr = (rcu_callback_t) offsetof(struct dyn_rcu_head, rh);
+		}
 
 		head->func = ptr;
 		head->next = krcp->head;
-- 
2.39.2

[PATCH 2/4] rcu/kvfree: Add a switcher for dynamic rcu_head

[Uladzislau Rezki (Sony)]

Add a sysfs attribute to control whether a dynamically
rcu_head should be attached or not. It can be controlled
via "/sys/module/rcutree/parameters/use_dyn_rcu_head".

By default it is OFF.

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

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 0124411fecfb..893ee69d4a4b 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -206,6 +206,9 @@ module_param(rcu_min_cached_objs, int, 0444);
 static int rcu_delay_page_cache_fill_msec = 5000;
 module_param(rcu_delay_page_cache_fill_msec, int, 0444);
 
+static bool use_dyn_rcu_head __read_mostly;
+module_param(use_dyn_rcu_head, bool, 0444);
+
 /* Retrieve RCU kthreads priority for rcutorture */
 int rcu_get_gp_kthreads_prio(void)
 {
@@ -3814,6 +3817,9 @@ attach_rcu_head_to_object(void *obj)
 {
 	struct dyn_rcu_head *rhp;
 
+	if (!use_dyn_rcu_head)
+		return NULL;
+
 	rhp = kmalloc(sizeof(struct dyn_rcu_head), GFP_KERNEL |
 		__GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
 
-- 
2.39.2

[PATCH 3/4] rcu/kvfree: Use polled API in a slow path

[Uladzislau Rezki (Sony)]

For a single argument use a polled API to see if a GP is
already passed. This allows to bypass an extra GP request
in a slow path.

Allocating a page or dynamic rcu_head might take some and
still fail, in that scenario a GP which is requested on entry
of kvfree_call_rcu() can be elapsed. Benefit from this.

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

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 893ee69d4a4b..030a453f36c6 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3845,6 +3845,7 @@ attach_rcu_head_to_object(void *obj)
 void kvfree_call_rcu(struct rcu_head *head, void *ptr)
 {
 	unsigned long flags;
+	struct rcu_gp_oldstate old_snap;
 	struct kfree_rcu_cpu *krcp;
 	bool success;
 
@@ -3855,8 +3856,10 @@ void kvfree_call_rcu(struct rcu_head *head, void *ptr)
 	 * only. For other places please embed an rcu_head to
 	 * your data.
 	 */
-	if (!head)
+	if (!head) {
 		might_sleep();
+		start_poll_synchronize_rcu_full(&old_snap);
+	}
 
 	// Queue the object but don't yet schedule the batch.
 	if (debug_rcu_head_queue(ptr)) {
@@ -3917,7 +3920,10 @@ void kvfree_call_rcu(struct rcu_head *head, void *ptr)
 	 */
 	if (!success) {
 		debug_rcu_head_unqueue((struct rcu_head *) ptr);
-		synchronize_rcu();
+
+		if (!poll_state_synchronize_rcu_full(&old_snap))
+			synchronize_rcu();
+
 		kvfree(ptr);
 	}
 }
-- 
2.39.2

[PATCH 4/4] rcu/kvfree: Switch to expedited version in slow path

[Uladzislau Rezki (Sony)]

For a single argument and its slow path, switch to expedited
version of synchronize_rcu(). This version is considered to
be more faster, thus under a high memory pressure a slow path
becoms more efficient.

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

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 030a453f36c6..835d90905ec1 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3922,7 +3922,7 @@ void kvfree_call_rcu(struct rcu_head *head, void *ptr)
 		debug_rcu_head_unqueue((struct rcu_head *) ptr);
 
 		if (!poll_state_synchronize_rcu_full(&old_snap))
-			synchronize_rcu();
+			synchronize_rcu_expedited();
 
 		kvfree(ptr);
 	}
-- 
2.39.2

Re: [PATCH 1/4] rcu/kvfree: Support dynamic rcu_head for single argument objects

[Vlastimil Babka]

On 8/28/24 13:09, Uladzislau Rezki (Sony) wrote:
> Add a support of dynamically attaching an rcu_head to an object
> which gets freed via the single argument of kvfree_rcu(). This is
> used in the path, when a page allocation fails due to a high memory
> pressure.
> 
> The basic idea behind of this is to minimize a hit of slow path
> which requires a caller to wait until a grace period is passed.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

So IIUC it's a situation where we can't allocate a page, but we hope the
kmalloc-32 slab has still free objects to give us dyn_rcu_head's before it
would have to also make a page allocation?

So that may really be possible and there might potentially be many such
objects, but I wonder if there's really a benefit. The system is struggling
for memory and the single-argument caller specifically is _mightsleep so it
could e.g. instead go direct reclaim a page rather than start depleting the
kmalloc-32 slab, no?

> ---
>  kernel/rcu/tree.c | 53 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index be00aac5f4e7..0124411fecfb 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3425,6 +3425,11 @@ kvfree_rcu_bulk(struct kfree_rcu_cpu *krcp,
>  	cond_resched_tasks_rcu_qs();
>  }
>  
> +struct dyn_rcu_head {
> +	unsigned long *ptr;
> +	struct rcu_head rh;
> +};
> +
>  static void
>  kvfree_rcu_list(struct rcu_head *head)
>  {
> @@ -3433,15 +3438,32 @@ kvfree_rcu_list(struct rcu_head *head)
>  	for (; head; head = next) {
>  		void *ptr = (void *) head->func;
>  		unsigned long offset = (void *) head - ptr;
> +		struct dyn_rcu_head *drhp = NULL;
> +
> +		/*
> +		 * For dynamically attached rcu_head, a ->func field
> +		 * points to _offset_, i.e. not to a pointer which has
> +		 * to be freed. For such objects, adjust an offset and
> +		 * pointer.
> +		 */
> +		if (__is_kvfree_rcu_offset((unsigned long) ptr)) {
> +			drhp = container_of(head, struct dyn_rcu_head, rh);
> +			offset = (unsigned long) drhp->rh.func;
> +			ptr = drhp->ptr;
> +		}
>  
>  		next = head->next;
>  		debug_rcu_head_unqueue((struct rcu_head *)ptr);
>  		rcu_lock_acquire(&rcu_callback_map);
>  		trace_rcu_invoke_kvfree_callback(rcu_state.name, head, offset);
>  
> -		if (!WARN_ON_ONCE(!__is_kvfree_rcu_offset(offset)))
> +		if (!WARN_ON_ONCE(!__is_kvfree_rcu_offset(offset))) {
>  			kvfree(ptr);
>  
> +			if (drhp)
> +				kvfree(drhp);
> +		}
> +
>  		rcu_lock_release(&rcu_callback_map);
>  		cond_resched_tasks_rcu_qs();
>  	}
> @@ -3787,6 +3809,21 @@ add_ptr_to_bulk_krc_lock(struct kfree_rcu_cpu **krcp,
>  	return true;
>  }
>  
> +static struct rcu_head *
> +attach_rcu_head_to_object(void *obj)
> +{
> +	struct dyn_rcu_head *rhp;
> +
> +	rhp = kmalloc(sizeof(struct dyn_rcu_head), GFP_KERNEL |
> +		__GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
> +
> +	if (!rhp)
> +		return NULL;
> +
> +	rhp->ptr = obj;
> +	return &rhp->rh;
> +}
> +
>  /*
>   * Queue a request for lazy invocation of the appropriate free routine
>   * after a grace period.  Please note that three paths are maintained,
> @@ -3830,9 +3867,17 @@ void kvfree_call_rcu(struct rcu_head *head, void *ptr)
>  	if (!success) {
>  		run_page_cache_worker(krcp);
>  
> -		if (head == NULL)
> -			// Inline if kvfree_rcu(one_arg) call.
> -			goto unlock_return;
> +		if (!head) {
> +			krc_this_cpu_unlock(krcp, flags);
> +			head = attach_rcu_head_to_object(ptr);
> +			krcp = krc_this_cpu_lock(&flags);
> +
> +			if (!head)
> +				// Inline if kvfree_rcu(one_arg) call.
> +				goto unlock_return;
> +
> +			ptr = (rcu_callback_t) offsetof(struct dyn_rcu_head, rh);
> +		}
>  
>  		head->func = ptr;
>  		head->next = krcp->head;

Re: [PATCH 1/4] rcu/kvfree: Support dynamic rcu_head for single argument objects

[Uladzislau Rezki]

On Wed, Aug 28, 2024 at 04:58:48PM +0200, Vlastimil Babka wrote:
> On 8/28/24 13:09, Uladzislau Rezki (Sony) wrote:
> > Add a support of dynamically attaching an rcu_head to an object
> > which gets freed via the single argument of kvfree_rcu(). This is
> > used in the path, when a page allocation fails due to a high memory
> > pressure.
> > 
> > The basic idea behind of this is to minimize a hit of slow path
> > which requires a caller to wait until a grace period is passed.
> > 
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> 
> So IIUC it's a situation where we can't allocate a page, but we hope the
> kmalloc-32 slab has still free objects to give us dyn_rcu_head's before it
> would have to also make a page allocation?
> 
Yes, you understood it correctly :)

>
> So that may really be possible and there might potentially be many such
> objects, but I wonder if there's really a benefit. The system is struggling
> for memory and the single-argument caller specifically is _mightsleep so it
> could e.g. instead go direct reclaim a page rather than start depleting the
> kmalloc-32 slab, no?
> 
This is a good question about benefit and i need to say that i do not
have a strong opinion here. I post this patch to get some opinions about
it. This "dynamic attaching" we discussed with RCU folk a few years ago
and decided not to go with it. I have not found an information why.

The page request path, which is "normal/fast", can lead to a "light"
direct reclaim, if still fails, then we are in a high pressure situation.
Depleting a slab is probably not worth it, especially that the patch in
this series:

[PATCH 4/4] rcu/kvfree: Switch to expedited version in slow path

switches to more faster synchronize_rcu() version to speedup a reclaim.

+ this [PATCH 3/4] rcu/kvfree: Use polled API in a slow path
which also improves a slow path in terms of that a GP might be already
passed for the object being freed.

I am totally OK to drop this patch. This is fine to me.

--
Uladzislau Rezki

Re: [PATCH 1/4] rcu/kvfree: Support dynamic rcu_head for single argument objects

[Paul E. McKenney]

On Wed, Aug 28, 2024 at 07:00:11PM +0200, Uladzislau Rezki wrote:
> On Wed, Aug 28, 2024 at 04:58:48PM +0200, Vlastimil Babka wrote:
> > On 8/28/24 13:09, Uladzislau Rezki (Sony) wrote:
> > > Add a support of dynamically attaching an rcu_head to an object
> > > which gets freed via the single argument of kvfree_rcu(). This is
> > > used in the path, when a page allocation fails due to a high memory
> > > pressure.
> > > 
> > > The basic idea behind of this is to minimize a hit of slow path
> > > which requires a caller to wait until a grace period is passed.
> > > 
> > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > 
> > So IIUC it's a situation where we can't allocate a page, but we hope the
> > kmalloc-32 slab has still free objects to give us dyn_rcu_head's before it
> > would have to also make a page allocation?
> > 
> Yes, you understood it correctly :)
> 
> >
> > So that may really be possible and there might potentially be many such
> > objects, but I wonder if there's really a benefit. The system is struggling
> > for memory and the single-argument caller specifically is _mightsleep so it
> > could e.g. instead go direct reclaim a page rather than start depleting the
> > kmalloc-32 slab, no?
> > 
> This is a good question about benefit and i need to say that i do not
> have a strong opinion here. I post this patch to get some opinions about
> it. This "dynamic attaching" we discussed with RCU folk a few years ago
> and decided not to go with it. I have not found an information why.

If I remember correctly, I asked "How are you testing this?", which
was then taken as a criticism rather than a question.  ;-)

No one has reported an OOM-related problem with the code in its current
form, for what little that is worth.

							Thanx, Paul

> The page request path, which is "normal/fast", can lead to a "light"
> direct reclaim, if still fails, then we are in a high pressure situation.
> Depleting a slab is probably not worth it, especially that the patch in
> this series:
> 
> [PATCH 4/4] rcu/kvfree: Switch to expedited version in slow path
> 
> switches to more faster synchronize_rcu() version to speedup a reclaim.
> 
> + this [PATCH 3/4] rcu/kvfree: Use polled API in a slow path
> which also improves a slow path in terms of that a GP might be already
> passed for the object being freed.
> 
> I am totally OK to drop this patch. This is fine to me.
> 
> --
> Uladzislau Rezki