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

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

[Stefan Hajnoczi]

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

[Thomas Huth]

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

[Vladimir Sementsov-Ogievskiy]

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

[Stefan Hajnoczi]

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

[Thomas Huth]

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

[Vladimir Sementsov-Ogievskiy]

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

[Thomas Huth]

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