Re: [PATCH 3/3] cpu-timers, icount: new modules

[Claudio Fontana <cfontana@suse.de>]

On 7/11/20 11:39 AM, Paolo Bonzini wrote:
> On 11/07/20 11:14, Claudio Fontana wrote:
>> On 7/11/20 12:45 AM, Paolo Bonzini wrote:
>>> On 10/07/20 06:36, Thomas Huth wrote:
>>>>
>>>> In short this goes away if I again set icount to enabled for qtest,
>>>> basically ensuring that --enable-tcg is there and then reenabling icount.
>>>>
>>>> qtest was forcing icount and shift=0 by creating qemu options, in order to misuse its counter feature,
>>>> instead of using a separate counter.
>>>
>>> Why would it need a separate counter?  In both cases it's a
>>> manually-updated counter that is used for QEMU_CLOCK_VIRTUAL.  The only
>>> difference is that shift > 0 doesn't make sense for qtest.
>>
>> I think I would reverse the question. Why reuse for qtest a counter that has absolutely nothing to do with it?
>>
>> qtest has nothing to do with instruction counting.
> 
> Apart from the name, icount is more like deterministic execution than

Maybe we should start choosing names more carefully in a way to express what we mean?

> instruction counting (it's not a coincidence that record/replay is
> fundamentally based on icount).

record/replay is tcg-only.

>  qtests need to be deterministic and
> describe which qtest instructions run before a given timer fires and
> which run after.
> 
> And in both cases, determinism is achieved by controlling the
> advancement of QEMU_CLOCK_VIRTUAL.  It's only this central component of
> icount that is shared by qtest and TCG, and I think the problem is that
> this patch conflates all of them together:

I think that the existing code in master conflates them together actually.
Qtest can have its own counter, it does not need to be the icount instruction counter.


> 
> - the basic "is QEMU_CLOCK_VIRTUAL software-driven" part is embedded in
> qemu-timer and should not be carved out into a separate module.  This
> includes the use_icount variable, which should be kept in core QEMU code.

I don't see how this follows, how is using a global use_icount variable better than having this checked using icount_enabled()?

> 
> - the fact qtest uses -icount instead of configuring the variables
> directly is definitely a hack and can be removed.
> 
> - the adaptive frequency adjustment is definitely TCG specific, and so
> are the particular functions in cpus.c that test icount_enabled() and
> broke with this patch.  All this code should be included in the TCG
> module only or, before that, should be made conditional on $(CONFIG_TCG).
> 
> So I think this patch should have been the last, not the first. :)  Once
> you move all the accelerator runtime code from cpus.c to separate files,
> it will be possible to move the frequency adjustment and deadline
> management code into accel/tcg.  And then it will be obvious which code
> is not TCG-specific and can be extracted for convenience into a
> cpu-timers.c file.
> 
> Thanks,
> 
> Paolo
> 

I will come back to this later on, this patch seems to have uncovered an underlying issue, which shows on s390.

I'd rather now continue investigating that, choosing to try to actually understand the issue, rather than hiding it under the carpet.

Thanks,

Claudio