1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] iotests: drop unnecessary accel=kvm

Stefan Hajnoczi posted 1 patch 5 years, 2 months ago
Test asan passed
Test docker-clang@ubuntu failed
Test docker-mingw@fedora passed
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190219115937.21587-1-stefanha@redhat.com
Maintainers: Max Reitz <mreitz@redhat.com>, Kevin Wolf <kwolf@redhat.com>
tests/qemu-iotests/235 | 1 -
tests/qemu-iotests/238 | 1 -
2 files changed, 2 deletions(-)
[Qemu-devel] [PATCH] iotests: drop unnecessary accel=kvm
Posted by Stefan Hajnoczi 5 years, 2 months ago 
Tests 235 and 238 do not require the kvm accelerator.  TCG works fine.

Use the default accelerator instead of requiring kvm.

Suggested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/235 | 1 -
 tests/qemu-iotests/238 | 1 -
 2 files changed, 2 deletions(-)

diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
index d6edd97ab4..329da8f0c2 100755
--- a/tests/qemu-iotests/235
+++ b/tests/qemu-iotests/235
@@ -49,7 +49,6 @@ qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
                 str(size))
 
 vm = QEMUMachine(iotests.qemu_prog)
-vm.add_args('-machine', 'accel=kvm')
 if iotests.qemu_default_machine == 's390-ccw-virtio':
         vm.add_args('-no-shutdown')
 vm.add_args('-drive', 'id=src,file=' + disk)
diff --git a/tests/qemu-iotests/238 b/tests/qemu-iotests/238
index f81ee1112f..e490bb4298 100755
--- a/tests/qemu-iotests/238
+++ b/tests/qemu-iotests/238
@@ -33,7 +33,6 @@ else:
     virtio_scsi_device = 'virtio-scsi-pci'
 
 vm = QEMUMachine(iotests.qemu_prog)
-vm.add_args('-machine', 'accel=kvm')
 vm.launch()
 
 log(vm.qmp('blockdev-add', node_name='hd0', driver='null-co'))
-- 
2.20.1
Re: [Qemu-devel] [PATCH] iotests: drop unnecessary accel=kvm
Posted by Thomas Huth 5 years, 2 months ago 
On 19/02/2019 12.59, Stefan Hajnoczi wrote:
> Tests 235 and 238 do not require the kvm accelerator.  TCG works fine.
> 
> Use the default accelerator instead of requiring kvm.
> 
> Suggested-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/235 | 1 -
>  tests/qemu-iotests/238 | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
> index d6edd97ab4..329da8f0c2 100755
> --- a/tests/qemu-iotests/235
> +++ b/tests/qemu-iotests/235
> @@ -49,7 +49,6 @@ qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>                  str(size))
>  
>  vm = QEMUMachine(iotests.qemu_prog)
> -vm.add_args('-machine', 'accel=kvm')

According to the initial commit log of 235:

 "iotests: simple mirror test with kvm on 1G image"

... so I assume KVM was used on purpose here?

 Thomas
Re: [Qemu-devel] [PATCH] iotests: drop unnecessary accel=kvm
Posted by Vladimir Sementsov-Ogievskiy 5 years, 2 months ago 
19.02.2019 15:02, Thomas Huth wrote:
> On 19/02/2019 12.59, Stefan Hajnoczi wrote:
>> Tests 235 and 238 do not require the kvm accelerator.  TCG works fine.
>>
>> Use the default accelerator instead of requiring kvm.
>>
>> Suggested-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   tests/qemu-iotests/235 | 1 -
>>   tests/qemu-iotests/238 | 1 -
>>   2 files changed, 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
>> index d6edd97ab4..329da8f0c2 100755
>> --- a/tests/qemu-iotests/235
>> +++ b/tests/qemu-iotests/235
>> @@ -49,7 +49,6 @@ qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>>                   str(size))
>>   
>>   vm = QEMUMachine(iotests.qemu_prog)
>> -vm.add_args('-machine', 'accel=kvm')
> 
> According to the initial commit log of 235:
> 
>   "iotests: simple mirror test with kvm on 1G image"
> 
> ... so I assume KVM was used on purpose here?
> 
>   Thomas
> 

As I remember kvm is not really necessary, and test have a comment about it:
# And it didn't reproduce if at least one of the following:
...
# 3. drop kvm and use iotests.VM() (maybe, because of qtest) (however, it still
#    reproduces, if just drop kvm, but gdb failed to produce full backtraces
#    for me)

But the comment should be updated with this patch.

