[PATCH] tests/functional/test_x86_64_hotplug_cpu: Fix race condition during unplug

[PATCH] tests/functional/test_x86_64_hotplug_cpu: Fix race condition during unplug

[Thomas Huth]

When unplugging the CPU, the test tries to check for a successful
unplug by changing to the /sys/devices/system/cpu/cpu1 directory
to see whether that fails. However, the "cd" could be faster than
the unplug operation in the kernel, so there is a race condition
and the test sometimes fails here.
Fix it by trying to change the directory in a loop until the the
CPU has really been unplugged.

Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/functional/test_x86_64_hotplug_cpu.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/functional/test_x86_64_hotplug_cpu.py b/tests/functional/test_x86_64_hotplug_cpu.py
index b1d5156c72..7b9200ac2e 100755
--- a/tests/functional/test_x86_64_hotplug_cpu.py
+++ b/tests/functional/test_x86_64_hotplug_cpu.py
@@ -59,11 +59,13 @@ def test_hotplug(self):
                                           'cd /sys/devices/system/cpu/cpu1',
                                           'cpu1#')
 
+        exec_command_and_wait_for_pattern(self, 'cd ..', prompt)
         self.vm.cmd('device_del', id='c1')
 
         exec_command_and_wait_for_pattern(self,
-                                          'cd /sys/devices/system/cpu/cpu1',
-                                          'No such file or directory')
+                                    'while cd /sys/devices/system/cpu/cpu1 ;'
+                                    ' do sleep 0.2 ; done',
+                                    'No such file or directory')
 
 if __name__ == '__main__':
     LinuxKernelTest.main()
-- 
2.47.1

Re: [PATCH] tests/functional/test_x86_64_hotplug_cpu: Fix race condition during unplug

[Daniel P. Berrangé]

On Tue, Jan 07, 2025 at 12:52:45PM +0100, Thomas Huth wrote:
> When unplugging the CPU, the test tries to check for a successful
> unplug by changing to the /sys/devices/system/cpu/cpu1 directory
> to see whether that fails. However, the "cd" could be faster than
> the unplug operation in the kernel, so there is a race condition
> and the test sometimes fails here.
> Fix it by trying to change the directory in a loop until the the
> CPU has really been unplugged.
> 
> Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/functional/test_x86_64_hotplug_cpu.py | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/functional/test_x86_64_hotplug_cpu.py b/tests/functional/test_x86_64_hotplug_cpu.py
> index b1d5156c72..7b9200ac2e 100755
> --- a/tests/functional/test_x86_64_hotplug_cpu.py
> +++ b/tests/functional/test_x86_64_hotplug_cpu.py
> @@ -59,11 +59,13 @@ def test_hotplug(self):
>                                            'cd /sys/devices/system/cpu/cpu1',
>                                            'cpu1#')
>  
> +        exec_command_and_wait_for_pattern(self, 'cd ..', prompt)

Is this actually needed ? Are we keeping the CPU from being unplugged by
being in that dir ? If so, why isn't it also needed in the while loop
below ?

>          self.vm.cmd('device_del', id='c1')
>  
>          exec_command_and_wait_for_pattern(self,
> -                                          'cd /sys/devices/system/cpu/cpu1',
> -                                          'No such file or directory')
> +                                    'while cd /sys/devices/system/cpu/cpu1 ;'
> +                                    ' do sleep 0.2 ; done',
> +                                    'No such file or directory')
>  
>  if __name__ == '__main__':
>      LinuxKernelTest.main()
> -- 
> 2.47.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] tests/functional/test_x86_64_hotplug_cpu: Fix race condition during unplug

[Thomas Huth]

