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) <ure...@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)" <ure...@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) <ure...@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
