Re: [PATCH 1/2] rcuscale: add kfree_rcu() single-argument scale test

[Paul E. McKenney]

On Tue, Feb 09, 2021 at 09:13:43PM +0100, Uladzislau Rezki wrote:
> On Thu, Feb 04, 2021 at 01:46:48PM -0800, Paul E. McKenney wrote:
> > On Fri, Jan 29, 2021 at 09:05:04PM +0100, Uladzislau Rezki (Sony) wrote:
> > > To stress and test a single argument of kfree_rcu() call, we
> > > should to have a special coverage for it. We used to have it
> > > in the test-suite related to vmalloc stressing. The reason is
> > > the rcuscale is a correct place for RCU related things.
> > > 
> > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@xxxxxxxxx>
> > 
> > This is a great addition, but it would be even better if there was
> > a way to say "test both in one run".  One way to do this is to have
> > torture_param() variables for both kfree_rcu_test_single and (say)
> > kfree_rcu_test_double, both bool and both initialized to false.  If both
> > have the same value (false or true) both are tested, otherwise only
> > the one with value true is tested.  The value of this is that it allows
> > testing of both options with one test.
> > 
> Make sense to me :)
> 
> >From ba083a543a123455455c81230b7b5a9aa2a9cb7f Mon Sep 17 00:00:00 2001
> From: "Uladzislau Rezki (Sony)" <urezki@xxxxxxxxx>
> Date: Fri, 29 Jan 2021 19:51:27 +0100
> Subject: [PATCH v2 1/1] rcuscale: add kfree_rcu() single-argument scale test
> 
> To stress and test a single argument of kfree_rcu() call, we
> should to have a special coverage for it. We used to have it
> in the test-suite related to vmalloc stressing. The reason is
> the rcuscale is a correct place for RCU related things.
> 
> Therefore introduce two torture_param() variables, one is for
> single-argument scale test and another one for double-argument
> scale test.
> 
> By default kfree_rcu_test_single and kfree_rcu_test_double are
> initialized to false. If both have the same value (false or true)
> both are tested in one run, otherwise only the one with value
> true is tested. The value of this is that it allows testing of
> both options with one test.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@xxxxxxxxx>
> ---
>  kernel/rcu/rcuscale.c | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/rcu/rcuscale.c b/kernel/rcu/rcuscale.c
> index 06491d5530db..0cde5c17f06c 100644
> --- a/kernel/rcu/rcuscale.c
> +++ b/kernel/rcu/rcuscale.c
> @@ -625,6 +625,8 @@ rcu_scale_shutdown(void *arg)
>  torture_param(int, kfree_nthreads, -1, "Number of threads running loops of kfree_rcu().");
>  torture_param(int, kfree_alloc_num, 8000, "Number of allocations and frees done in an iteration.");
>  torture_param(int, kfree_loops, 10, "Number of loops doing kfree_alloc_num allocations and frees.");
> +torture_param(int, kfree_rcu_test_single, 0, "Do we run a kfree_rcu() single-argument scale test?");
> +torture_param(int, kfree_rcu_test_double, 0, "Do we run a kfree_rcu() double-argument scale test?");

Good!  But why int instead of bool?

>  static struct task_struct **kfree_reader_tasks;
>  static int kfree_nrealthreads;
> @@ -641,7 +643,7 @@ kfree_scale_thread(void *arg)
>  {
>  	int i, loop = 0;
>  	long me = (long)arg;
> -	struct kfree_obj *alloc_ptr;
> +	struct kfree_obj *alloc_ptr[2];

You lost me on this one...

>  	u64 start_time, end_time;
>  	long long mem_begin, mem_during = 0;
>  
> @@ -665,12 +667,33 @@ kfree_scale_thread(void *arg)
>  			mem_during = (mem_during + si_mem_available()) / 2;
>  		}
>  
> +		// By default kfree_rcu_test_single and kfree_rcu_test_double are
> +		// initialized to false. If both have the same value (false or true)
> +		// both are tested in one run, otherwise only the one with value
> +		// true is tested.
>  		for (i = 0; i < kfree_alloc_num; i++) {
> -			alloc_ptr = kmalloc(kfree_mult * sizeof(struct kfree_obj), GFP_KERNEL);
> -			if (!alloc_ptr)
> -				return -ENOMEM;
> +			alloc_ptr[0] = kmalloc(kfree_mult * sizeof(struct kfree_obj), GFP_KERNEL);
> +			alloc_ptr[1] = (kfree_rcu_test_single == kfree_rcu_test_double) ?
> +				kmalloc(kfree_mult * sizeof(struct kfree_obj), GFP_KERNEL) : NULL;
> +
> +			// 0 ptr. is freed either over single or double argument.
> +			if (alloc_ptr[0]) {
> +				if (kfree_rcu_test_single == kfree_rcu_test_double ||
> +						kfree_rcu_test_single) {
> +					kfree_rcu(alloc_ptr[0]);
> +				} else {
> +					kfree_rcu(alloc_ptr[0], rh);
> +				}
> +			}
> +
> +			// 1 ptr. is always freed over double argument.
> +			if (alloc_ptr[1])
> +				kfree_rcu(alloc_ptr[1], rh);
>  
> -			kfree_rcu(alloc_ptr, rh);
> +			if (!alloc_ptr[0] ||
> +					(kfree_rcu_test_single == kfree_rcu_test_double &&
> +						!alloc_ptr[1]))
> +				return -ENOMEM;

How about something like this?

	bool krts = kfree_rcu_test_single || kfree_rcu_test_single == kfree_rcu_test_double;
	bool krtd = kfree_rcu_test_double || kfree_rcu_test_single == kfree_rcu_test_double;
	bool krtb = kfree_rcu_test_single && kfree_rcu_test_double;
	DEFINE_TORTURE_RANDOM(tr);

	...

			alloc_ptr = kmalloc(kfree_mult * sizeof(struct kfree_obj), GFP_KERNEL);
			if (!alloc_ptr)
				return -ENOMEM;
			if (krtd || (krtb && (torture_random(&tr) & 0x800)))
				kfree_rcu(alloc_ptr, rh);
			else
				kfree_rcu(alloc_ptr);

>  		}
>  
>  		cond_resched();

And this is why I was so confused about the earlier OOMs.  We need
something stronger, and not here, but rather inside the above loop.
The function rcu_torture_fwd_prog_cond_resched() does what is needed,
which needs to be moved to kernel/torture.c or to be a static inline in
include/linux/torture.h so that it can be invoked here.

The flooding we are looking to emulate has to have frequent trips into
userspace, and rcu_torture_fwd_prog_cond_resched() is the way that we
emulate those trips.

But please make this change be a separate patch.

							Thanx, Paul