| Message ID | 20200528134849.7890-2-ycote@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/livepatch: rework of test-klp-{callbacks,shadow_vars} | expand |
On Thu 2020-05-28 09:48:46, Yannick Cote wrote: > From: Joe Lawrence <joe.lawrence@redhat.com> > > The test-klp-callbacks script includes a few tests which rely on kernel > task timings that may not always execute as expected under system load. > These will generate out of sequence kernel log messages that result in > test failure. > > Instead of using sleep timing windows to orchestrate the test, rework > the test_klp_callbacks_busy module to use completion variables. > > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com> > Signed-off-by: Yannick Cote <ycote@redhat.com> > --- > lib/livepatch/test_klp_callbacks_busy.c | 42 +++++++++++++++---- > .../selftests/livepatch/test-callbacks.sh | 29 +++++++------ > 2 files changed, 47 insertions(+), 24 deletions(-) > > diff --git a/lib/livepatch/test_klp_callbacks_busy.c b/lib/livepatch/test_klp_callbacks_busy.c > index 40beddf8a0e2..c3df12f47e5e 100644 > --- a/lib/livepatch/test_klp_callbacks_busy.c > +++ b/lib/livepatch/test_klp_callbacks_busy.c > @@ -5,34 +5,58 @@ > > #include <linux/module.h> > #include <linux/kernel.h> > +#include <linux/sched.h> > #include <linux/workqueue.h> > #include <linux/delay.h> > > -static int sleep_secs; > -module_param(sleep_secs, int, 0644); > -MODULE_PARM_DESC(sleep_secs, "sleep_secs (default=0)"); > +/* load/run-time control from sysfs writer */ > +static bool block_transition; > +module_param(block_transition, bool, 0644); > +MODULE_PARM_DESC(block_transition, "block_transition (default=false)"); > > static void busymod_work_func(struct work_struct *work); > -static DECLARE_DELAYED_WORK(work, busymod_work_func); > +static DECLARE_WORK(work, busymod_work_func); > +static DECLARE_COMPLETION(busymod_work_complete); > > static void busymod_work_func(struct work_struct *work) > { > - pr_info("%s, sleeping %d seconds ...\n", __func__, sleep_secs); > - msleep(sleep_secs * 1000); > + bool early_complete = block_transition; > + > + pr_info("%s enter\n", __func__); > + > + /* > + * When blocking the livepatch transition, set completion flag > + * early so subsequent test commands see the transition. > + */ > + if (early_complete) > + complete(&busymod_work_complete); I have to say that the code is really confusing. A completion called "work_complete" is completed when the work gets actually. It is completed later when the work is done immediately. Do we need the completion at all? See below. > + > + while (block_transition) The compiler might optimize the code and avoid the re-reads. Please, use: /* Re-read variable in each cycle */ while (READ_ONCE(block_transition)) > + msleep(1000); Nit: This is still a busy wait even though there is a big delay between waits. The right solution would be using wait_event(). But feel free to keep msleep(). It is good enough for selftests. > + > pr_info("%s exit\n", __func__); > + > + /* > + * In non-blocking case, wait until we're done to complete to > + * ensure kernel log ordering > + */ > + if (!early_complete) > + complete(&busymod_work_complete); > } > > static int test_klp_callbacks_busy_init(void) > { > pr_info("%s\n", __func__); > - schedule_delayed_work(&work, > - msecs_to_jiffies(1000 * 0)); > + schedule_work(&work); > + wait_for_completion(&busymod_work_complete); IMHO, the completion is not needed when using: schedule_work(&work); /* * Print all messages from the work before returning from init(). * It helps to serialize messages from the loaded modules. */ if (!block_transition) flush_work(&work); > + > return 0; > } > > static void test_klp_callbacks_busy_exit(void) > { > - cancel_delayed_work_sync(&work); > + block_transition = false; The compiler could move this assignment after the following call. Please, use: /* Make sure that the variable is set before flushing work. */ WRITE_ONCE(block_transition, false); > + cancel_work_sync(&work); The work is not longer canceled. flush_work() better fits here. Also I would do this only when the transition is blocked: if (block_transition) { /* Make sure that the variable is set before flushing work. */ WRITE_ONCE(block_transition, false); flush_work(&work); } Otherwise this is a nice improvement. Best Regards, Petr
diff --git a/lib/livepatch/test_klp_callbacks_busy.c b/lib/livepatch/test_klp_callbacks_busy.c index 40beddf8a0e2..c3df12f47e5e 100644 --- a/lib/livepatch/test_klp_callbacks_busy.c +++ b/lib/livepatch/test_klp_callbacks_busy.c @@ -5,34 +5,58 @@ #include <linux/module.h> #include <linux/kernel.h> +#include <linux/sched.h> #include <linux/workqueue.h> #include <linux/delay.h> -static int sleep_secs; -module_param(sleep_secs, int, 0644); -MODULE_PARM_DESC(sleep_secs, "sleep_secs (default=0)"); +/* load/run-time control from sysfs writer */ +static bool block_transition; +module_param(block_transition, bool, 0644); +MODULE_PARM_DESC(block_transition, "block_transition (default=false)"); static void busymod_work_func(struct work_struct *work); -static DECLARE_DELAYED_WORK(work, busymod_work_func); +static DECLARE_WORK(work, busymod_work_func); +static DECLARE_COMPLETION(busymod_work_complete); static void busymod_work_func(struct work_struct *work) { - pr_info("%s, sleeping %d seconds ...\n", __func__, sleep_secs); - msleep(sleep_secs * 1000); + bool early_complete = block_transition; + + pr_info("%s enter\n", __func__); + + /* + * When blocking the livepatch transition, set completion flag + * early so subsequent test commands see the transition. + */ + if (early_complete) + complete(&busymod_work_complete); + + while (block_transition) + msleep(1000); + pr_info("%s exit\n", __func__); + + /* + * In non-blocking case, wait until we're done to complete to + * ensure kernel log ordering + */ + if (!early_complete) + complete(&busymod_work_complete); } static int test_klp_callbacks_busy_init(void) { pr_info("%s\n", __func__); - schedule_delayed_work(&work, - msecs_to_jiffies(1000 * 0)); + schedule_work(&work); + wait_for_completion(&busymod_work_complete); + return 0; } static void test_klp_callbacks_busy_exit(void) { - cancel_delayed_work_sync(&work); + block_transition = false; + cancel_work_sync(&work); pr_info("%s\n", __func__); } diff --git a/tools/testing/selftests/livepatch/test-callbacks.sh b/tools/testing/selftests/livepatch/test-callbacks.sh index a35289b13c9c..32b57ba07f4f 100755 --- a/tools/testing/selftests/livepatch/test-callbacks.sh +++ b/tools/testing/selftests/livepatch/test-callbacks.sh @@ -356,9 +356,7 @@ livepatch: '$MOD_LIVEPATCH': unpatching complete echo -n "TEST: multiple target modules ... " dmesg -C -load_mod $MOD_TARGET_BUSY sleep_secs=0 -# give $MOD_TARGET_BUSY::busymod_work_func() a chance to run -sleep 5 +load_mod $MOD_TARGET_BUSY block_transition=N load_lp $MOD_LIVEPATCH load_mod $MOD_TARGET unload_mod $MOD_TARGET @@ -366,9 +364,9 @@ disable_lp $MOD_LIVEPATCH unload_lp $MOD_LIVEPATCH unload_mod $MOD_TARGET_BUSY -check_result "% modprobe $MOD_TARGET_BUSY sleep_secs=0 +check_result "% modprobe $MOD_TARGET_BUSY block_transition=N $MOD_TARGET_BUSY: ${MOD_TARGET_BUSY}_init -$MOD_TARGET_BUSY: busymod_work_func, sleeping 0 seconds ... +$MOD_TARGET_BUSY: busymod_work_func enter $MOD_TARGET_BUSY: busymod_work_func exit % modprobe $MOD_LIVEPATCH livepatch: enabling patch '$MOD_LIVEPATCH' @@ -404,11 +402,10 @@ livepatch: '$MOD_LIVEPATCH': unpatching complete $MOD_TARGET_BUSY: ${MOD_TARGET_BUSY}_exit" - # TEST: busy target module # # A similar test as the previous one, but force the "busy" kernel module -# to do longer work. +# to block the livepatch transition. # # The livepatching core will refuse to patch a task that is currently # executing a to-be-patched function -- the consistency model stalls the @@ -417,8 +414,7 @@ $MOD_TARGET_BUSY: ${MOD_TARGET_BUSY}_exit" # function for a long time. Meanwhile, load and unload other target # kernel modules while the livepatch transition is in progress. # -# - Load the "busy" kernel module, this time make it do 10 seconds worth -# of work. +# - Load the "busy" kernel module, this time make its work function loop # # - Meanwhile, the livepatch is loaded. Notice that the patch # transition does not complete as the targeted "busy" module is @@ -438,20 +434,23 @@ $MOD_TARGET_BUSY: ${MOD_TARGET_BUSY}_exit" echo -n "TEST: busy target module ... " dmesg -C -load_mod $MOD_TARGET_BUSY sleep_secs=10 +load_mod $MOD_TARGET_BUSY block_transition=Y load_lp_nowait $MOD_LIVEPATCH -# Don't wait for transition, load $MOD_TARGET while the transition -# is still stalled in $MOD_TARGET_BUSY::busymod_work_func() -sleep 5 + +# Wait until the livepatch reports in-transition state, i.e. that it's +# stalled on $MOD_TARGET_BUSY::busymod_work_func() +loop_until 'grep -q '^1$' /sys/kernel/livepatch/$MOD_LIVEPATCH/transition' || + die "failed to stall transition" + load_mod $MOD_TARGET unload_mod $MOD_TARGET disable_lp $MOD_LIVEPATCH unload_lp $MOD_LIVEPATCH unload_mod $MOD_TARGET_BUSY -check_result "% modprobe $MOD_TARGET_BUSY sleep_secs=10 +check_result "% modprobe $MOD_TARGET_BUSY block_transition=Y $MOD_TARGET_BUSY: ${MOD_TARGET_BUSY}_init -$MOD_TARGET_BUSY: busymod_work_func, sleeping 10 seconds ... +$MOD_TARGET_BUSY: busymod_work_func enter % modprobe $MOD_LIVEPATCH livepatch: enabling patch '$MOD_LIVEPATCH' livepatch: '$MOD_LIVEPATCH': initializing patching transition