[PATCH 1/3] rcu/tree: use more permissive parameters when attaching a head

[PATCH 1/3] rcu/tree: use more permissive parameters when attaching a head

[Uladzislau Rezki (Sony)]

It is documneted that a headless object can be reclaimed from
might_sleep() context only. Because of that when a head is
dynamically attached it makes sense to drop the lock and do
an allocation with much more permissve flags comparing if it
is done from atomic context.

That is why use GFP_KERNEL flag plus some extra ones which
would make an allocation most likely to be succeed. The big
advantage of doing so is a direct reclaim process.

Tested such approach on my local tiny system with 145MB of
ram(the minimum amount the KVM system is capable of booting)
and 4xCPUs. For stressing the rcuperf module was used. During
tests with difference combinations i did not observe any hit
of our last emergency case, when synchronize_rcu() is involved.

Please note, the "dynamically attaching" path was enabled only,
apart of that all types of objects were considered as headless
variant during testing.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
Suggested-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/tree.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 6172e6296dd7..24f620a06219 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3148,13 +3148,10 @@ static inline struct rcu_head *attach_rcu_head_to_object(void *obj)
 {
 	unsigned long *ptr;
 
+	/* Try hard to get the memory. */
 	ptr = kmalloc(sizeof(unsigned long *) +
-			sizeof(struct rcu_head), GFP_NOWAIT | __GFP_NOWARN);
-
-	if (!ptr)
-		ptr = kmalloc(sizeof(unsigned long *) +
-				sizeof(struct rcu_head), GFP_ATOMIC | __GFP_NOWARN);
-
+		sizeof(struct rcu_head), GFP_KERNEL |
+			__GFP_ATOMIC | __GFP_HIGH | __GFP_RETRY_MAYFAIL);
 	if (!ptr)
 		return NULL;
 
