rcu/tree: add emergency pool for headless case

Quoted 4 lines

kernel/rcu/tree.c +0 -0

@@ -2877,6 +2885,12 @@ struct kfree_rcu_cpu {
 	bool initialized;
 	// Number of objects for which GP not started
 	int count;

kernel/rcu/tree.c struct kfree_rcu_cpu { · line 2893

Quoted 6 lines

kernel/rcu/tree.c +6 -0

+
+	/*
+	 * Reserved emergency pool for headless variant.
+	 */
+	int nr_emergency;
+	void **emergency;

This is a pretty expensive way to maintain an emergency pool.

Try something like this ...

struct emergency_pool_object {
	union {
		struct whatever foo;
		struct {
			int remaining;
			void *next;
		};
	};
};

struct kfree_rcu_cpu {
	...
	struct emergency_pool_object *epo;
};

struct whatever *get_emergency_object(struct kfree_rcu_cpu *krc)

{
	struct emergency_pool_object *epo = krc->epo;
	if (epo)
		krc->epo = epo->next;
	return &epo->foo;
}

void alloc_emergency_objects(struct kfree_rcu_cpu *krc, int n)

{
	int i = 0;

	if (krc->epo)
		i = krc->epo->remaining;

	while (++i < n) {
		struct emergency_pool_object *epo = kmalloc(sizeof(epo), GFP);
		epo->remaining = i;
		epo->next = krc->epo;
		krc->epo = epo;
	}
}

Maintain an emergency pool for each CPU with some extra objects. There is read-only sysfs attribute, the name is "rcu_nr_emergency_objs". It reflects the size of the pool. As for now the default value is 3.

The pool is populated when low memory condition is detected. Please note it is only for headless case it means when the regular SLAB is not able to serve any request, the pool is used.

Signed-off-by Uladzislau Rezki (Sony) <[email protected]>

---
 kernel/rcu/tree.c | 133 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 97 insertions(+), 36 deletions(-)

kernel/rcu/tree.c +0 -0

index 5e26145e9ead..f9f1f935ab0b 100644

Quoted 24 lines · Uladzislau Rezki (Sony)

Maintain an emergency pool for each CPU with some extra objects. There is read-only sysfs attribute, the name is "rcu_nr_emergency_objs". It reflects the size of the pool. As for now the default value is 3.

The pool is populated when low memory condition is detected. Please note it is only for headless case it means when the regular SLAB is not able to serve any request, the pool is used.

Signed-off-by Uladzislau Rezki (Sony) <[email protected]>

---
 kernel/rcu/tree.c | 133 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 97 insertions(+), 36 deletions(-)

kernel/rcu/tree.c +0 -0

index 5e26145e9ead..f9f1f935ab0b 100644
@@ -114,6 +114,14 @@ int rcu_num_lvls __read_mostly = RCU_NUM_LVLS;
 int rcu_kfree_nowarn;
 module_param(rcu_kfree_nowarn, int, 0444);
 

kernel/rcu/tree.c int rcu_num_lvls __read_mostly = RCU_NUM_LVLS; · line 123

Quoted 7 lines

kernel/rcu/tree.c +7 -0

+/*
+ * For headless variant. Under memory pressure an
+ * emergency pool can be used if the regular SLAB
+ * is not able to serve some memory for us.
+ */
+int rcu_nr_emergency_objs = 3;
+module_param(rcu_nr_emergency_objs, int, 0444);

Please document this in Documentation/admin-guide/kernel-parameters.txt.

Maintain an emergency pool for each CPU with some extra objects. There is read-only sysfs attribute, the name is "rcu_nr_emergency_objs". It reflects the size of the pool. As for now the default value is 3.

The pool is populated when low memory condition is detected. Please note it is only for headless case it means when the regular SLAB is not able to serve any request, the pool is used.

Signed-off-by Uladzislau Rezki (Sony) <[email protected]>

---
 kernel/rcu/tree.c | 133 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 97 insertions(+), 36 deletions(-)

kernel/rcu/tree.c +97 -36

index 5e26145e9ead..f9f1f935ab0b 100644
@@ -114,6 +114,14 @@ int rcu_num_lvls __read_mostly = RCU_NUM_LVLS;
 int rcu_kfree_nowarn;
 module_param(rcu_kfree_nowarn, int, 0444);
 
+/*
+ * For headless variant. Under memory pressure an
+ * emergency pool can be used if the regular SLAB
+ * is not able to serve some memory for us.
+ */
+int rcu_nr_emergency_objs = 3;
+module_param(rcu_nr_emergency_objs, int, 0444);
+
 /* Number of rcu_nodes at specified level. */
 int num_rcu_lvl[] = NUM_RCU_LVL_INIT;
 int rcu_num_nodes __read_mostly = NUM_RCU_NODES; /* Total # rcu_nodes in use. */

@@ -2877,6 +2885,12 @@ struct kfree_rcu_cpu {
 	bool initialized;
 	// Number of objects for which GP not started
 	int count;
+
+	/*
+	 * Reserved emergency pool for headless variant.
+	 */
+	int nr_emergency;
+	void **emergency;
 };
 
 static DEFINE_PER_CPU(struct kfree_rcu_cpu, krc);

@@ -2892,6 +2906,27 @@ debug_rcu_bhead_unqueue(struct kvfree_rcu_bulk_data *bhead)
 #endif
 }
 
+static inline struct kfree_rcu_cpu *
+krc_this_cpu_lock(unsigned long *flags)
+{
+	struct kfree_rcu_cpu *krcp;
+
+	local_irq_save(*flags);	// For safely calling this_cpu_ptr().
+	krcp = this_cpu_ptr(&krc);
+	if (likely(krcp->initialized))
+		spin_lock(&krcp->lock);
+
+	return krcp;
+}
+
+static inline void
+krc_this_cpu_unlock(struct kfree_rcu_cpu *krcp, unsigned long flags)
+{
+	if (likely(krcp->initialized))
+		spin_unlock(&krcp->lock);
+	local_irq_restore(flags);
+}
+
 /*
  * This function is invoked in workqueue context after a grace period.
  * It frees all the objects queued on ->bhead_free or ->head_free.

@@ -2974,6 +3009,7 @@ static void kfree_rcu_work(struct work_struct *work)
 	 */
 	for (; head; head = next) {
 		unsigned long offset = (unsigned long)head->func;
+		unsigned long flags;
 		bool headless;
 		void *ptr;
 

@@ -2991,10 +3027,23 @@ static void kfree_rcu_work(struct work_struct *work)
 		trace_rcu_invoke_kvfree_callback(rcu_state.name, head, offset);
 
 		if (!WARN_ON_ONCE(!__is_kvfree_rcu_offset(offset))) {
-			if (headless)
+			if (headless) {
 				kvfree((void *) *((unsigned long *) ptr));
 
-			kvfree(ptr);
+				krcp = krc_this_cpu_lock(&flags);
+				if (krcp->emergency) {
+					if (krcp->nr_emergency < rcu_nr_emergency_objs) {
+						krcp->emergency[krcp->nr_emergency++] = ptr;
+
+						/* Bypass freeing of it, it is in emergency pool. */
+						ptr = NULL;
+					}
+				}
+				krc_this_cpu_unlock(krcp, flags);
+			}
+
+			if (ptr)
+				kvfree(ptr);
 		}
 
 		rcu_lock_release(&rcu_callback_map);

@@ -3144,40 +3193,26 @@ kvfree_call_rcu_add_ptr_to_bulk(struct kfree_rcu_cpu *krcp, void *ptr)
 	return true;
 }
 
-static inline struct rcu_head *attach_rcu_head_to_object(void *obj)
+static inline struct rcu_head *
+set_ptr_in_rcu_head_obj(void *ptr, unsigned long *rho)
+{
+	rho[0] = (unsigned long) ptr;
+	return ((struct rcu_head *) ++rho);
+}
+
+static inline struct rcu_head *
+alloc_rcu_head_obj(void *ptr)
 {
-	unsigned long *ptr;
+	unsigned long *rho;
 
 	/* Try hard to get the memory. */
-	ptr = kmalloc(sizeof(unsigned long *) +
+	rho = kmalloc(sizeof(unsigned long *) +
 		sizeof(struct rcu_head), GFP_KERNEL |
 			__GFP_ATOMIC | __GFP_HIGH | __GFP_RETRY_MAYFAIL);
-	if (!ptr)
+	if (!rho)
 		return NULL;
 
-	ptr[0] = (unsigned long) obj;
-	return ((struct rcu_head *) ++ptr);
-}
-
-static inline struct kfree_rcu_cpu *
-krc_this_cpu_lock(unsigned long *flags)
-{
-	struct kfree_rcu_cpu *krcp;
-
-	local_irq_save(*flags);	// For safely calling this_cpu_ptr().
-	krcp = this_cpu_ptr(&krc);
-	if (likely(krcp->initialized))
-		spin_lock(&krcp->lock);
-
-	return krcp;
-}
-
-static inline void
-krc_this_cpu_unlock(struct kfree_rcu_cpu *krcp, unsigned long flags)
-{
-	if (likely(krcp->initialized))
-		spin_unlock(&krcp->lock);
-	local_irq_restore(flags);
+	return set_ptr_in_rcu_head_obj(ptr, rho);
 }
 
 /*

@@ -3237,15 +3272,31 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 	if (!success) {
 		/* Is headless object? */
 		if (head == NULL) {
-			/* Drop the lock. */
+			/*
+			 * Drop the lock to use more permissive
+			 * parameters, after that take it back.
+			 */
 			krc_this_cpu_unlock(krcp, flags);
+			head = alloc_rcu_head_obj(ptr);
+			krcp = krc_this_cpu_lock(&flags);
 
-			head = attach_rcu_head_to_object(ptr);
-			if (head == NULL)
-				goto inline_return;
+			/*
+			 * Use emergency pool if still fails.
+			 */
+			if (head == NULL) {
+				if (!krcp->nr_emergency)
+					goto unlock_return;
 
-			/* Take it back. */
-			krcp = krc_this_cpu_lock(&flags);
+				head = set_ptr_in_rcu_head_obj(ptr,
+					krcp->emergency[--krcp->nr_emergency]);
+
+				/*
+				 * We do not need to do it. But just in case
+				 * let's set the pulled slot to NULL to avoid
+				 * magic issues.
+				 */
+				krcp->emergency[krcp->nr_emergency] = NULL;
+			}
 
 			/*
 			 * Tag the headless object. Such objects have a back-pointer

@@ -3282,7 +3333,6 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 unlock_return:
 	krc_this_cpu_unlock(krcp, flags);
 
-inline_return:
 	/*
 	 * High memory pressure, so inline kvfree() after
 	 * synchronize_rcu(). We can do it from might_sleep()

@@ -4272,6 +4322,17 @@ static void __init kfree_rcu_batch_init(void)
 		}
 
 		INIT_DELAYED_WORK(&krcp->monitor_work, kfree_rcu_monitor);
+
+		/*
+		 * The poll will be populated when low memory condition
+		 * is detected. Therefore we do not fill it in here.
+		 */
+		krcp->emergency = kmalloc_array(rcu_nr_emergency_objs,
+			sizeof(void *), GFP_NOWAIT);
+
+		if (!krcp->emergency)
+			pr_err("Failed to create emergency pool for %d CPU!\n", cpu);
+
 		krcp->initialized = true;
 	}
 	if (register_shrinker(&kfree_rcu_shrinker))

Signature · 1 line

2.20.1

Quoted 6 lines · Uladzislau Rezki (Sony)

Maintain an emergency pool for each CPU with some extra objects. There is read-only sysfs attribute, the name is "rcu_nr_emergency_objs". It reflects the size of the pool. As for now the default value is 3.

Quoted 6 lines

The pool is populated when low memory condition is detected. Please note it is only for headless case it means when the regular SLAB is not able to serve any request, the pool is used.

Signed-off-by Uladzislau Rezki (Sony) <[email protected]>

Hi Vlad,

One concern I have is this moves the problem a bit further down. My belief is we should avoid the likelihood of even needing an rcu_head allocated for the headless case, to begin with - than trying to do damage-control when it does happen. The only way we would end up needing an rcu_head is if we could not allocate an array.

So instead of adding a pool for rcu_head allocations, how do you feel about pre-allocation of the per-cpu cache array instead, which has the same effect as you are intending?

This has 3 benefits:
1. It scales with number of CPUs, no configuration needed.
2. It makes the first kfree_rcu() faster and less dependent on an allocation succeeding.
3. Much simpler code, no new structures or special handling.
4. In the future we can extend it to allocate more than 2 pages per CPU using the same caching mechanism.

The obvious drawback being its 2 pages per CPU but at least it scales by number of CPUs. Something like the following (just lightly tested):

---8<-----------------------

From: "Joel Fernandes (Google)" <[email protected]>
Subject: [PATCH] rcu/tree: Preallocate the per-cpu cache for kfree_rcu()

In recent changes, we have made it possible to use kfree_rcu() without embedding an rcu_head in the object being free'd. This requires dynamic allocation. In case dynamic allocation fails due to memory pressure, we would end up synchronously waiting for an RCU grace period thus hurting kfree_rcu() latency.

To make this less probable, let us pre-allocate the per-cpu cache so we depend less on the dynamic allocation succeeding. This also has the effect of making kfree_rcu() slightly faster at run time.

Signed-off-by Joel Fernandes (Google) <[email protected]>

---
 kernel/rcu/tree.c | 5 +++++
 1 file changed, 5 insertions(+)

kernel/rcu/tree.c +0 -0

index 6172e6296dd7d..9fbdeb4048425 100644
@@ -4251,6 +4251,11 @@ static void __init kfree_rcu_batch_init(void)
 			krcp->krw_arr[i].krcp = krcp;

Quoted 14 lines · Joel Fernandes

Quoted 6 lines · Uladzislau Rezki (Sony)

Maintain an emergency pool for each CPU with some extra objects. There is read-only sysfs attribute, the name is "rcu_nr_emergency_objs". It reflects the size of the pool. As for now the default value is 3.

Quoted 6 lines

The pool is populated when low memory condition is detected. Please note it is only for headless case it means when the regular SLAB is not able to serve any request, the pool is used.

Signed-off-by Uladzislau Rezki (Sony) <[email protected]>

Quoted 8 lines

Hi Vlad,

One concern I have is this moves the problem a bit further down. My belief is we should avoid the likelihood of even needing an rcu_head allocated for the headless case, to begin with - than trying to do damage-control when it does happen. The only way we would end up needing an rcu_head is if we could not allocate an array.

Let me share my view on all such caching. I think that now it becomes less as the issue, because of we have now https://lkml.org/lkml/2020/4/2/383 patch. I see that it does help a lot. I tried to simulate low memory condition and apply high memory pressure with that. I did not manage to trigger the "synchronize rcu" path at all. It is because of using much more permissive parameters when we request a memory from the SLAB(direct reclaim, etc...).

Quoted 4 lines

So instead of adding a pool for rcu_head allocations, how do you feel about pre-allocation of the per-cpu cache array instead, which has the same effect as you are intending?

In the v2 i have a list of such objects. It is also per-CPU(it is scaled to CPUs), but the difference is, those objects require much less memory, it is 8 + sizeof(struct rcu_head) bytes comparing to one page. Therefore the memory footprint is lower.

I have doubts that we would ever hit this emergency list, because of mentioned above patch, but from the other hand i can not say and guarantee 100%. Just in case, we may keep it.

Paul, could you please share your view and opinion? It would be appreciated :)

Quoted 39 lines

This has 3 benefits:
1. It scales with number of CPUs, no configuration needed.
2. It makes the first kfree_rcu() faster and less dependent on an allocation succeeding.
3. Much simpler code, no new structures or special handling.
4. In the future we can extend it to allocate more than 2 pages per CPU using the same caching mechanism.

The obvious drawback being its 2 pages per CPU but at least it scales by number of CPUs. Something like the following (just lightly tested):

---8<-----------------------

From: "Joel Fernandes (Google)" <[email protected]>
Subject: [PATCH] rcu/tree: Preallocate the per-cpu cache for kfree_rcu()

In recent changes, we have made it possible to use kfree_rcu() without embedding an rcu_head in the object being free'd. This requires dynamic allocation. In case dynamic allocation fails due to memory pressure, we would end up synchronously waiting for an RCU grace period thus hurting kfree_rcu() latency.

To make this less probable, let us pre-allocate the per-cpu cache so we depend less on the dynamic allocation succeeding. This also has the effect of making kfree_rcu() slightly faster at run time.

Signed-off-by Joel Fernandes (Google) <[email protected]>

---
 kernel/rcu/tree.c | 5 +++++
 1 file changed, 5 insertions(+)

kernel/rcu/tree.c +0 -0

index 6172e6296dd7d..9fbdeb4048425 100644
@@ -4251,6 +4251,11 @@ static void __init kfree_rcu_batch_init(void)
 			krcp->krw_arr[i].krcp = krcp;
 		}
 

