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

[Uladzislau Rezki <urezki@gmail.com>]

On Tue, Feb 16, 2021 at 09:35:02AM -0800, Paul E. McKenney wrote:
> On Mon, Feb 15, 2021 at 05:27:05PM +0100, Uladzislau Rezki wrote:
> > 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@gmail.com>
> > > > > 
> > > > > 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@gmail.com>
> > > > 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@gmail.com>
> > > > ---
> > > >  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 :)
> 
> This does look good to me!  Could you please send it as an email
> containing only the patch, just to make it official?  And to catch the
> attention of anyone who might have tuned out of this email thread.  ;-)
> 
I will send out as a fresh patch :)

--
Vlad Rezki