Re: [PATCHv2 2/2] lib/percpu_test: Add extra tests in percpu_test

From: Dennis Zhou
Date: Mon Aug 23 2021 - 22:14:07 EST

Next message: Andi Kleen: "Re: [PATCH v4 11/15] pci: Add pci_iomap_shared{,_range}"
Previous message: Leo Yan: "Re: [PATCH v1 2/3] perf auxtrace: Add compat_auxtrace_mmap__{read_head|write_tail}"
In reply to: Ritesh Harjani: "[PATCHv2 2/2] lib/percpu_test: Add extra tests in percpu_test"
Next in thread: Dennis Zhou: "Re: [PATCHv2 1/2] kernel/workqueue: Make schedule_on_each_cpu as EXPORT_SYMBOL_GPL"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hello,

On Tue, Aug 24, 2021 at 02:12:30AM +0530, Ritesh Harjani wrote:
> While debugging a issue, we needed to stress test the percpu alloc/free
> path. Hence added some tests in lib/percpu_test to stress test
> percpu subsystem for allocation with different sizes.
>

Can you explain more about the problem you are trying to debug and why
it required stressing the percpu allocator?

> This patch keeps the default behavior of insmod module same for default
> test. But when given insmod with different option, it can run a
> percpu_stressd daemon (percpu_test_num=2) which does a stress test
> evey 10secs unless the module is unloaded.
>
> We found this to be helpful in our testing, since with this we could
> easily excercise percpu allo/free path. Hence cleaned this up for
> inclusion in percpu_test module.
>
> Logs
> ======
> qemu-> sudo insmod /mnt/percpu_test.ko percpu_test_num=0
> [ 334.362973] percpu_test: INIT, interval: 1000, max_shift: 13, run_tests: percpu_verify
> [ 334.364946] TEST Starts: percpu_verify
> [ 334.365601] TEST Completed: percpu_verify
> insmod: ERROR: could not insert module /mnt/percpu_test.ko: Resource temporarily unavailable

^ this seems wrong? What am I missing.

>
> qemu-> sudo insmod /mnt/percpu_test.ko percpu_test_num=1
> [ 336.556464] percpu_test: INIT, interval: 1000, max_shift: 13, run_tests: percpu_stress
> [ 336.558388] TEST Starts: percpu_stress
> [ 336.560611] TEST Completed: percpu_stress
> insmod: ERROR: could not insert module /mnt/percpu_test.ko: Resource temporarily unavailable
>
> qemu-> sudo insmod /mnt/percpu_test.ko percpu_test_num=2
> [ 339.164406] percpu_test: INIT, interval: 1000, max_shift: 13, run_tests: percpu_stressd
> [ 339.165935] TEST Starts: percpu_stressd
> [ 339.167033] TEST Completed: percpu_stressd
> [ 339.167082] DAEMON: starts percpu_stressd
> [ 339.168498] TEST Starts: percpu_stressd: iter (1)
> [ 339.182530] TEST Completed: percpu_stressd: iter (1)
> [ 349.341109] TEST Starts: percpu_stressd: iter (2)
> [ 349.344447] TEST Completed: percpu_stressd: iter (2)
> [ 359.580829] TEST Starts: percpu_stressd: iter (3)
> [ 359.584315] TEST Completed: percpu_stressd: iter (3)
> [ 369.820471] TEST Starts: percpu_stressd: iter (4)
> [ 369.844402] TEST Completed: percpu_stressd: iter (4)
>
> qemu-> sudo rmmod percpu_test
> [ 375.001098] percpu_test: EXIT
> [qemu][~]
>
> Cc: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxx>
> Cc: Vaibhav Jain <vaibhav@xxxxxxxxxxxxx>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Signed-off-by: Ritesh Harjani <riteshh@xxxxxxxxxxxxx>
> ---
> [v1 -> v2]: Fix warnings from kernel test robot
>
> lib/percpu_test.c | 240 ++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 191 insertions(+), 49 deletions(-)
>
> diff --git a/lib/percpu_test.c b/lib/percpu_test.c
> index 4a3d70bbc1a0..68c57c288dc6 100644
> --- a/lib/percpu_test.c
> +++ b/lib/percpu_test.c
> @@ -1,4 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/workqueue.h>
> +#include <linux/kthread.h>
> +#include <linux/cpu.h>
> #include <linux/module.h>
>
> /* validate @native and @pcp counter values match @expected */
> @@ -14,10 +17,25 @@
> (long long)(expected), (long long)(expected)); \
> } while (0)
>
> -static DEFINE_PER_CPU(long, long_counter);
> -static DEFINE_PER_CPU(unsigned long, ulong_counter);