kernel/rcu/tree.c static void __init kfree_rcu_batch_init(void) · line 4261

Quoted 8 lines

kernel/rcu/tree.c +5 -0

+		krcp->bkvcache[0] =  (struct kvfree_rcu_bulk_data *)
+					__get_free_page(GFP_NOWAIT | __GFP_NOWARN);
+		krcp->bkvcache[1] =  (struct kvfree_rcu_bulk_data *)
+					__get_free_page(GFP_NOWAIT | __GFP_NOWARN);
+
 		INIT_DELAYED_WORK(&krcp->monitor_work, kfree_rcu_monitor);
 		krcp->initialized = true;
 	}

We pre-allocate it, but differently comparing with your proposal :) I do not see how it can improve things. The difference is you do it during initializing or booting phase. In case of current code it will pre-allocate and cache one page after first calling of the kvfree_call_rcu(), say in one second. So basically both variants are the same.

But i think that we should allow to be used two pages as cached ones, no matter whether it is vmalloc ptrs. or SLAB ones. So basically, two cached pages can be used by vmalloc path and SLAB path. And probably it makes sense because of two
phases: one is when we collect pointers, second one is memory reclaim path. Thus one page per one phase, i.e. it would be paired.

Thanks, Joel!

--
Vlad Rezki

Quoted 9 lines

