Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag

[Alex Bennée]

Sergey Fedorov <address@hidden> writes:

> On 18/04/16 17:09, Alex Bennée wrote:
>> Sergey Fedorov <address@hidden> writes:
>>> From: Sergey Fedorov <address@hidden>
>>>
>>> 'tb_invalidated_flag' was meant to catch two events:
>>>  * some TB has been invalidated by tb_phys_invalidate();
>>>  * the whole translation buffer has been flushed by tb_flush().
>>>
>>> Then it was checked:
>>>  * in cpu_exec() to ensure that the last executed TB can be safely
>>>    linked to directly call the next one;
>>>  * in cpu_exec_nocache() to decide if the original TB should be provided
>>>    for further possible invalidation along with the temporarily
>>>    generated TB.
>>>
>>> It is always safe to patch an invalidated TB since it is not going to be
>>> used anyway.
>> Wouldn't that have implications for code searching through the linked
>> list of jump patched TBs?
>
> The only implication I can see is that the jump in that already
> invalidated TB could just get reset back later on in
> tb_phys_invalidate(). We could keep track of invalidated TB's and skip
> patching those but it's also some overhead in the main CPU execution
> loop wich I'm not sure is worth to be introduced.
>
> (snip)
>>> diff --git a/cpu-exec.c b/cpu-exec.c
> (snip)
>>> @@ -507,14 +510,12 @@ int cpu_exec(CPUState *cpu)
>>>                  }
>>>                  tb_lock();
>>>                  tb = tb_find_fast(cpu);
>>> -                /* Note: we do it here to avoid a gcc bug on Mac OS X when
>>> -                   doing it in tb_find_slow */
>> Is this still true? Would it make more sense to push the patching down
>> to the gen_code?
>
> This comment comes up to the commit:
>
>     commit 1538800276aa7228d74f9d00bf275f54dc9e9b43
>     Author: bellard <address@hidden>
>     Date:   Mon Dec 19 01:42:32 2005 +0000
>
>         workaround for gcc bug on PowerPC
>
>
> It was added more than ten years ago. Anyway, now this code is here not
> because of the bug: we need to reset 'next_tb' which is a local variable
> in cpu_exec(). Personally, I don't think it would be neater to hide it
> into gen_code(). Do you have some thoughts on how we could benefit from
> doing so? BTW, I had a feeling that it may be useful to reorganize
> cpu_exec() a bit, although I don't have a solid idea of how to do this
> so far.

I'm mainly eyeing the tb_lock/unlock which would be nice to push further
down the call chain if we can, especially if the need to lock
tb_find_fast can be removed later on.

>>
>> I got slightly confused as to what next_tb ends up meaning at what point
>> in the run loop.
>
> Yes, it seems to be a misleading name for this variable. As it was
> discussed on IRC, I'd like to break it into two variables, say 'last_tb'
> and 'tb_exit_idx', as soon as cpu_tb_exec() returns. Probably this
> series could also include such a patch.

Yes this would be a worthwhile separate patch.

>
> Kind regards,
> Sergey


--
Alex Bennée