tests/functional/test_x86_64_hotplug_cpu.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
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.1On 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
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 :|
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
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 :|
© 2016 - 2025 Red Hat, Inc.