Quoted 9 lines

Hi Vlad,

One concern I have is this moves the problem a bit further down. My belief is we should avoid the likelihood of even needing an rcu_head allocated for the headless case, to begin with - than trying to do damage-control when it does happen. The only way we would end up needing an rcu_head is if we could not allocate an array.

Quoted 6 lines

Let me share my view on all such caching. I think that now it becomes less as the issue, because of we have now https://lkml.org/lkml/2020/4/2/383 patch. I see that it does help a lot. I tried to simulate low memory condition and apply high memory pressure with that. I did not manage to trigger the "synchronize rcu" path at all. It is because of using much more permissive parameters when we request a memory from the SLAB(direct reclaim, etc...).

That's a good sign that we don't hit this path in your tests.

I guess also, with your latest patch on releasing the lock to be in a non-atomic context, and then doing the allocation, it became even more permissive? If you drop that patch and tried, do you still not hit the synchronous path more often?

Could you also try introducing memory pressure by reducing your system's total memory and see how it behaves?

Quoted 7 lines

Quoted 4 lines

So instead of adding a pool for rcu_head allocations, how do you feel about pre-allocation of the per-cpu cache array instead, which has the same effect as you are intending?

In the v2 i have a list of such objects. It is also per-CPU(it is scaled to CPUs), but the difference is, those objects require much less memory, it is 8 + sizeof(struct rcu_head) bytes comparing to one page. Therefore the memory footprint is lower.

