[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/3] cpu-timers, icount: new modules
From: |
Claudio Fontana |
Subject: |
Re: [PATCH 3/3] cpu-timers, icount: new modules |
Date: |
Sat, 11 Jul 2020 14:48:51 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 |
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; those three
> are the only non-TCG-specific functions that read use_icount, as far as
> I can see. qemu_start_warp_timer() does have an "if (qtest_enabled())"
> even, so it's clearly fishy.
>
> 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. 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. :)
>
>> 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.
Yes, absolutely this is what I wanted to express.
I would like to find out what the problem is that appears in s390,
I am not sure though that it is actually an s390-specific problem, it could
even be a migration qemu-file issue,
as apparently just flushing with qemu_fflush(f) "fixes" it.
My patch made the stream a bit smaller, and changed the layout of the
s390-skeys, which have an interesting field length (32768),
I wonder if I got just the right alignment to trigger a bug where the qemu-file
buffer is not properly flushed.
>
> (And by the way, my suggested patch to icount_enabled() was completely
> wrong!).
>
> Paolo
>
>
We will come back to this later, thanks a lot for the exchange!
Ciao,
Claudio
- Re: [PATCH 3/3] cpu-timers, icount: new modules, (continued)
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Cornelia Huck, 2020/07/13
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Claudio Fontana, 2020/07/11
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Cornelia Huck, 2020/07/13
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Claudio Fontana, 2020/07/13
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Thomas Huth, 2020/07/10
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Paolo Bonzini, 2020/07/10
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Claudio Fontana, 2020/07/11
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Paolo Bonzini, 2020/07/11
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Claudio Fontana, 2020/07/11
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Paolo Bonzini, 2020/07/11
- Re: [PATCH 3/3] cpu-timers, icount: new modules,
Claudio Fontana <=
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Claudio Fontana, 2020/07/29
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Paolo Bonzini, 2020/07/29
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Claudio Fontana, 2020/07/30
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Paolo Bonzini, 2020/07/30
- Re: [PATCH 3/3] cpu-timers, icount: new modules, Claudio Fontana, 2020/07/31