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

[Uladzislau Rezki]

On Tue, Feb 09, 2021 at 05:00:52PM -0800, Paul E. McKenney wrote:
> 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();
>
Sorry for my late answer. I got it differently as we discussed offline.
Please see below the v3. Hope we are on the same page now :)