Yes, true. That is one drawback is it higher memory usage. But if you have at least 1 kfree_rcu() request an each CPU, then pre-allocation does not increase memory usage any more than it already has right now. Unless, we extend my proposal to cache more than 2 pages per-cpu which I think you mentioned below.

Quoted 3 lines

I have doubts that we would ever hit this emergency list, because of mentioned above patch, but from the other hand i can not say and guarantee 100%. Just in case, we may keep it.

You really have to hit OOM in your tests to trigger it I suppose. Basically the emergency pool improves situation under OOM, but otherwise does not improve it due to the direct-reclaim that happens as you mentioned. Right?

Quoted 41 lines

Paul, could you please share your view and opinion? It would be appreciated :)

Quoted 39 lines

This has 3 benefits:
1. It scales with number of CPUs, no configuration needed.
2. It makes the first kfree_rcu() faster and less dependent on an allocation succeeding.
3. Much simpler code, no new structures or special handling.
4. In the future we can extend it to allocate more than 2 pages per CPU using the same caching mechanism.

The obvious drawback being its 2 pages per CPU but at least it scales by number of CPUs. Something like the following (just lightly tested):

---8<-----------------------

From: "Joel Fernandes (Google)" <[email protected]>
Subject: [PATCH] rcu/tree: Preallocate the per-cpu cache for kfree_rcu()

In recent changes, we have made it possible to use kfree_rcu() without embedding an rcu_head in the object being free'd. This requires dynamic allocation. In case dynamic allocation fails due to memory pressure, we would end up synchronously waiting for an RCU grace period thus hurting kfree_rcu() latency.

To make this less probable, let us pre-allocate the per-cpu cache so we depend less on the dynamic allocation succeeding. This also has the effect of making kfree_rcu() slightly faster at run time.

Signed-off-by Joel Fernandes (Google) <[email protected]>

---
 kernel/rcu/tree.c | 5 +++++
 1 file changed, 5 insertions(+)

kernel/rcu/tree.c +0 -0

index 6172e6296dd7d..9fbdeb4048425 100644
@@ -4251,6 +4251,11 @@ static void __init kfree_rcu_batch_init(void)
 			krcp->krw_arr[i].krcp = krcp;
 		}
 

Quoted 13 lines

kernel/rcu/tree.c static void __init kfree_rcu_batch_init(void) · line 4261

Quoted 8 lines

kernel/rcu/tree.c +5 -0

+		krcp->bkvcache[0] =  (struct kvfree_rcu_bulk_data *)
+					__get_free_page(GFP_NOWAIT | __GFP_NOWARN);
+		krcp->bkvcache[1] =  (struct kvfree_rcu_bulk_data *)
+					__get_free_page(GFP_NOWAIT | __GFP_NOWARN);
+
 		INIT_DELAYED_WORK(&krcp->monitor_work, kfree_rcu_monitor);
 		krcp->initialized = true;
 	}

We pre-allocate it, but differently comparing with your proposal :) I do not see how it can improve things. The difference is you do it during initializing or booting phase. In case of current code it will pre-allocate and cache one page after first calling of the kvfree_call_rcu(), say in one second. So basically both variants are the same.

Well, one proposal is only 5 lines extra ;-). That has got to be at least a bit appealing ;-) ;-).

Quoted 5 lines

But i think that we should allow to be used two pages as cached ones, no matter whether it is vmalloc ptrs. or SLAB ones. So basically, two cached pages can be used by vmalloc path and SLAB path. And probably it makes sense because of two
phases: one is when we collect pointers, second one is memory reclaim path. Thus one page per one phase, i.e. it would be paired.

You are saying this with regard to my proposal right? I agree number of pages could be increased. The caching mechanism already in-place could be starting point for that extension.

Hello, Joel.

Quoted 16 lines

Quoted 15 lines

Quoted 9 lines

Hi Vlad,

One concern I have is this moves the problem a bit further down. My belief is we should avoid the likelihood of even needing an rcu_head allocated for the headless case, to begin with - than trying to do damage-control when it does happen. The only way we would end up needing an rcu_head is if we could not allocate an array.

Let me share my view on all such caching. I think that now it becomes less as the issue, because of we have now https://lkml.org/lkml/2020/4/2/383 patch. I see that it does help a lot. I tried to simulate low memory condition and apply high memory pressure with that. I did not manage to trigger the "synchronize rcu" path at all. It is because of using much more permissive parameters when we request a memory from the SLAB(direct reclaim, etc...).

Quoted 2 lines

That's a good sign that we don't hit this path in your tests.

Just one request, of course if you have a time :)
Could you please double check on your test environment to stress the system to check if you also can not hit it?

How i test it. Please apply below patch:
<snip>
t a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 5e26145e9ead..25f7ac8583e1 100644

kernel/rcu/tree.c +1 -0