^ These are good to keep fwiw as it's one of the few instances of using
the reserved region of the first chunk of percpu memory.

> +/* upto max alloc size tests for percpu var */
> +static char __percpu *counters[1 << PAGE_SHIFT];
> +static struct task_struct *percpu_stressd_thread;
>
> -static int __init percpu_test_init(void)
> +/* let's not trigger OOM */
> +int percpu_alloc_max_size_shift = PAGE_SHIFT - 3;
> +module_param(percpu_alloc_max_size_shift, int, 0644);
> +MODULE_PARM_DESC(percpu_alloc_max_size_shift, "max size of allocation in stress test will be upto 1 << percpu_alloc_max_size_shift");
> +
> +static long percpu_stressd_interval = 1 * 10 * HZ;
> +module_param(percpu_stressd_interval, long, 0644);
> +MODULE_PARM_DESC(percpu_stressd_interval, "percpu_stressd internal");
> +
> +/* keep the default test same */
> +static int percpu_test_num;
> +module_param(percpu_test_num, int, 0644);
> +MODULE_PARM_DESC(percpu_test_num, "Test number percpu_test_num");
> +
> +static int percpu_test_verify(void)
> {
> /*
> * volatile prevents compiler from optimizing it uses, otherwise the
> @@ -26,109 +44,233 @@ static int __init percpu_test_init(void)
> volatile unsigned int ui_one = 1;
> long l = 0;
> unsigned long ul = 0;
> + long __percpu *long_counter = alloc_percpu(long);
> + unsigned long __percpu *ulong_counter = alloc_percpu(unsigned long);
>
> - pr_info("percpu test start\n");
> + if (!long_counter || !ulong_counter)
> + goto out;
> +
> + pr_debug("percpu_test: %s start cpu: %d\n", __func__, smp_processor_id());
>
> preempt_disable();
>
> l += -1;
> - __this_cpu_add(long_counter, -1);
> - CHECK(l, long_counter, -1);
> + __this_cpu_add(*long_counter, -1);
> + CHECK(l, *long_counter, -1);
>
> l += 1;
> - __this_cpu_add(long_counter, 1);
> - CHECK(l, long_counter, 0);
> + __this_cpu_add(*long_counter, 1);
> + CHECK(l, *long_counter, 0);
>
> ul = 0;
> - __this_cpu_write(ulong_counter, 0);
> + __this_cpu_write(*ulong_counter, 0);
>
> ul += 1UL;
> - __this_cpu_add(ulong_counter, 1UL);
> - CHECK(ul, ulong_counter, 1);
> + __this_cpu_add(*ulong_counter, 1UL);
> + CHECK(ul, *ulong_counter, 1);
>
> ul += -1UL;
> - __this_cpu_add(ulong_counter, -1UL);
> - CHECK(ul, ulong_counter, 0);
> + __this_cpu_add(*ulong_counter, -1UL);
> + CHECK(ul, *ulong_counter, 0);
>
> ul += -(unsigned long)1;
> - __this_cpu_add(ulong_counter, -(unsigned long)1);
> - CHECK(ul, ulong_counter, -1);
> + __this_cpu_add(*ulong_counter, -(unsigned long)1);
> + CHECK(ul, *ulong_counter, -1);
>
> ul = 0;
> - __this_cpu_write(ulong_counter, 0);
> + __this_cpu_write(*ulong_counter, 0);
>
> ul -= 1;
> - __this_cpu_dec(ulong_counter);
> - CHECK(ul, ulong_counter, -1);
> - CHECK(ul, ulong_counter, ULONG_MAX);
> + __this_cpu_dec(*ulong_counter);
> + CHECK(ul, *ulong_counter, -1);
> + CHECK(ul, *ulong_counter, ULONG_MAX);
>
> l += -ui_one;
> - __this_cpu_add(long_counter, -ui_one);
> - CHECK(l, long_counter, 0xffffffff);
> + __this_cpu_add(*long_counter, -ui_one);
> + CHECK(l, *long_counter, 0xffffffff);
>
> l += ui_one;
> - __this_cpu_add(long_counter, ui_one);
> - CHECK(l, long_counter, (long)0x100000000LL);
> + __this_cpu_add(*long_counter, ui_one);
> + CHECK(l, *long_counter, (long)0x100000000LL);
>
>
> l = 0;
> - __this_cpu_write(long_counter, 0);
> + __this_cpu_write(*long_counter, 0);
>
> l -= ui_one;
> - __this_cpu_sub(long_counter, ui_one);
> - CHECK(l, long_counter, -1);
> + __this_cpu_sub(*long_counter, ui_one);
> + CHECK(l, *long_counter, -1);
>
> l = 0;
> - __this_cpu_write(long_counter, 0);
> + __this_cpu_write(*long_counter, 0);
>
> l += ui_one;
> - __this_cpu_add(long_counter, ui_one);
> - CHECK(l, long_counter, 1);
> + __this_cpu_add(*long_counter, ui_one);
> + CHECK(l, *long_counter, 1);
>
> l += -ui_one;
> - __this_cpu_add(long_counter, -ui_one);
> - CHECK(l, long_counter, (long)0x100000000LL);
> + __this_cpu_add(*long_counter, -ui_one);
> + CHECK(l, *long_counter, (long)0x100000000LL);
>
> l = 0;
> - __this_cpu_write(long_counter, 0);
> + __this_cpu_write(*long_counter, 0);
>
> l -= ui_one;
> - this_cpu_sub(long_counter, ui_one);
> - CHECK(l, long_counter, -1);
> - CHECK(l, long_counter, ULONG_MAX);
> + this_cpu_sub(*long_counter, ui_one);
> + CHECK(l, *long_counter, -1);
> + CHECK(l, *long_counter, ULONG_MAX);
>
> ul = 0;
> - __this_cpu_write(ulong_counter, 0);
> + __this_cpu_write(*ulong_counter, 0);
>
> ul += ui_one;
> - __this_cpu_add(ulong_counter, ui_one);
> - CHECK(ul, ulong_counter, 1);
> + __this_cpu_add(*ulong_counter, ui_one);
> + CHECK(ul, *ulong_counter, 1);
>
> ul = 0;
> - __this_cpu_write(ulong_counter, 0);
> + __this_cpu_write(*ulong_counter, 0);
>
> ul -= ui_one;
> - __this_cpu_sub(ulong_counter, ui_one);
> - CHECK(ul, ulong_counter, -1);
> - CHECK(ul, ulong_counter, ULONG_MAX);
> + __this_cpu_sub(*ulong_counter, ui_one);
> + CHECK(ul, *ulong_counter, -1);
> + CHECK(ul, *ulong_counter, ULONG_MAX);
>
> ul = 3;
> - __this_cpu_write(ulong_counter, 3);
> + __this_cpu_write(*ulong_counter, 3);
>
> - ul = this_cpu_sub_return(ulong_counter, ui_one);
> - CHECK(ul, ulong_counter, 2);
> + ul = this_cpu_sub_return(*ulong_counter, ui_one);
> + CHECK(ul, *ulong_counter, 2);
>
> - ul = __this_cpu_sub_return(ulong_counter, ui_one);
> - CHECK(ul, ulong_counter, 1);
> + ul = __this_cpu_sub_return(*ulong_counter, ui_one);
> + CHECK(ul, *ulong_counter, 1);
>
> preempt_enable();
>
> - pr_info("percpu test done\n");
> - return -EAGAIN; /* Fail will directly unload the module */
> +out:
> + free_percpu(long_counter);
> + free_percpu(ulong_counter);
> + pr_debug("percpu_test: %s done cpu: %d\n", __func__, smp_processor_id());
> +
> + /*
> + * Keep the default functionality same.
> + * Fail will directly unload this module.
> + */
> + return -EAGAIN;
> +}
> +
> +static void percpu_test_verify_work(struct work_struct *work)
> +{
> + percpu_test_verify();
> +}
> +
> +static int percpu_test_stress(void)
> +{
> + int i;
> +
> + for (i = 1; i < (1 << percpu_alloc_max_size_shift); i++) {
> + size_t size = i;
> +
> + if (size > PCPU_MIN_ALLOC_SIZE)
> + break;
> + counters[i] = (char __percpu *)__alloc_percpu(size, __alignof__(char));
> + if (!counters[i])
> + break;
> + cond_resched();
> + }
> +
> + schedule_on_each_cpu(percpu_test_verify_work);

I'm not understanding the value of scheduling on each cpu here? Each
CHECK() call is accessing the value via __this_cpu_read() meaning it has
nothing to do with any other cpu's copy of the variable?

> +
> + for (i = 0; i < (1 << percpu_alloc_max_size_shift); i++) {
> + free_percpu(counters[i]);
> + cond_resched();
> + }
> + return -EAGAIN;
> +}
> +
> +static int percpu_stressd(void *v)
> +{
> + int iter = 0;
> +
> + pr_info("DAEMON: starts %s\n", __func__);
> + do {
> + if (kthread_should_stop())
> + break;
> + iter++;
> + pr_info("TEST Starts: %s: iter (%d)\n", __func__, iter);
> + percpu_test_stress();
> + pr_info("TEST Completed: %s: iter (%d)\n", __func__, iter);
> +
> + set_current_state(TASK_INTERRUPTIBLE);
> + schedule_timeout(percpu_stressd_interval);
> + } while (1);
> +
> + return 0;
> +}
> +
> +static int percpu_test_stressd(void)
> +{
> + percpu_stressd_thread = kthread_run(percpu_stressd, NULL, "percpu_stressd");
> + if (IS_ERR(percpu_stressd_thread))
> + percpu_stressd_thread = NULL;
> + return 0;
> +}
> +
> +enum test_type {
> + PERCPU_VERIFY,
> + PERCPU_STRESS,
> + PERCPU_STRESSD,
> + NR_TESTS,
> +};
> +
> +const char *test_names[NR_TESTS] = {
> + [PERCPU_VERIFY] = "percpu_verify",
> + [PERCPU_STRESS] = "percpu_stress",
> + [PERCPU_STRESSD] = "percpu_stressd",
> +};
> +
> +static int __init percpu_test_init(void)
> +{
> + int i, ret = 0;
> + typedef int (*percpu_tests)(void);
> + const percpu_tests test_funcs[NR_TESTS] = {
> + [PERCPU_VERIFY] = percpu_test_verify,
> + [PERCPU_STRESS] = percpu_test_stress,
> + [PERCPU_STRESSD] = percpu_test_stressd,
> + };
> +
> + /* sanity checks */
> + if (percpu_alloc_max_size_shift > PAGE_SHIFT)
> + percpu_alloc_max_size_shift = PAGE_SHIFT;
> + if (percpu_test_num > NR_TESTS)
> + percpu_test_num = NR_TESTS;
> +
> + pr_info("percpu_test: INIT, interval: %ld, max_shift: %d, run_tests: %s\n",
> + percpu_stressd_interval, percpu_alloc_max_size_shift,
> + percpu_test_num == NR_TESTS ? "run all tests" :
> + test_names[percpu_test_num]);
> +
> + /* run a given test */
> + if (percpu_test_num < NR_TESTS) {
> + pr_info("TEST Starts: %s\n", test_names[percpu_test_num]);
> + ret = test_funcs[percpu_test_num]();
> + pr_info("TEST Completed: %s\n", test_names[percpu_test_num]);
> + goto out;
> + }
> +
> + for (i = 0; i < NR_TESTS; i++) {
> + pr_info("TEST Starts: %s\n", test_names[i]);
> + test_funcs[i]();
> + pr_info("TEST Completed: %s\n", test_names[i]);
> + }
> +out:
> + return ret;
> }
>
> static void __exit percpu_test_exit(void)
> {
> + if (percpu_stressd_thread)
> + kthread_stop(percpu_stressd_thread);
> + pr_info("percpu_test: EXIT\n");
> }
>
> module_init(percpu_test_init)
> --
> 2.31.1
>

Thanks,
Dennis

Next message: Andi Kleen: "Re: [PATCH v4 11/15] pci: Add pci_iomap_shared{,_range}"
Previous message: Leo Yan: "Re: [PATCH v1 2/3] perf auxtrace: Add compat_auxtrace_mmap__{read_head|write_tail}"
In reply to: Ritesh Harjani: "[PATCHv2 2/2] lib/percpu_test: Add extra tests in percpu_test"
Next in thread: Dennis Zhou: "Re: [PATCHv2 1/2] kernel/workqueue: Make schedule_on_each_cpu as EXPORT_SYMBOL_GPL"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]