@@ -3222,9 +3219,20 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 	if (!success) {
 		/* Is headless object? */
 		if (head == NULL) {
+			/* Drop the lock. */
+			if (krcp->initialized)
+				spin_unlock(&krcp->lock);
+			local_irq_restore(flags);
+
 			head = attach_rcu_head_to_object(ptr);
 			if (head == NULL)
-				goto unlock_return;
+				goto inline_return;
+
+			/* Take it back. */
+			local_irq_save(flags);
+			krcp = this_cpu_ptr(&krc);
+			if (krcp->initialized)
+				spin_lock(&krcp->lock);
 
 			/*
 			 * Tag the headless object. Such objects have a back-pointer
@@ -3263,6 +3271,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 		spin_unlock(&krcp->lock);
 	local_irq_restore(flags);
 
+inline_return:
 	/*
 	 * High memory pressure, so inline kvfree() after
 	 * synchronize_rcu(). We can do it from might_sleep()
-- 
2.20.1

[PATCH 2/3] rcu/tree: move locking/unlocking to separate functions

[Uladzislau Rezki (Sony)]

Introduce two helpers to lock and unlock an access to the
per-cpu "kfree_rcu_cpu" structure. The reason is to make
kvfree_call_rcu() function to be more readable.

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

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 24f620a06219..5e26145e9ead 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3159,6 +3159,27 @@ static inline struct rcu_head *attach_rcu_head_to_object(void *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);
+}
+
 /*
  * Queue a request for lazy invocation of appropriate free routine after a
  * grace period. Please note there are three paths are maintained, two are the
@@ -3195,10 +3216,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 		ptr = (unsigned long *) func;
 	}
 
-	local_irq_save(flags);	// For safely calling this_cpu_ptr().
-	krcp = this_cpu_ptr(&krc);
-	if (krcp->initialized)
-		spin_lock(&krcp->lock);
+	krcp = krc_this_cpu_lock(&flags);
 
 	// Queue the object but don't yet schedule the batch.
 	if (debug_rcu_head_queue(ptr)) {
@@ -3220,19 +3238,14 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 		/* Is headless object? */
 		if (head == NULL) {
 			/* Drop the lock. */
-			if (krcp->initialized)
-				spin_unlock(&krcp->lock);
-			local_irq_restore(flags);
+			krc_this_cpu_unlock(krcp, flags);
 
 			head = attach_rcu_head_to_object(ptr);
 			if (head == NULL)
 				goto inline_return;
 
 			/* Take it back. */
-			local_irq_save(flags);
-			krcp = this_cpu_ptr(&krc);
-			if (krcp->initialized)
-				spin_lock(&krcp->lock);
+			krcp = krc_this_cpu_lock(&flags);
 
 			/*
 			 * Tag the headless object. Such objects have a back-pointer
@@ -3267,9 +3280,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 	}
 
 unlock_return:
-	if (krcp->initialized)
-		spin_unlock(&krcp->lock);
-	local_irq_restore(flags);
+	krc_this_cpu_unlock(krcp, flags);
 
 inline_return:
 	/*
-- 
2.20.1

[PATCH 3/3] lib/test_vmalloc.c: introduce two new test cases

[Uladzislau Rezki (Sony)]

Introduce two more test cases which are specific for RCU
freeing of vmalloc pointer. One test case is for object
that has rcu_head inside and second one is for headless
testing.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 lib/test_vmalloc.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/lib/test_vmalloc.c b/lib/test_vmalloc.c
index 8bbefcaddfe8..1734ba7fc400 100644
--- a/lib/test_vmalloc.c
+++ b/lib/test_vmalloc.c
@@ -15,6 +15,7 @@
 #include <linux/delay.h>
 #include <linux/rwsem.h>
 #include <linux/mm.h>
+#include <linux/rcupdate.h>
 
 #define __param(type, name, init, msg)		\
 	static type name = init;				\
@@ -43,6 +44,8 @@ __param(int, run_test_mask, INT_MAX,
 		"\t\tid: 32,  name: random_size_align_alloc_test\n"
 		"\t\tid: 64,  name: align_shift_alloc_test\n"
 		"\t\tid: 128, name: pcpu_alloc_test\n"
+		"\t\tid: 256, name: kvfree_rcu_with_head_test\n"
+		"\t\tid: 512, name: kvfree_rcu_head_less_test\n"
 		/* Add a new test case description here. */
 );
 
@@ -328,6 +331,49 @@ pcpu_alloc_test(void)
 	return rv;
 }
 
+struct test_kvfree_rcu {
+	struct rcu_head rcu;
+	unsigned char array[100];
+};
+
+static int
+kvfree_rcu_with_head_test(void)
+{
+	struct test_kvfree_rcu *p;
+	int i;
+
+	for (i = 0; i < test_loop_count; i++) {
+		p = vmalloc(1 * PAGE_SIZE);
+
+		if (!p)
+			return -1;
+
+		p->array[0] = 'a';
+		kvfree_rcu(p, rcu);
+	}
+
+	return 0;
+}
+
+static int
+kvfree_rcu_head_less_test(void)
+{
+	struct test_kvfree_rcu *p;
+	int i;
+
+	for (i = 0; i < test_loop_count; i++) {
+		p = vmalloc(1 * PAGE_SIZE);
+
+		if (!p)
+			return -1;
+
+		p->array[0] = 'a';
+		kvfree_rcu(p);
+	}
+
+	return 0;
+}
+
 struct test_case_desc {
 	const char *test_name;
 	int (*test_func)(void);
@@ -342,6 +388,8 @@ static struct test_case_desc test_case_array[] = {
 	{ "random_size_align_alloc_test", random_size_align_alloc_test },
 	{ "align_shift_alloc_test", align_shift_alloc_test },
 	{ "pcpu_alloc_test", pcpu_alloc_test },
+	{ "kvfree_rcu_with_head_test", kvfree_rcu_with_head_test },
+	{ "kvfree_rcu_head_less_test", kvfree_rcu_head_less_test },
 	/* Add a new test case here. */
 };
 
-- 
2.20.1

Re: [PATCH 1/3] rcu/tree: use more permissive parameters when attaching a head

[Joel Fernandes]

On Thu, Apr 02, 2020 at 02:32:51PM +0200, Uladzislau Rezki (Sony) wrote:
> It is documneted that a headless object can be reclaimed from
> might_sleep() context only. Because of that when a head is
> dynamically attached it makes sense to drop the lock and do
> an allocation with much more permissve flags comparing if it
> is done from atomic context.
> 
> That is why use GFP_KERNEL flag plus some extra ones which
> would make an allocation most likely to be succeed. The big
> advantage of doing so is a direct reclaim process.
> 
> Tested such approach on my local tiny system with 145MB of
> ram(the minimum amount the KVM system is capable of booting)
> and 4xCPUs. For stressing the rcuperf module was used. During
> tests with difference combinations i did not observe any hit
> of our last emergency case, when synchronize_rcu() is involved.
> 
> Please note, the "dynamically attaching" path was enabled only,
> apart of that all types of objects were considered as headless
> variant during testing.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> Suggested-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Beautifully done, thanks Vlad! I agree with this and the other 2 changes and
I applied it to my rcu/kfree branch for further testing by both of us.

thanks,

 - Joel

> ---
>  kernel/rcu/tree.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 6172e6296dd7..24f620a06219 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3148,13 +3148,10 @@ static inline struct rcu_head *attach_rcu_head_to_object(void *obj)
>  {
>  	unsigned long *ptr;
>  
> +	/* Try hard to get the memory. */
>  	ptr = kmalloc(sizeof(unsigned long *) +
> -			sizeof(struct rcu_head), GFP_NOWAIT | __GFP_NOWARN);
> -
> -	if (!ptr)
> -		ptr = kmalloc(sizeof(unsigned long *) +
> -				sizeof(struct rcu_head), GFP_ATOMIC | __GFP_NOWARN);
> -
> +		sizeof(struct rcu_head), GFP_KERNEL |
> +			__GFP_ATOMIC | __GFP_HIGH | __GFP_RETRY_MAYFAIL);
>  	if (!ptr)
>  		return NULL;
>  
> @@ -3222,9 +3219,20 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
>  	if (!success) {
>  		/* Is headless object? */
>  		if (head == NULL) {
> +			/* Drop the lock. */
> +			if (krcp->initialized)
> +				spin_unlock(&krcp->lock);
> +			local_irq_restore(flags);
> +
>  			head = attach_rcu_head_to_object(ptr);
>  			if (head == NULL)
> -				goto unlock_return;
> +				goto inline_return;
> +
> +			/* Take it back. */
> +			local_irq_save(flags);
> +			krcp = this_cpu_ptr(&krc);
> +			if (krcp->initialized)
> +				spin_lock(&krcp->lock);
>  
>  			/*
>  			 * Tag the headless object. Such objects have a back-pointer
> @@ -3263,6 +3271,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
>  		spin_unlock(&krcp->lock);
>  	local_irq_restore(flags);
>  
> +inline_return:
>  	/*
>  	 * High memory pressure, so inline kvfree() after
>  	 * synchronize_rcu(). We can do it from might_sleep()
> -- 
> 2.20.1
> 

Re: [PATCH 1/3] rcu/tree: use more permissive parameters when attaching a head

[Joel Fernandes]

On Thu, Apr 02, 2020 at 02:32:51PM +0200, Uladzislau Rezki (Sony) wrote:
> It is documneted that a headless object can be reclaimed from
> might_sleep() context only. Because of that when a head is
> dynamically attached it makes sense to drop the lock and do
> an allocation with much more permissve flags comparing if it
> is done from atomic context.
> 
> That is why use GFP_KERNEL flag plus some extra ones which
> would make an allocation most likely to be succeed. The big
> advantage of doing so is a direct reclaim process.
> 
> Tested such approach on my local tiny system with 145MB of
> ram(the minimum amount the KVM system is capable of booting)
> and 4xCPUs. For stressing the rcuperf module was used. During
> tests with difference combinations i did not observe any hit
> of our last emergency case, when synchronize_rcu() is involved.
> 
> Please note, the "dynamically attaching" path was enabled only,
> apart of that all types of objects were considered as headless
> variant during testing.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> Suggested-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
>  kernel/rcu/tree.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 6172e6296dd7..24f620a06219 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3148,13 +3148,10 @@ static inline struct rcu_head *attach_rcu_head_to_object(void *obj)
>  {
>  	unsigned long *ptr;
>  
> +	/* Try hard to get the memory. */
>  	ptr = kmalloc(sizeof(unsigned long *) +
> -			sizeof(struct rcu_head), GFP_NOWAIT | __GFP_NOWARN);
> -
> -	if (!ptr)
> -		ptr = kmalloc(sizeof(unsigned long *) +
> -				sizeof(struct rcu_head), GFP_ATOMIC | __GFP_NOWARN);
> -
> +		sizeof(struct rcu_head), GFP_KERNEL |
> +			__GFP_ATOMIC | __GFP_HIGH | __GFP_RETRY_MAYFAIL);

On thing here though, you removed the NOWARN. Was there a reason? It would
now warn even when synchronously waiting right? I will fixup your commit to
add it back for now but let me know if you had some other reason to remove it.

thanks,

 - Joel


>  	if (!ptr)
>  		return NULL;
>  
> @@ -3222,9 +3219,20 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
>  	if (!success) {
>  		/* Is headless object? */
>  		if (head == NULL) {
> +			/* Drop the lock. */
> +			if (krcp->initialized)
> +				spin_unlock(&krcp->lock);
> +			local_irq_restore(flags);
> +
>  			head = attach_rcu_head_to_object(ptr);
>  			if (head == NULL)
> -				goto unlock_return;
> +				goto inline_return;
> +
> +			/* Take it back. */
> +			local_irq_save(flags);
> +			krcp = this_cpu_ptr(&krc);
> +			if (krcp->initialized)
> +				spin_lock(&krcp->lock);
>  
>  			/*
>  			 * Tag the headless object. Such objects have a back-pointer
> @@ -3263,6 +3271,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
>  		spin_unlock(&krcp->lock);
>  	local_irq_restore(flags);
>  
> +inline_return:
>  	/*
>  	 * High memory pressure, so inline kvfree() after
>  	 * synchronize_rcu(). We can do it from might_sleep()
> -- 
> 2.20.1
>