@@ -3203,6 +3203,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 
        if (head) {
                ptr = (void *) head - (unsigned long) func;
+               head = NULL;
        } else {
                /*
                 * Please note there is a limitation for the head-less

kernel/rcu/tree.c +8 -3

+       /* if (!success) { */
                /* Is headless object? */
                if (head == NULL) {
                        /* Drop the lock. */
                        krc_this_cpu_unlock(krcp, flags);
 
                        head = attach_rcu_head_to_object(ptr);
-                       if (head == NULL)
+                       if (head == NULL) {
+                               success = false;
                                goto inline_return;
+                       }
 
                        /* Take it back. */
                        krcp = krc_this_cpu_lock(&flags);

@@ -3267,7 +3270,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
                 */
                expedited_drain = true;
                success = true;
-       }
+       /* } */
 
        WRITE_ONCE(krcp->count, krcp->count + 1);
 

@@ -3297,7 +3300,9 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
                if (!rcu_kfree_nowarn)
                        WARN_ON_ONCE(1);
                debug_rcu_head_unqueue(ptr);
-               synchronize_rcu();
+               /* synchronize_rcu(); */
+               printk(KERN_ERR "-> hit synchronize_rcu() path.\n");
+               trace_printk("-> hit synchronize_rcu() path.\n");
                kvfree(ptr);
        }
 }
<snip>

lower the memory size and run kfree rcu tests. It would be appreciated.

Quoted 5 lines

I guess also, with your latest patch on releasing the lock to be in a non-atomic context, and then doing the allocation, it became even more permissive? If you drop that patch and tried, do you still not hit the synchronous path more often?

Yep. If i drop the patch, i can hit it.

Quoted 3 lines

Could you also try introducing memory pressure by reducing your system's total memory and see how it behaves?

I simulated low memory condition by setting the system memory to 145MB. That was the minimum amount the KVM system was capable of booting. After that i used kfree rcu tests to simulate memory pressure.

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

So instead of adding a pool for rcu_head allocations, how do you feel about pre-allocation of the per-cpu cache array instead, which has the same effect as you are intending?

In the v2 i have a list of such objects. It is also per-CPU(it is scaled to CPUs), but the difference is, those objects require much less memory, it is 8 + sizeof(struct rcu_head) bytes comparing to one page. Therefore the memory footprint is lower.

Quoted 6 lines

Yes, true. That is one drawback is it higher memory usage. But if you have at least 1 kfree_rcu() request an each CPU, then pre-allocation does not increase memory usage any more than it already has right now. Unless, we extend my proposal to cache more than 2 pages per-cpu which I think you mentioned below.

If we cache two pages per-CPU, i think that is fine. When it comes to increasing it, it can be a bit wasting. For example consider 128 CPUs system.

Quoted 8 lines

Quoted 3 lines

I have doubts that we would ever hit this emergency list, because of mentioned above patch, but from the other hand i can not say and guarantee 100%. Just in case, we may keep it.

You really have to hit OOM in your tests to trigger it I suppose. Basically the emergency pool improves situation under OOM, but otherwise does not improve it due to the direct-reclaim that happens as you mentioned. Right?

See above how i simulated it. Direct reclaim is our GFP_KERNEL + other flags helper. If even after reclaim process there is no memory, then emergency list is supposed to be used. But we can drop this patch, i mean "emergency list" if we agree on it. The good point would be if you could stress your system by the i did. See above description :)

Quoted 6 lines

Quoted 5 lines

But i think that we should allow to be used two pages as cached ones, no matter whether it is vmalloc ptrs. or SLAB ones. So basically, two cached pages can be used by vmalloc path and SLAB path. And probably it makes sense because of two
phases: one is when we collect pointers, second one is memory reclaim path. Thus one page per one phase, i.e. it would be paired.

Quoted 4 lines

You are saying this with regard to my proposal right? I agree number of pages could be increased. The caching mechanism already in-place could be starting point for that extension.

We already have two pages. What we need is to allow to use them in both paths, vmalloc one and SLAB one, i mean reclaim path. At least that fits well to the collecting/reclaim phases.

Hi Vlad,

Quoted 18 lines · Uladzislau Rezki

Hello, Joel.

Quoted 16 lines

Quoted 15 lines

Quoted 9 lines

Hi Vlad,

One concern I have is this moves the problem a bit further down. My belief is we should avoid the likelihood of even needing an rcu_head allocated for the headless case, to begin with - than trying to do damage-control when it does happen. The only way we would end up needing an rcu_head is if we could not allocate an array.

Let me share my view on all such caching. I think that now it becomes less as the issue, because of we have now https://lkml.org/lkml/2020/4/2/383 patch. I see that it does help a lot. I tried to simulate low memory condition and apply high memory pressure with that. I did not manage to trigger the "synchronize rcu" path at all. It is because of using much more permissive parameters when we request a memory from the SLAB(direct reclaim, etc...).

Quoted 5 lines

Quoted 2 lines

That's a good sign that we don't hit this path in your tests.

Just one request, of course if you have a time :)
Could you please double check on your test environment to stress the system to check if you also can not hit it?

Sure, I am planning to do so and happy to spend time on it :) One question I had about the below test:

Quoted 17 lines

How i test it. Please apply below patch:
<snip>
t a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 5e26145e9ead..25f7ac8583e1 100644

kernel/rcu/tree.c +1 -0

@@ -3203,6 +3203,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 
        if (head) {
                ptr = (void *) head - (unsigned long) func;
+               head = NULL;
        } else {
                /*
                 * Please note there is a limitation for the head-less

@@ -3233,16 +3234,18 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
         * Under high memory pressure GFP_NOWAIT can fail,
         * in that case the emergency path is maintained.

kernel/rcu/tree.c void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) · line 3237

Quoted 4 lines

kernel/rcu/tree.c +1 -2

         */
-       success = kvfree_call_rcu_add_ptr_to_bulk(krcp, ptr);
-       if (!success) {
+       /* success = kvfree_call_rcu_add_ptr_to_bulk(krcp, ptr); */

If you drop this, then it is not realistic right? I mean it changes behavior of the code completely. We need to try to allocate array and then try to allocate the head.

Quoted 37 lines

kernel/rcu/tree.c +8 -3

+       /* if (!success) { */
                /* Is headless object? */
                if (head == NULL) {
                        /* Drop the lock. */
                        krc_this_cpu_unlock(krcp, flags);
 
                        head = attach_rcu_head_to_object(ptr);
-                       if (head == NULL)
+                       if (head == NULL) {
+                               success = false;
                                goto inline_return;
+                       }
 
                        /* Take it back. */
                        krcp = krc_this_cpu_lock(&flags);

@@ -3267,7 +3270,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
                 */
                expedited_drain = true;
                success = true;
-       }
+       /* } */
 
        WRITE_ONCE(krcp->count, krcp->count + 1);
 

@@ -3297,7 +3300,9 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
                if (!rcu_kfree_nowarn)
                        WARN_ON_ONCE(1);
                debug_rcu_head_unqueue(ptr);
-               synchronize_rcu();
+               /* synchronize_rcu(); */
+               printk(KERN_ERR "-> hit synchronize_rcu() path.\n");
+               trace_printk("-> hit synchronize_rcu() path.\n");
                kvfree(ptr);
        }
 }
