Re: [Qemu-devel] [PATCH 22/22] translate-all: do not hold tb_lock during code generation in softmmu

[Richard Henderson <rth@twiddle.net>]

On 07/08/2017 09:50 PM, Emilio G. Cota wrote:
> +    if (!have_tb_lock) {
> +        TranslationBlock *t;
> +
> +        tb_lock();
> +        /*
> +         * There's a chance that our desired tb has been translated while
> +         * we were translating it.
> +         */
> +        t = tb_htable_lookup(cpu, pc, cs_base, flags);
> +        if (unlikely(t)) {
> +            /* discard what we just translated */
> +            uintptr_t orig_aligned = (uintptr_t)gen_code_buf;
> +
> +            orig_aligned -= ROUND_UP(sizeof(*tb), qemu_icache_linesize);
> +            atomic_set(&tcg_ctx.code_gen_ptr, orig_aligned);
> +            return t;
> +        }
> +    }
>       /* As long as consistency of the TB stuff is provided by tb_lock in user
>        * mode and is implicit in single-threaded softmmu emulation, no explicit
>        * memory barrier is required before tb_link_page() makes the TB visible

I think it would be better to have a tb_htable_lookup_or_insert function, which 
performs the insert iff a matching object isn't already there, returning the 
entry which *is* there in either case.

The hash table already has per-bucket locking.  So here you're taking 3 locks 
(tb_lock, bucket lock for lookup, bucket lock for insert) instead of just 
taking the bucket lock once.  And, recall, the htable is designed such that the 
buckets do not contend often, so you ought to be eliminating most of the 
contention that you're seeing on tb_lock.

It might also be helpful to merge tb_link_page into its one caller in order to 
make the locking issues within that subroutine less muddled.

Anyway, you'd wind up with

   TranslationBlock *ret;

   ret = tb_htable_lookup_or_insert(arguments);
   if (unlikely(ret != tb)) {
      /* discard what we just translated */
   }

   return ret;


r~