[PATCH] selftests/livepatch: wait for atomic replace to occur

[PATCH] selftests/livepatch: wait for atomic replace to occur

[Ryan Sullivan]

On some machines with a large number of CPUs there is a sizable delay
between an atomic replace occurring and when sysfs updates accordingly.
This fix uses 'loop_until' to wait for the atomic replace to unload all
previous livepatches.

Signed-off-by: Ryan Sullivan <rysulliv@redhat.com>
---
 tools/testing/selftests/livepatch/test-livepatch.sh | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/livepatch/test-livepatch.sh b/tools/testing/selftests/livepatch/test-livepatch.sh
index 65c9c058458d..bd13257bfdfe 100755
--- a/tools/testing/selftests/livepatch/test-livepatch.sh
+++ b/tools/testing/selftests/livepatch/test-livepatch.sh
@@ -139,11 +139,8 @@ load_lp $MOD_REPLACE replace=1
 grep 'live patched' /proc/cmdline > /dev/kmsg
 grep 'live patched' /proc/meminfo > /dev/kmsg
 
-mods=(/sys/kernel/livepatch/*)
-nmods=${#mods[@]}
-if [ "$nmods" -ne 1 ]; then
-	die "Expecting only one moduled listed, found $nmods"
-fi
+loop_until 'mods=(/sys/kernel/livepatch/*); nmods=${#mods[@]}; [[ "$nmods" -eq 1 ]]' ||
+        die "Expecting only one moduled listed, found $nmods"
 
 # These modules were disabled by the atomic replace
 for mod in $MOD_LIVEPATCH3 $MOD_LIVEPATCH2 $MOD_LIVEPATCH1; do
-- 
2.44.0

Re: [PATCH] selftests/livepatch: wait for atomic replace to occur

[Petr Mladek]

Hi,

this is 2nd version of the patch. There should have been used
[PATCH v2] in the Subject to make it clear in the mailbox.

On Thu 2024-08-22 13:31:22, Ryan Sullivan wrote:
> On some machines with a large number of CPUs there is a sizable delay
> between an atomic replace occurring and when sysfs updates accordingly.
> This fix uses 'loop_until' to wait for the atomic replace to unload all
> previous livepatches.
> 

I think that Joe suggested to add:

Reported-by: CKI Project <cki-project@redhat.com>
Closes: https://datawarehouse.cki-project.org/kcidb/tests/redhat:1413102084-x86_64-kernel_upt_28

> Signed-off-by: Ryan Sullivan <rysulliv@redhat.com>
> ---

Also it is a good practice to summarize changes between versions.
In this case it would have been something like:

Changes against v1:

  - Cleaned the commit message.

>  tools/testing/selftests/livepatch/test-livepatch.sh | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/livepatch/test-livepatch.sh b/tools/testing/selftests/livepatch/test-livepatch.sh
> index 65c9c058458d..bd13257bfdfe 100755
> --- a/tools/testing/selftests/livepatch/test-livepatch.sh
> +++ b/tools/testing/selftests/livepatch/test-livepatch.sh
> @@ -139,11 +139,8 @@ load_lp $MOD_REPLACE replace=1
>  grep 'live patched' /proc/cmdline > /dev/kmsg
>  grep 'live patched' /proc/meminfo > /dev/kmsg
>  
> -mods=(/sys/kernel/livepatch/*)
> -nmods=${#mods[@]}
> -if [ "$nmods" -ne 1 ]; then
> -	die "Expecting only one moduled listed, found $nmods"
> -fi
> +loop_until 'mods=(/sys/kernel/livepatch/*); nmods=${#mods[@]}; [[ "$nmods" -eq 1 ]]' ||
> +        die "Expecting only one moduled listed, found $nmods"
>  
>  # These modules were disabled by the atomic replace
>  for mod in $MOD_LIVEPATCH3 $MOD_LIVEPATCH2 $MOD_LIVEPATCH1; do

Otherwise, it looks good to me. With the added references:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

PS: No need to resend the patch. I would add the references when
    committing. I am going to wait few more days before committing.

Re: [PATCH] selftests/livepatch: wait for atomic replace to occur

[Ryan B. Sullivan]

[-- Attachment #1: Type: text/plain, Size: 173 bytes --]

Changes from v2:

Adds:
	Reported-by: CKI Project <cki-project@redhat.com>
	Closes: https://datawarehouse.cki-project.org/kcidb/tests/redhat:1413102084-x86_64-kernel_upt_28

[-- Attachment #2: PATCHv3 --]
[-- Type: text/plain, Size: 1626 bytes --]

From 9d9bfb21e86a3a79fb92fd22d927329510c6a672 Mon Sep 17 00:00:00 2001
From: Ryan Sullivan <rysulliv@redhat.com>
Date: Thu, 22 Aug 2024 12:19:54 -0400
Subject: [PATCH v3] selftests/livepatch: wait for atomic replace to occur

On some machines with a large number of CPUs there is a sizable delay
between an atomic replace occurring and when sysfs updates accordingly.
This fix uses 'loop_until' to wait for the atomic replace to unload all
previous livepatches.

Reported-by: CKI Project <cki-project@redhat.com>
Closes: https://datawarehouse.cki-project.org/kcidb/tests/redhat:1413102084-x86_64-kernel_upt_28
Signed-off-by: Ryan Sullivan <rysulliv@redhat.com>
---
 tools/testing/selftests/livepatch/test-livepatch.sh | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/livepatch/test-livepatch.sh b/tools/testing/selftests/livepatch/test-livepatch.sh
index 65c9c058458d..bd13257bfdfe 100755
--- a/tools/testing/selftests/livepatch/test-livepatch.sh
+++ b/tools/testing/selftests/livepatch/test-livepatch.sh
@@ -139,11 +139,8 @@ load_lp $MOD_REPLACE replace=1
 grep 'live patched' /proc/cmdline > /dev/kmsg
 grep 'live patched' /proc/meminfo > /dev/kmsg
 
-mods=(/sys/kernel/livepatch/*)
-nmods=${#mods[@]}
-if [ "$nmods" -ne 1 ]; then
-	die "Expecting only one moduled listed, found $nmods"
-fi
+loop_until 'mods=(/sys/kernel/livepatch/*); nmods=${#mods[@]}; [[ "$nmods" -eq 1 ]]' ||
+        die "Expecting only one moduled listed, found $nmods"
 
 # These modules were disabled by the atomic replace
 for mod in $MOD_LIVEPATCH3 $MOD_LIVEPATCH2 $MOD_LIVEPATCH1; do
-- 
2.44.0

Re: [PATCH] selftests/livepatch: wait for atomic replace to occur

[Petr Mladek]

Hi,

JFYI, I have committed the patch into livepatching.git,
branch for-6.11/selftests-fixup.

Few nits below :-)

On Fri 2024-08-23 09:09:26, Ryan B. Sullivan wrote:
> Changes from v2:
> 
> Adds:
> 	Reported-by: CKI Project <cki-project@redhat.com>
> 	Closes: https://datawarehouse.cki-project.org/kcidb/tests/redhat:1413102084-x86_64-kernel_upt_28

The "changelog" should have been added after the "---" line.

It allows to send the patch as the main part of the mail (no attachment).
The lines below the "---" gets automatically removed when the "mail"
gets applied using "git am".

Best Regards,
Petr

[PATCH] selftests/livepatch: wait for atomic replace to occur

[Ryan Sullivan]

Uses 'loop_until' to wait for the atomic replace to unload all previous
livepatches, as on some machines with a large number of CPUs there is a
sizable delay between the atomic replace ocurring and when sysfs
updates accordingly.

Signed-off-by: Ryan Sullivan <rysulliv@redhat.com>
---
 tools/testing/selftests/livepatch/test-livepatch.sh | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/livepatch/test-livepatch.sh b/tools/testing/selftests/livepatch/test-livepatch.sh
index 65c9c058458d..bd13257bfdfe 100755
--- a/tools/testing/selftests/livepatch/test-livepatch.sh
+++ b/tools/testing/selftests/livepatch/test-livepatch.sh
@@ -139,11 +139,8 @@ load_lp $MOD_REPLACE replace=1
 grep 'live patched' /proc/cmdline > /dev/kmsg
 grep 'live patched' /proc/meminfo > /dev/kmsg
 
-mods=(/sys/kernel/livepatch/*)
-nmods=${#mods[@]}
-if [ "$nmods" -ne 1 ]; then
-	die "Expecting only one moduled listed, found $nmods"
-fi
+loop_until 'mods=(/sys/kernel/livepatch/*); nmods=${#mods[@]}; [[ "$nmods" -eq 1 ]]' ||
+        die "Expecting only one moduled listed, found $nmods"
 
 # These modules were disabled by the atomic replace
 for mod in $MOD_LIVEPATCH3 $MOD_LIVEPATCH2 $MOD_LIVEPATCH1; do
-- 
2.44.0

Re: [PATCH] selftests/livepatch: wait for atomic replace to occur

[Joe Lawrence]

On 8/22/24 12:34, Ryan Sullivan wrote:
> Uses 'loop_until' to wait for the atomic replace to unload all previous
> livepatches, as on some machines with a large number of CPUs there is a
> sizable delay between the atomic replace ocurring and when sysfs
> updates accordingly.
> 

Small nit: flip this around so it describes the problem first, then the
'loop_util' solution.  Also spell check "occurring" if you keep it 😄

> Signed-off-by: Ryan Sullivan <rysulliv@redhat.com>

Let's give the CKI credit for finding this:
Reported-by: CKI Project <cki-project@redhat.com>

If anyone is interested, here is the test/log link:
https://datawarehouse.cki-project.org/kcidb/tests/redhat:1413102084-x86_64-kernel_upt_28

By the way, was it easy to repro on a similar machine as the one
reported by CKI and then how did it fare with this patch in place?

> ---
>  tools/testing/selftests/livepatch/test-livepatch.sh | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/livepatch/test-livepatch.sh b/tools/testing/selftests/livepatch/test-livepatch.sh
> index 65c9c058458d..bd13257bfdfe 100755
> --- a/tools/testing/selftests/livepatch/test-livepatch.sh
> +++ b/tools/testing/selftests/livepatch/test-livepatch.sh
> @@ -139,11 +139,8 @@ load_lp $MOD_REPLACE replace=1
>  grep 'live patched' /proc/cmdline > /dev/kmsg
>  grep 'live patched' /proc/meminfo > /dev/kmsg
>  
> -mods=(/sys/kernel/livepatch/*)
> -nmods=${#mods[@]}
> -if [ "$nmods" -ne 1 ]; then
> -	die "Expecting only one moduled listed, found $nmods"
> -fi
> +loop_until 'mods=(/sys/kernel/livepatch/*); nmods=${#mods[@]}; [[ "$nmods" -eq 1 ]]' ||
> +        die "Expecting only one moduled listed, found $nmods"
>  
>  # These modules were disabled by the atomic replace
>  for mod in $MOD_LIVEPATCH3 $MOD_LIVEPATCH2 $MOD_LIVEPATCH1; do

With commit msg additions above:

Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

Thanks,

-- 
Joe