<snip>

Quoted 1 line

lower the memory size and run kfree rcu tests. It would be appreciated.

I am happy to try out the diff if I can understand how the above diff is close enough with current code's behavior, if we are not using the array. One other issue with current kfree rcu tests is, the test is itself the reason for the pressure -- I believe we should also have some testing that shows that the memory pressure is caused else where (such as a real user workload causing OOM), and then we see how RCU behaves under OOM -- if we have too many synchronous latencies, does the additional caching remove such latenies under OOM? etc. I also want to look into your vmalloc tests.

Quoted 6 lines

Quoted 5 lines

I guess also, with your latest patch on releasing the lock to be in a non-atomic context, and then doing the allocation, it became even more permissive? If you drop that patch and tried, do you still not hit the synchronous path more often?

Yep. If i drop the patch, i can hit it.

Ah, cool. So basically the direct-reclaim path does the synchronous waiting, instead of synchronize_rcu(). Either way, we wait synchronously. How to chose which way is better though? If direct reclaim improves the memory situation, then we should enter that path. But if direct reclaim takes too much time (thus hurting the kfree_rcu() latency), then perhaps it is better for kfree_rcu() to just do the synchronize_rcu() and let someone else enter the direct-reclaim path. We should probably quantify and see which approach works better.

Quoted 6 lines

Quoted 3 lines

Could you also try introducing memory pressure by reducing your system's total memory and see how it behaves?

I simulated low memory condition by setting the system memory to 145MB. That was the minimum amount the KVM system was capable of booting. After that i used kfree rcu tests to simulate memory pressure.

Ah, ok. I do a similar thing. Thanks for sharing. It would be nice if we can both commit something into the tree (like modify the rcu torture KVM scripts to simulate this automatically (while also generating memory pressure external to RCU).

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

So instead of adding a pool for rcu_head allocations, how do you feel about pre-allocation of the per-cpu cache array instead, which has the same effect as you are intending?

In the v2 i have a list of such objects. It is also per-CPU(it is scaled to CPUs), but the difference is, those objects require much less memory, it is 8 + sizeof(struct rcu_head) bytes comparing to one page. Therefore the memory footprint is lower.

Quoted 8 lines

Quoted 6 lines

Yes, true. That is one drawback is it higher memory usage. But if you have at least 1 kfree_rcu() request an each CPU, then pre-allocation does not increase memory usage any more than it already has right now. Unless, we extend my proposal to cache more than 2 pages per-cpu which I think you mentioned below.

If we cache two pages per-CPU, i think that is fine. When it comes to increasing it, it can be a bit wasting. For example consider 128 CPUs system.

Right, if we could we assume that the system's memory scales with number of CPUs, it could be reasonable.

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

I have doubts that we would ever hit this emergency list, because of mentioned above patch, but from the other hand i can not say and guarantee 100%. Just in case, we may keep it.

You really have to hit OOM in your tests to trigger it I suppose. Basically the emergency pool improves situation under OOM, but otherwise does not improve it due to the direct-reclaim that happens as you mentioned. Right?

Quoted 5 lines

See above how i simulated it. Direct reclaim is our GFP_KERNEL + other flags helper. If even after reclaim process there is no memory, then emergency list is supposed to be used. But we can drop this patch, i mean "emergency list" if we agree on it. The good point would be if you could stress your system by the i did. See above description :)

Yes I will stress it and make time to do so today :)

Quoted 10 lines

Quoted 10 lines

Quoted 5 lines

But i think that we should allow to be used two pages as cached ones, no matter whether it is vmalloc ptrs. or SLAB ones. So basically, two cached pages can be used by vmalloc path and SLAB path. And probably it makes sense because of two
phases: one is when we collect pointers, second one is memory reclaim path. Thus one page per one phase, i.e. it would be paired.

You are saying this with regard to my proposal right? I agree number of pages could be increased. The caching mechanism already in-place could be starting point for that extension.

Quoted 3 lines

We already have two pages. What we need is to allow to use them in both paths, vmalloc one and SLAB one, i mean reclaim path. At least that fits well to the collecting/reclaim phases.

Ah yes, that's even better.

Quoted 47 lines · Joel Fernandes

Hi Vlad,

Quoted 18 lines · Uladzislau Rezki

Hello, Joel.

Quoted 16 lines

Quoted 15 lines

Quoted 9 lines

Hi Vlad,

One concern I have is this moves the problem a bit further down. My belief is we should avoid the likelihood of even needing an rcu_head allocated for the headless case, to begin with - than trying to do damage-control when it does happen. The only way we would end up needing an rcu_head is if we could not allocate an array.

Let me share my view on all such caching. I think that now it becomes less as the issue, because of we have now https://lkml.org/lkml/2020/4/2/383 patch. I see that it does help a lot. I tried to simulate low memory condition and apply high memory pressure with that. I did not manage to trigger the "synchronize rcu" path at all. It is because of using much more permissive parameters when we request a memory from the SLAB(direct reclaim, etc...).

Quoted 5 lines

Quoted 2 lines

That's a good sign that we don't hit this path in your tests.

Just one request, of course if you have a time :)
Could you please double check on your test environment to stress the system to check if you also can not hit it?

Sure, I am planning to do so and happy to spend time on it :) One question I had about the below test:

Quoted 17 lines

How i test it. Please apply below patch:
<snip>
t a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 5e26145e9ead..25f7ac8583e1 100644

kernel/rcu/tree.c +1 -0