On 07/01/2025 12.57, Daniel P. Berrangé wrote:
> On Tue, Jan 07, 2025 at 12:52:45PM +0100, Thomas Huth wrote:
>> When unplugging the CPU, the test tries to check for a successful
>> unplug by changing to the /sys/devices/system/cpu/cpu1 directory
>> to see whether that fails. However, the "cd" could be faster than
>> the unplug operation in the kernel, so there is a race condition
>> and the test sometimes fails here.
>> Fix it by trying to change the directory in a loop until the the
>> CPU has really been unplugged.
>>
>> Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   tests/functional/test_x86_64_hotplug_cpu.py | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/functional/test_x86_64_hotplug_cpu.py b/tests/functional/test_x86_64_hotplug_cpu.py
>> index b1d5156c72..7b9200ac2e 100755
>> --- a/tests/functional/test_x86_64_hotplug_cpu.py
>> +++ b/tests/functional/test_x86_64_hotplug_cpu.py
>> @@ -59,11 +59,13 @@ def test_hotplug(self):
>>                                             'cd /sys/devices/system/cpu/cpu1',
>>                                             'cpu1#')
>>   
>> +        exec_command_and_wait_for_pattern(self, 'cd ..', prompt)
> 
> Is this actually needed ? Are we keeping the CPU from being unplugged by
> being in that dir ? If so, why isn't it also needed in the while loop
> below ?

I added it to make the console output less confusing (otherwise it's still 
shown in the prompt after the CPU has been unplugged)... but I can also 
remove this line again if you prefer it?

  Thomas

Re: [PATCH] tests/functional/test_x86_64_hotplug_cpu: Fix race condition during unplug

[Daniel P. Berrangé]

On Tue, Jan 07, 2025 at 01:03:52PM +0100, Thomas Huth wrote:
> On 07/01/2025 12.57, Daniel P. Berrangé wrote:
> > On Tue, Jan 07, 2025 at 12:52:45PM +0100, Thomas Huth wrote:
> > > When unplugging the CPU, the test tries to check for a successful
> > > unplug by changing to the /sys/devices/system/cpu/cpu1 directory
> > > to see whether that fails. However, the "cd" could be faster than
> > > the unplug operation in the kernel, so there is a race condition
> > > and the test sometimes fails here.
> > > Fix it by trying to change the directory in a loop until the the
> > > CPU has really been unplugged.
> > > 
> > > Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > ---
> > >   tests/functional/test_x86_64_hotplug_cpu.py | 6 ++++--
> > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tests/functional/test_x86_64_hotplug_cpu.py b/tests/functional/test_x86_64_hotplug_cpu.py
> > > index b1d5156c72..7b9200ac2e 100755
> > > --- a/tests/functional/test_x86_64_hotplug_cpu.py
> > > +++ b/tests/functional/test_x86_64_hotplug_cpu.py
> > > @@ -59,11 +59,13 @@ def test_hotplug(self):
> > >                                             'cd /sys/devices/system/cpu/cpu1',
> > >                                             'cpu1#')
> > > +        exec_command_and_wait_for_pattern(self, 'cd ..', prompt)
> > 
> > Is this actually needed ? Are we keeping the CPU from being unplugged by
> > being in that dir ? If so, why isn't it also needed in the while loop
> > below ?
> 
> I added it to make the console output less confusing (otherwise it's still
> shown in the prompt after the CPU has been unplugged)... but I can also
> remove this line again if you prefer it?

No, i'm not that fussed.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] tests/functional/test_x86_64_hotplug_cpu: Fix race condition during unplug

[Stefan Hajnoczi]

On Tue, 7 Jan 2025 at 06:52, Thomas Huth <thuth@redhat.com> wrote:
>
> When unplugging the CPU, the test tries to check for a successful
> unplug by changing to the /sys/devices/system/cpu/cpu1 directory
> to see whether that fails. However, the "cd" could be faster than
> the unplug operation in the kernel, so there is a race condition
> and the test sometimes fails here.
> Fix it by trying to change the directory in a loop until the the
> CPU has really been unplugged.
>
> Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/functional/test_x86_64_hotplug_cpu.py | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Thanks for fixing this! I'll keep an eye on the CI job status after
merging your next pull request.

Stefan