On 07/07/16 22:46, Sergey Fedorov wrote:
> On 07/07/16 22:36, Alex Bennée wrote:
>> Sergey Fedorov <sergey.fedo...@linaro.org> writes:
>>
>>> From: Sergey Fedorov <serge.f...@gmail.com>
>>>
>>> Signed-off-by: Sergey Fedorov <serge.f...@gmail.com>
>>> Signed-off-by: Sergey Fedorov <sergey.fedo...@linaro.org>
>>> ---
>>> cpu-exec.c | 15 +++++++++------
>>> 1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/cpu-exec.c b/cpu-exec.c
>>> index dd0bd5007701..54c935039592 100644
>>> --- a/cpu-exec.c
>>> +++ b/cpu-exec.c
>>> @@ -295,7 +295,8 @@ static TranslationBlock *tb_find_slow(CPUState *cpu,
>>>
>>> /* mmap_lock is needed by tb_gen_code, and mmap_lock must be
>>> * taken outside tb_lock. As system emulation is currently
>>> - * single threaded the locks are NOPs.
>>> + * single threaded the locks are NOPs. Both locks are to be
>>> + * released at the end of tb_find_fast().
>>> */
>>> mmap_lock();
>>> tb_lock();
>>> @@ -308,9 +309,6 @@ static TranslationBlock *tb_find_slow(CPUState *cpu,
>>> /* if no translated code available, then translate it now */
>>> tb = tb_gen_code(cpu, pc, cs_base, flags, 0);
>>> }
>>> -
>>> - tb_unlock();
>>> - mmap_unlock();
>> Hmm pushing these outside of tb_find_slow() makes me uncomfortable. I
>> guess tb_find_fast/slow are intimately tied together but the idea of
>> taking locks which are the responsibility of the calling function to
>> clear seems ugly to me.
> Okay, what if we also:
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 54c935039592..ff8f92bc1dc1 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -278,45 +278,7 @@ static TranslationBlock *tb_find_physical(CPUState
> *cpu,
> return qht_lookup(&tcg_ctx.tb_ctx.htable, tb_cmp, &desc, h);
> }
>
> -static TranslationBlock *tb_find_slow(CPUState *cpu,
> - target_ulong pc,
> - target_ulong cs_base,
> - uint32_t flags)
> -{
> - TranslationBlock *tb;
> -
> - /* Ensure that we won't find a TB in the shared hash table
> - * if it is being invalidated by some other thread.
> - * Otherwise we'd put it back to CPU's local cache.
> - * Pairs with smp_wmb() in tb_phys_invalidate(). */
> - smp_rmb();
> - tb = tb_find_physical(cpu, pc, cs_base, flags);
> - if (!tb) {
> -
> - /* mmap_lock is needed by tb_gen_code, and mmap_lock must be
> - * taken outside tb_lock. As system emulation is currently
> - * single threaded the locks are NOPs. Both locks are to be
> - * released at the end of tb_find_fast().
> - */
> - mmap_lock();
> - tb_lock();
> -
> - /* There's a chance that our desired tb has been translated while
> - * taking the locks so we check again inside the lock.
> - */
> - tb = tb_find_physical(cpu, pc, cs_base, flags);
> - if (!tb) {
> - /* if no translated code available, then translate it now */
> - tb = tb_gen_code(cpu, pc, cs_base, flags, 0);
> - }
> - }
> -
> - /* We add the TB in the virtual pc hash table for the fast lookup */
> - atomic_set(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)], tb);
> - return tb;
> -}
> -
> -static inline TranslationBlock *tb_find_fast(CPUState *cpu,
> +static inline TranslationBlock *tb_find(CPUState *cpu,
> TranslationBlock **last_tb,
> int tb_exit)
> {
> @@ -332,7 +294,34 @@ static inline TranslationBlock
> *tb_find_fast(CPUState *cpu,
> tb = atomic_read(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)]);
> if (unlikely(!tb || tb->pc != pc || tb->cs_base != cs_base ||
> tb->flags != flags)) {
> - tb = tb_find_slow(cpu, pc, cs_base, flags);
> + /* Ensure that we won't find a TB in the shared hash table
> + * if it is being invalidated by some other thread.
> + * Otherwise we'd put it back to CPU's local cache.
> + * Pairs with smp_wmb() in tb_phys_invalidate(). */
> + smp_rmb();
> + tb = tb_find_physical(cpu, pc, cs_base, flags);
> + if (!tb) {
> +
> + /* mmap_lock is needed by tb_gen_code, and mmap_lock must be
> + * taken outside tb_lock. As system emulation is currently
> + * single threaded the locks are NOPs. Both locks are to be
> + * released at the end of the function.
> + */
> + mmap_lock();
> + tb_lock();
> +
> + /* There's a chance that our desired tb has been translated
> while
> + * taking the locks so we check again inside the lock.
> + */
> + tb = tb_find_physical(cpu, pc, cs_base, flags);
> + if (!tb) {
> + /* if no translated code available, then translate it
> now */
> + tb = tb_gen_code(cpu, pc, cs_base, flags, 0);
> + }
> + }
> +
> + /* We add the TB in the virtual pc hash table for the fast
> lookup */
> + atomic_set(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)], tb);
> }
> if (cpu->tb_flushed) {
> /* Ensure that no TB jump will be modified as the
> @@ -440,7 +429,7 @@ static inline bool cpu_handle_exception(CPUState
> *cpu, int *ret)
> && cpu->icount_decr.u16.low + cpu->icount_extra == 0) {
> /* try to cause an exception pending in the log */
> TranslationBlock *last_tb = NULL; /* Avoid chaining TBs */
> - cpu_exec_nocache(cpu, 1, tb_find_fast(cpu, &last_tb, 0), true);
> + cpu_exec_nocache(cpu, 1, tb_find(cpu, &last_tb, 0), true);
> *ret = -1;
> return true;
> #endif
> @@ -624,7 +613,7 @@ int cpu_exec(CPUState *cpu)
> cpu->tb_flushed = false; /* reset before first TB lookup */
> for(;;) {
> cpu_handle_interrupt(cpu, &last_tb);
> - tb = tb_find_fast(cpu, &last_tb, tb_exit);
> + tb = tb_find(cpu, &last_tb, tb_exit);
> cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit, &sc);
> /* Try to align the host and virtual clocks
> if the guest is in advance */
>
>
Using goto statements, the lines from tb_find_slow() which take the
locks can even be on the same indentation level as the lines resetting
them. I could prepare a patch series based on your first 3 patches and
this idea.
Regards,
Sergey
>
>
>>> }
>>>
>>> /* We add the TB in the virtual pc hash table for the fast lookup */
>>> @@ -354,10 +352,15 @@ static inline TranslationBlock *tb_find_fast(CPUState
>>> *cpu,
>>> #endif
>>> /* See if we can patch the calling TB. */
>>> if (*last_tb && !qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
>>> - tb_lock();
>>> + if (!tb_lock_locked()) {
>>> + tb_lock();
>>> + }
>>> tb_add_jump(*last_tb, tb_exit, tb);
>>> - tb_unlock();
>>> }
>>> +
>>> + tb_lock_reset();
>>> + mmap_lock_reset();
>>> +
>>> return tb;
>>> }
>> --
>> Alex Bennée