@@ -3203,6 +3203,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 
        if (head) {
                ptr = (void *) head - (unsigned long) func;
+               head = NULL;
        } else {
                /*
                 * Please note there is a limitation for the head-less

@@ -3233,16 +3234,18 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
         * Under high memory pressure GFP_NOWAIT can fail,
         * in that case the emergency path is maintained.

Quoted 9 lines

kernel/rcu/tree.c void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) · line 3237

Quoted 4 lines

kernel/rcu/tree.c +1 -2

         */
-       success = kvfree_call_rcu_add_ptr_to_bulk(krcp, ptr);
-       if (!success) {
+       /* success = kvfree_call_rcu_add_ptr_to_bulk(krcp, ptr); */

If you drop this, then it is not realistic right? I mean it changes behavior of the code completely. We need to try to allocate array and then try to allocate the head.

That just bypasses an allocation for the array, to make it more simple and move forward toward the path we would like to test. Also head is set to NULL to simulated headless freeing.

Quoted 37 lines

Quoted 37 lines

kernel/rcu/tree.c +8 -3

+       /* if (!success) { */
                /* Is headless object? */
                if (head == NULL) {
                        /* Drop the lock. */
                        krc_this_cpu_unlock(krcp, flags);
 
                        head = attach_rcu_head_to_object(ptr);
-                       if (head == NULL)
+                       if (head == NULL) {
+                               success = false;
                                goto inline_return;
+                       }
 
                        /* Take it back. */
                        krcp = krc_this_cpu_lock(&flags);

@@ -3267,7 +3270,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
                 */
                expedited_drain = true;
                success = true;
-       }
+       /* } */
 
        WRITE_ONCE(krcp->count, krcp->count + 1);
 

@@ -3297,7 +3300,9 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
                if (!rcu_kfree_nowarn)
                        WARN_ON_ONCE(1);
                debug_rcu_head_unqueue(ptr);
-               synchronize_rcu();
+               /* synchronize_rcu(); */
+               printk(KERN_ERR "-> hit synchronize_rcu() path.\n");
+               trace_printk("-> hit synchronize_rcu() path.\n");
                kvfree(ptr);
        }
 }
<snip>

Quoted 11 lines

Quoted 1 line

lower the memory size and run kfree rcu tests. It would be appreciated.

I am happy to try out the diff if I can understand how the above diff is close enough with current code's behavior, if we are not using the array. One other issue with current kfree rcu tests is, the test is itself the reason for the pressure -- I believe we should also have some testing that shows that the memory pressure is caused else where (such as a real user workload causing OOM), and then we see how RCU behaves under OOM -- if we have too many synchronous latencies, does the additional caching remove such latenies under OOM? etc. I also want to look into your vmalloc tests.

Of course to have real tests would be good.

Quoted 7 lines

Quoted 6 lines

Quoted 5 lines

I guess also, with your latest patch on releasing the lock to be in a non-atomic context, and then doing the allocation, it became even more permissive? If you drop that patch and tried, do you still not hit the synchronous path more often?

Yep. If i drop the patch, i can hit it.

Quoted 9 lines

Ah, cool. So basically the direct-reclaim path does the synchronous waiting, instead of synchronize_rcu(). Either way, we wait synchronously. How to chose which way is better though? If direct reclaim improves the memory situation, then we should enter that path. But if direct reclaim takes too much time (thus hurting the kfree_rcu() latency), then perhaps it is better for kfree_rcu() to just do the synchronize_rcu() and let someone else enter the direct-reclaim path. We should probably quantify and see which approach works better.

I see at it like, headless variant has to be called from the sleeping context, therefore it can sleep. What is better to call synchronize_rcu() or doing direct reclaim depends on how many CPUs in a system we have. I suspect that doing direct reclaim is better, at least it will free some memory for us. We could also extend that patch and make it a bit different, for example do NOWAIT then try ATOMIC and as a last step do GFP_KERNEL alloc.

Hello, Joel.

Quoted 16 lines

Quoted 15 lines

Quoted 9 lines

Hi Vlad,

One concern I have is this moves the problem a bit further down. My belief is we should avoid the likelihood of even needing an rcu_head allocated for the headless case, to begin with - than trying to do damage-control when it does happen. The only way we would end up needing an rcu_head is if we could not allocate an array.

Let me share my view on all such caching. I think that now it becomes less as the issue, because of we have now https://lkml.org/lkml/2020/4/2/383 patch. I see that it does help a lot. I tried to simulate low memory condition and apply high memory pressure with that. I did not manage to trigger the "synchronize rcu" path at all. It is because of using much more permissive parameters when we request a memory from the SLAB(direct reclaim, etc...).

Quoted 2 lines

That's a good sign that we don't hit this path in your tests.

Just one request, of course if you have a time :) Could you please double check on your test environment to stress the system to check if you also can not hit it?

How i test it. Please apply below patch:

<snip>
t a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 5e26145e9ead..25f7ac8583e1 100644

kernel/rcu/tree.c +10 -5

@@ -3203,6 +3203,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 
        if (head) {
                ptr = (void *) head - (unsigned long) func;
+               head = NULL;
        } else {
                /*
                 * Please note there is a limitation for the head-less

@@ -3233,16 +3234,18 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
         * Under high memory pressure GFP_NOWAIT can fail,
         * in that case the emergency path is maintained.
         */
-       success = kvfree_call_rcu_add_ptr_to_bulk(krcp, ptr);
-       if (!success) {
+       /* success = kvfree_call_rcu_add_ptr_to_bulk(krcp, ptr); */
+       /* if (!success) { */
                /* Is headless object? */
                if (head == NULL) {
                        /* Drop the lock. */
                        krc_this_cpu_unlock(krcp, flags);
 
                        head = attach_rcu_head_to_object(ptr);
-                       if (head == NULL)
+                       if (head == NULL) {
+                               success = false;
                                goto inline_return;
+                       }
 
                        /* Take it back. */
                        krcp = krc_this_cpu_lock(&flags);

@@ -3267,7 +3270,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
                 */
                expedited_drain = true;
                success = true;
-       }
+       /* } */
 
        WRITE_ONCE(krcp->count, krcp->count + 1);
 

@@ -3297,7 +3300,9 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
                if (!rcu_kfree_nowarn)
                        WARN_ON_ONCE(1);
                debug_rcu_head_unqueue(ptr);
-               synchronize_rcu();
+               /* synchronize_rcu(); */
+               printk(KERN_ERR "-> hit synchronize_rcu() path.\n");
+               trace_printk("-> hit synchronize_rcu() path.\n");
                kvfree(ptr);
        }
 }
<snip>

lower the memory size and run kfree rcu tests. It would be appreciated.

Quoted 6 lines

I guess also, with your latest patch on releasing the lock to be in a non-atomic context, and then doing the allocation, it became even more permissive? If you drop that patch and tried, do you still not hit the synchronous path more often?

Yep. If i drop the patch, i can hit it.

