Re: [Qemu-devel] [PATCH v6 3/7] trace: [tcg] Delay changes to dynamic state when translating

[Paolo Bonzini <pbonzini@redhat.com>]

On 12/01/2017 20:37, Lluís Vilanova wrote:
> Stefan Hajnoczi writes:
> 
>> On Tue, Jan 10, 2017 at 05:31:37PM +0100, Paolo Bonzini wrote:
>>> On 09/01/2017 18:01, Stefan Hajnoczi wrote:
>>>> Or use a simpler scheme:
>>>>
>>>> struct CPUState {
>>>>     ...
>>>>     uint32_t dstate_update_count;
>>>> };
>>>>
>>>> In trace_event_set_vcpu_state_dynamic():
>>>>
>>>>     if (state) {
>>>>         trace_events_enabled_count++;
>>>>         set_bit(vcpu_id, vcpu->trace_dstate_delayed);
>>>> 	atomic_inc(&vcpu->dstate_update_count, 1);
>>>>         (*ev->dstate)++;
>>>>     } ...
>>>>
>>>> In cpu_exec() and friends:
>>>>
>>>>     last_dstate_update_count = atomic_read(&vcpu->dstate_update_count);
>>>>
>>>>     tb = tb_find(cpu, last_tb, tb_exit);
>>>>     cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit, &sc);
>>>>
>>>>     /* apply and disable delayed dstate changes */
>>>>     if (unlikely(atomic_read(&cpu->dstate_update_count) != last_dstate_update_count)) {
>>>>         bitmap_copy(cpu->trace_dstate, cpu->trace_dstate_delayed,
>>>>         trace_get_vcpu_event_count());
>>>>     }
>>>>
>>>> (You'll need to adjust the details but the update counter approach
>>>> should be workable.)
>>>
>>> Would it work to use async_run_on_cpu?
> 
>> I think so.
> 
> AFAIU we cannot use async_run_on_cpu(), since we need to reset the local
> variable "last_tb" to avoid chaining TBs with different dstates, and we cannot
> use cpu_loop_exit() inside the callback.

async_run_on_cpu would run as soon as the currently executing TB
finishes, and would leave cpu_exec completely, so there would be no
chaining.

Paolo

> To make it work, we'd need to add some new boolean flag on the vCPU to control
> when to reset "last_tb", and then we're just as good as implementing the async
> work "protocol" manually for this specific case.
> 
> What I'll do is fix the race condition by simplifying that code (haven't looked
> at the problem yet).
> 
> 
> Thanks,
>   Lluis
>