-- 
Best regards,
Vladimir
Re: [Qemu-devel] [Qemu-block] [PATCH] iotests: drop unnecessary accel=kvm
Posted by Stefan Hajnoczi 5 years, 2 months ago 
On Tue, Feb 19, 2019 at 12:12 PM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
> 19.02.2019 15:02, Thomas Huth wrote:
> > On 19/02/2019 12.59, Stefan Hajnoczi wrote:
> >> Tests 235 and 238 do not require the kvm accelerator.  TCG works fine.
> >>
> >> Use the default accelerator instead of requiring kvm.
> >>
> >> Suggested-by: Thomas Huth <thuth@redhat.com>
> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> ---
> >>   tests/qemu-iotests/235 | 1 -
> >>   tests/qemu-iotests/238 | 1 -
> >>   2 files changed, 2 deletions(-)
> >>
> >> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
> >> index d6edd97ab4..329da8f0c2 100755
> >> --- a/tests/qemu-iotests/235
> >> +++ b/tests/qemu-iotests/235
> >> @@ -49,7 +49,6 @@ qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
> >>                   str(size))
> >>
> >>   vm = QEMUMachine(iotests.qemu_prog)
> >> -vm.add_args('-machine', 'accel=kvm')
> >
> > According to the initial commit log of 235:
> >
> >   "iotests: simple mirror test with kvm on 1G image"
> >
> > ... so I assume KVM was used on purpose here?
> >
> >   Thomas
> >
>
> As I remember kvm is not really necessary, and test have a comment about it:
> # And it didn't reproduce if at least one of the following:
> ...
> # 3. drop kvm and use iotests.VM() (maybe, because of qtest) (however, it still
> #    reproduces, if just drop kvm, but gdb failed to produce full backtraces
> #    for me)
>
> But the comment should be updated with this patch.

Will fix in v2.

Stefan
Re: [Qemu-devel] [Qemu-block] [PATCH] iotests: drop unnecessary accel=kvm
Posted by Thomas Huth 5 years, 2 months ago 
On 19/02/2019 14.07, Stefan Hajnoczi wrote:
> On Tue, Feb 19, 2019 at 12:12 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com> wrote:
>> 19.02.2019 15:02, Thomas Huth wrote:
>>> On 19/02/2019 12.59, Stefan Hajnoczi wrote:
>>>> Tests 235 and 238 do not require the kvm accelerator.  TCG works fine.
>>>>
>>>> Use the default accelerator instead of requiring kvm.
>>>>
>>>> Suggested-by: Thomas Huth <thuth@redhat.com>
>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>> ---
>>>>   tests/qemu-iotests/235 | 1 -
>>>>   tests/qemu-iotests/238 | 1 -
>>>>   2 files changed, 2 deletions(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
>>>> index d6edd97ab4..329da8f0c2 100755
>>>> --- a/tests/qemu-iotests/235
>>>> +++ b/tests/qemu-iotests/235
>>>> @@ -49,7 +49,6 @@ qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>>>>                   str(size))
>>>>
>>>>   vm = QEMUMachine(iotests.qemu_prog)
>>>> -vm.add_args('-machine', 'accel=kvm')
>>>
>>> According to the initial commit log of 235:
>>>
>>>   "iotests: simple mirror test with kvm on 1G image"
>>>
>>> ... so I assume KVM was used on purpose here?
>>>
>>>   Thomas
>>>
>>
>> As I remember kvm is not really necessary, and test have a comment about it:
>> # And it didn't reproduce if at least one of the following:
>> ...
>> # 3. drop kvm and use iotests.VM() (maybe, because of qtest) (however, it still
>> #    reproduces, if just drop kvm, but gdb failed to produce full backtraces
>> #    for me)
>>
>> But the comment should be updated with this patch.
> 
> Will fix in v2.

Any chance that you could even make it somehow work with -M accel=qtest
instead? ... in case someone compiled their QEMU with --disable-tcg, too
...?

 Thomas
Re: [Qemu-devel] [Qemu-block] [PATCH] iotests: drop unnecessary accel=kvm
Posted by Vladimir Sementsov-Ogievskiy 5 years, 2 months ago 
19.02.2019 16:20, Thomas Huth wrote:
> On 19/02/2019 14.07, Stefan Hajnoczi wrote:
>> On Tue, Feb 19, 2019 at 12:12 PM Vladimir Sementsov-Ogievskiy
>> <vsementsov@virtuozzo.com> wrote:
>>> 19.02.2019 15:02, Thomas Huth wrote:
>>>> On 19/02/2019 12.59, Stefan Hajnoczi wrote:
>>>>> Tests 235 and 238 do not require the kvm accelerator.  TCG works fine.
>>>>>
>>>>> Use the default accelerator instead of requiring kvm.
>>>>>
>>>>> Suggested-by: Thomas Huth <thuth@redhat.com>
>>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>>> ---
>>>>>    tests/qemu-iotests/235 | 1 -
>>>>>    tests/qemu-iotests/238 | 1 -
>>>>>    2 files changed, 2 deletions(-)
>>>>>
>>>>> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
>>>>> index d6edd97ab4..329da8f0c2 100755
>>>>> --- a/tests/qemu-iotests/235
>>>>> +++ b/tests/qemu-iotests/235
>>>>> @@ -49,7 +49,6 @@ qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>>>>>                    str(size))
>>>>>
>>>>>    vm = QEMUMachine(iotests.qemu_prog)
>>>>> -vm.add_args('-machine', 'accel=kvm')
>>>>
>>>> According to the initial commit log of 235:
>>>>
>>>>    "iotests: simple mirror test with kvm on 1G image"
>>>>
>>>> ... so I assume KVM was used on purpose here?
>>>>
>>>>    Thomas
>>>>
>>>
>>> As I remember kvm is not really necessary, and test have a comment about it:
>>> # And it didn't reproduce if at least one of the following:
>>> ...
>>> # 3. drop kvm and use iotests.VM() (maybe, because of qtest) (however, it still
>>> #    reproduces, if just drop kvm, but gdb failed to produce full backtraces
>>> #    for me)
>>>
>>> But the comment should be updated with this patch.
>>
>> Will fix in v2.
> 
> Any chance that you could even make it somehow work with -M accel=qtest
> instead? ... in case someone compiled their QEMU with --disable-tcg, too
> ...?
> 

