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

[Claudio Fontana]

Hi Paolo,

now that things exposed by my patch are fixed, back on this topic :-)

On 7/11/20 2:19 PM, Paolo Bonzini wrote:
> On 11/07/20 13:49, Claudio Fontana wrote:
>>> 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?
> 
> I don't disagree.  For icount in particular however we're about 12 years
> too late.
> 
>>>  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.
> 
> If you want you can add to your accelerator ops series one for
> qemu_get_clock_ns(QEMU_CLOCK_VIRTUAL), cpu_get_ticks() and
> qemu_start_warp_timer(), that would certainly work for me;


The problem I see here is, as usual, one of meaning.

Are qemu_get_clock_ns, cpu_get_ticks and qemu_start_warp_timer
accelerator-specific cpu interfaces?

Looking at their implementation, currently I don't think they are, what do you 
think?

Should these be grouped together with

create_vcpu_thread,
kick_vcpu_thread,
synchronize_cpu_state

in the same interface?



> those three
> are the only non-TCG-specific functions that read use_icount, as far as
> I can see.  


There is some more use of use_icount also in non-TCG code, to either ignore 
icount VIRTUAL timers or produce more deterministic behavior:

dma-helpers.c checks it to make reads "more deterministic" then icount is 
enabled,
util/async.c indirectly checks it calling timerlistgroup_deadline_ns to do 
aio_compute_timeout,
as does main_loop_wait(), to check that each timer can be used for deadlines 
(is not an icount VIRTUAL timer).
hw/core/ptimer.c checks it to again offer more deterministic behavior for 
icount,
and there is some use of it in target/riscv/csr.c (curious, as I would expect 
to rely on cpu_get_host_ticks instead of using icount directly).
and the notify_cb() in timerlist_notify() also checks it.

May be more..?


> qemu_start_warp_timer() does have an "if (qtest_enabled())"
> even, so it's clearly fishy.

[should read "qemu_start_warp_timer() does have an "if (icount_enabled())"']

It looks absolutely fishy to me.

One way it could be better I think would be to have this "if" in the places 
where qemu_start_warp_timer is actually called, and make it clear that this an 
icount-specific thing,

ie saying in

main_loop_wait()
{

if (icount_enabled()) {
  icount_start_warp_timer()
}

}

and also in timerlist_rearm saying:

if (icount_enabled()) {
  icount_start_warp_timer()
}


Regarding the actual warp, icount_warp_rt(),
note that qtest has its own "qtest_clock_warp" function,

which in my mind is just _misusing_ the icount bias field to realize its 
functionality:

atomic_set_i64(&timers_state.qemu_icount_bias,
               timers_state.qemu_icount_bias + warp);

instead of having its own counter.

Speculation: does separating the two even allow qtesting icount and replay 
functionality?


> 
> It may even be a good idea for TCG to have three sets of accelerator ops
> for respectively multi-threaded, round-robin and icount.
> 
> My point is that this patch is not the right way to start the
> refactoring because *for now* it's wrong to treat icount as a TCG-only
> concept.  


If we create a separate counter for qtest, as we do in the patch, I think is 
correct to treat them separately,

and check for qtest_enabled() and icount_enabled() separately as necessary in 
each given situation,
as we actually have now already, but with the opportune adjustments to correct 
for the now non-shared counter.

Which means that we need to make more explicit the cases where the 
qtest_enabled() and icount_enabled() cases are actually matching,
which I think would benefit from being explicit.

In my understanding the cases where qtest_enabled() and icount_enabled() 
actually match are the ones where we want to behave more "deterministically",
either because we are testing (qtest_enabled()) or because we want a stable 
instruction count (icount_enabled()).


>Having more separation between accelerators, as well as a
> clear interface between core and accelerators is certainly a laudable
> goal though.
> 
>>> - 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()?
> 
> If you can get rid of use_icount using a new accelerator ops member, it
> would be even better. :)


Maybe you could mention in more detail what you propose here?

To me it really seems correct to separate icount and qtest, the fact that their 
implementation currently ends up using the same counter is what needs to be 
rectified first,
but if you see a better abstraction to be able to refactor them better let me 
know, maybe it could be a next step?

I am not sure that the cpu accelerator interface is the right place to do this 
abstraction semantically, but maybe you can show me otherwise?


> 
>> 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.  But I don't think it's sweeping anything under the carpet; it's
> great if we find a currently latent s390 bug, but it is orthogonal to
> the design of that core<->accelerator interface.



Happy to see the loadvm bug now finally fixed, it has been bothering me for a 
while :-)


> 
> (And by the way, my suggested patch to icount_enabled() was completely
> wrong!).
> 
> Paolo
> 

Ciao,

Claudio