Quoted 4 lines

Could you also try introducing memory pressure by reducing your system's total memory and see how it behaves?

I simulated low memory condition by setting the system memory to 145MB. That was the minimum amount the KVM system was capable of booting. After that i used kfree rcu tests to simulate memory pressure.

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

So instead of adding a pool for rcu_head allocations, how do you feel about pre-allocation of the per-cpu cache array instead, which has the same effect as you are intending?

In the v2 i have a list of such objects. It is also per-CPU(it is scaled to CPUs), but the difference is, those objects require much less memory, it is 8 + sizeof(struct rcu_head) bytes comparing to one page. Therefore the memory footprint is lower.

Quoted 6 lines

Yes, true. That is one drawback is it higher memory usage. But if you have at least 1 kfree_rcu() request an each CPU, then pre-allocation does not increase memory usage any more than it already has right now. Unless, we extend my proposal to cache more than 2 pages per-cpu which I think you mentioned below.

If we cache two pages per-CPU, i think that is fine. When it comes to increasing it, it can be a bit wasting. For example consider 128 CPUs system.

Quoted 8 lines

Quoted 3 lines

I have doubts that we would ever hit this emergency list, because of mentioned above patch, but from the other hand i can not say and guarantee 100%. Just in case, we may keep it.

You really have to hit OOM in your tests to trigger it I suppose. Basically the emergency pool improves situation under OOM, but otherwise does not improve it due to the direct-reclaim that happens as you mentioned. Right?

See above how i simulated it. Direct reclaim is our GFP_KERNEL + other flags helper. If even after reclaim process there is no memory, then emergency list is supposed to be used. But we can drop this patch, i mean "emergency list" if we agree on it. The good point would be if you could stress your system by the i did. See above description :)

Quoted 55 lines

Quoted 41 lines

Paul, could you please share your view and opinion? It would be appreciated :)

Quoted 39 lines

This has 3 benefits:
1. It scales with number of CPUs, no configuration needed.
2. It makes the first kfree_rcu() faster and less dependent on an allocation succeeding.
3. Much simpler code, no new structures or special handling.
4. In the future we can extend it to allocate more than 2 pages per CPU using the same caching mechanism.

The obvious drawback being its 2 pages per CPU but at least it scales by number of CPUs. Something like the following (just lightly tested):

---8<-----------------------

From: "Joel Fernandes (Google)" <[email protected]>
Subject: [PATCH] rcu/tree: Preallocate the per-cpu cache for kfree_rcu()

In recent changes, we have made it possible to use kfree_rcu() without embedding an rcu_head in the object being free'd. This requires dynamic allocation. In case dynamic allocation fails due to memory pressure, we would end up synchronously waiting for an RCU grace period thus hurting kfree_rcu() latency.

To make this less probable, let us pre-allocate the per-cpu cache so we depend less on the dynamic allocation succeeding. This also has the effect of making kfree_rcu() slightly faster at run time.

Signed-off-by Joel Fernandes (Google) <[email protected]>

---
 kernel/rcu/tree.c | 5 +++++
 1 file changed, 5 insertions(+)

kernel/rcu/tree.c +0 -0

index 6172e6296dd7d..9fbdeb4048425 100644
@@ -4251,6 +4251,11 @@ static void __init kfree_rcu_batch_init(void)
 			krcp->krw_arr[i].krcp = krcp;
 		}
 

Quoted 13 lines

kernel/rcu/tree.c static void __init kfree_rcu_batch_init(void) · line 4261

Quoted 8 lines

kernel/rcu/tree.c +5 -0

+		krcp->bkvcache[0] =  (struct kvfree_rcu_bulk_data *)
+					__get_free_page(GFP_NOWAIT | __GFP_NOWARN);
+		krcp->bkvcache[1] =  (struct kvfree_rcu_bulk_data *)
+					__get_free_page(GFP_NOWAIT | __GFP_NOWARN);
+
 		INIT_DELAYED_WORK(&krcp->monitor_work, kfree_rcu_monitor);
 		krcp->initialized = true;
 	}

We pre-allocate it, but differently comparing with your proposal :) I do not see how it can improve things. The difference is you do it during initializing or booting phase. In case of current code it will pre-allocate and cache one page after first calling of the kvfree_call_rcu(), say in one second. So basically both variants are the same.

Quoted 3 lines

Well, one proposal is only 5 lines extra ;-). That has got to be at least a bit appealing ;-) ;-).

:)

Quoted 10 lines

Quoted 5 lines

But i think that we should allow to be used two pages as cached ones, no matter whether it is vmalloc ptrs. or SLAB ones. So basically, two cached pages can be used by vmalloc path and SLAB path. And probably it makes sense because of two
phases: one is when we collect pointers, second one is memory reclaim path. Thus one page per one phase, i.e. it would be paired.

You are saying this with regard to my proposal right? I agree number of pages could be increased. The caching mechanism already in-place could be starting point for that extension.

We already have two pages. What we need is to allow to use them in both paths, vmalloc one and SLAB one, i mean reclaim path. At least that fits well to the collecting/reclaim phases.

Thanks for comments!

--
Vlad Rezki

Quoted 18 lines

Hello, Joel.

Quoted 16 lines

Quoted 15 lines

Quoted 9 lines

Hi Vlad,

One concern I have is this moves the problem a bit further down. My belief is we should avoid the likelihood of even needing an rcu_head allocated for the headless case, to begin with - than trying to do damage-control when it does happen. The only way we would end up needing an rcu_head is if we could not allocate an array.

Let me share my view on all such caching. I think that now it becomes less as the issue, because of we have now https://lkml.org/lkml/2020/4/2/383 patch. I see that it does help a lot. I tried to simulate low memory condition and apply high memory pressure with that. I did not manage to trigger the "synchronize rcu" path at all. It is because of using much more permissive parameters when we request a memory from the SLAB(direct reclaim, etc...).

Quoted 7 lines

Quoted 2 lines

That's a good sign that we don't hit this path in your tests.

Just one request, of course if you have a time :) Could you please double check on your test environment to stress the system to check if you also can not hit it?

How i test it. Please apply below patch:

This is of course a double challenge.

I can assure you that even if we cannot make it happen in the comfort and safety of our tests systems, someone somewhere will make it happen all the time. Because there is a very large number of Linux systems running out there.

Which leads to the other challenge: How do we test this code path?