diff mbox series

[1/4] selftests/livepatch: rework test-klp-callbacks to use completion variables

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

Commit Message

Yannick Cote May 28, 2020, 1:48 p.m. UTC
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(-)

Comments

Petr Mladek June 2, 2020, 8:16 a.m. UTC | #1
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 mbox series

Patch

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