I didn't investigate why bug not reproduced with qtest.. I think, the simplest
option should be a helper like iotests.needs_tcg(), to skip the test if
it is unsupported.

Or you case is that kvm is OK for you but tcg is not? Stefan, do you have strong reason
for removing kvm from iotests? Could it be something like

   if not iotests.supports_tcg():
      vm.add_args('-machine', 'accel=kvm')

?

Hmm, maybe, good option would be topmost option, like we have for format -qcow2, -raw, etc. So,
it would be -qtest, -kvm, -tcg.. But in this case nobody will run all test on something other
than qtest..

It's not bad to run this test on qtest. The problem is we can miss the bug, if not run this
test on kvm or tcg.

-- 
Best regards,
Vladimir
Re: [Qemu-devel] [Qemu-block] [PATCH] iotests: drop unnecessary accel=kvm
Posted by Thomas Huth 5 years, 2 months ago 
On 19/02/2019 14.36, Vladimir Sementsov-Ogievskiy wrote:
> 19.02.2019 16:20, Thomas Huth wrote:
>> On 19/02/2019 14.07, Stefan Hajnoczi wrote:
>>> On Tue, Feb 19, 2019 at 12:12 PM Vladimir Sementsov-Ogievskiy
>>> <vsementsov@virtuozzo.com> wrote:
>>>> 19.02.2019 15:02, Thomas Huth wrote:
>>>>> On 19/02/2019 12.59, Stefan Hajnoczi wrote:
>>>>>> Tests 235 and 238 do not require the kvm accelerator.  TCG works fine.
>>>>>>
>>>>>> Use the default accelerator instead of requiring kvm.
>>>>>>
>>>>>> Suggested-by: Thomas Huth <thuth@redhat.com>
>>>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>>>> ---
>>>>>>    tests/qemu-iotests/235 | 1 -
>>>>>>    tests/qemu-iotests/238 | 1 -
>>>>>>    2 files changed, 2 deletions(-)
>>>>>>
>>>>>> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
>>>>>> index d6edd97ab4..329da8f0c2 100755
>>>>>> --- a/tests/qemu-iotests/235
>>>>>> +++ b/tests/qemu-iotests/235
>>>>>> @@ -49,7 +49,6 @@ qemu_img_create('-f', iotests.imgfmt, '-o', 'preallocation=metadata', disk,
>>>>>>                    str(size))
>>>>>>
>>>>>>    vm = QEMUMachine(iotests.qemu_prog)
>>>>>> -vm.add_args('-machine', 'accel=kvm')
>>>>>
>>>>> According to the initial commit log of 235:
>>>>>
>>>>>    "iotests: simple mirror test with kvm on 1G image"
>>>>>
>>>>> ... so I assume KVM was used on purpose here?
>>>>
>>>> As I remember kvm is not really necessary, and test have a comment about it:
>>>> # And it didn't reproduce if at least one of the following:
>>>> ...
>>>> # 3. drop kvm and use iotests.VM() (maybe, because of qtest) (however, it still
>>>> #    reproduces, if just drop kvm, but gdb failed to produce full backtraces
>>>> #    for me)
>>>>
>>>> But the comment should be updated with this patch.
>>>
>>> Will fix in v2.
>>
>> Any chance that you could even make it somehow work with -M accel=qtest
>> instead? ... in case someone compiled their QEMU with --disable-tcg, too
>> ...?
> 
> I didn't investigate why bug not reproduced with qtest.. I think, the simplest
> option should be a helper like iotests.needs_tcg(), to skip the test if
> it is unsupported.
> 
> Or you case is that kvm is OK for you but tcg is not? Stefan, do you have strong reason
> for removing kvm from iotests? Could it be something like
> 
>    if not iotests.supports_tcg():
>       vm.add_args('-machine', 'accel=kvm')
> 
> ?

You can also use "accel=kvm:tcg" which tries to use kvm first and then
falls back to tcg if KVM is not available.

I just have this weird condition where I try to compile qemu with
--disable-tcg, and then run the iotests on a CI system where KVM is also
not available at runtime. So qtest is the only available accelerator
there...

 Thomas

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.