Re: [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire

[Paolo Bonzini]

On 27/01/2017 07:09, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:address@hidden
>> On 26/01/2017 15:32, Pavel Dovgalyuk wrote:
>>>> From: Paolo Bonzini [mailto:address@hidden
>>>> On 26/01/2017 14:37, Pavel Dovgalyuk wrote:
>>>>>> Simpler:
>>>>>>
>>>>>>  use_icount &&
>>>>>>  ((int32_t)cpu->icount_decr.u32 < 0 ||
>>>>>>   cpu->icount_decr.u16.low + cpu->icount_extra == 0)
>>>>> Right.
>>>>>
>>>>>> But I'm not sure that you need to test u32.  After all you're not
>>>>> Checking u32 is needed, because sometimes it is less than zero.
>>>>
>>>> If cpu->icount_decr.u32 is less than zero, the next translation block
>>>> would immediately exit with TB_EXIT_ICOUNT_EXPIRED, causing
>>>>
>>>>             cpu->exception_index = EXCP_INTERRUPT;
>>>>             *last_tb = NULL;       
>>>>             cpu_loop_exit(cpu);
>>>>
>>>> from cpu_loop_exec_tb's "case TB_EXIT_ICOUNT_EXPIRED".
>>>>
>>>> And the same is true for cpu->icount_decr.u16.low + cpu->icount_extra ==
>>>> 0, so I don't understand why this part of the patch is necessary.
>>>
>>> I removed that lines because we have to check icount=0 not only when it is 
>>> expired,
>>> but also when all instructions were executed successfully.
>>> If there are no instructions to execute, calling tb_find (and translation 
>>> then)
>>> may cause an exception at the wrong moment.
>>
>> Ok, that makes sense for cpu->icount_decr.u16.low + cpu->icount_extra == 0.
>>
>> But for decr.u32 < 0, the same reasoning of this comment is also true:
>>
>>         /* Something asked us to stop executing
>>          * chained TBs; just continue round the main
>>          * loop. Whatever requested the exit will also
>>          * have set something else (eg exit_request or
>>          * interrupt_request) which we will handle
>>          * next time around the loop.  But we need to
>>          * ensure the tcg_exit_req read in generated code
>>          * comes before the next read of cpu->exit_request
>>          * or cpu->interrupt_request.
>>          */
> 
> Right. If the following lines will not be removed (as opposite to my patch) 
> then checking
> decr.u32 < 0 will not be needed.

That's what I'm not sure about.  u32 < 0 is only true if you have set
the interrupt_request as well, but interrupt requests are processed in
cpu_handle_interrupt and that doesn't require going back to the main loop.

Let me try some cleanups early next week and come back to you with a
patch to base your work on.

Paolo

> -             cpu->exception_index = EXCP_INTERRUPT;
> -             *last_tb = NULL;        
> -             cpu_loop_exit(cpu);
> 
> What is your point about the new version of that patch?
> 
> Pavel Dovgalyuk
> 
> 
>