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

From: Paolo Bonzini <pbonzini@redhat.com>
To: "Claudio Fontana" <cfontana@suse.de>,
	"Thomas Huth" <thuth@redhat.com>,
	"Laurent Vivier" <lvivier@redhat.com>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Roman Bolshakov" <r.bolshakov@yadro.com>,
	"Markus Armbruster" <armbru@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>,
	Colin Xu <colin.xu@intel.com>,
	Marcelo Tosatti <mtosatti@redhat.com>,
	qemu-devel@nongnu.org, haxm-team@intel.com,
	Wenchao Wang <wenchao.wang@intel.com>,
	Sunil Muthuswamy <sunilmut@microsoft.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH 3/3] cpu-timers, icount: new modules
Date: Sat, 11 Jul 2020 11:39:37 +0200	[thread overview]
Message-ID: <76aac4ac-40f5-4870-ed2b-bab8b68b0a64@redhat.com> (raw)
In-Reply-To: <e3cc11a4-8ba7-917a-844b-4f6ec69d140a@suse.de>

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
instruction counting (it's not a coincidence that record/replay is
fundamentally based on icount).  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:

- 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.

- 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