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
