Re: [Qemu-devel] Making all TB invalidation asynchronous (MTTCG safe_work)?

[Frederic Konrad <fred.konrad@greensocs.com>]

Hi Alex,

We decided in Seattle to make this flag per tb (eg move it to the tb
struct).



On 24/02/2016 18:30, Alex Bennée wrote:
> Hi,
>
> So I've been working on reducing MTTCG tb_lock contention and currently
> have a tb_lock around the following code (in my cpu_exec):
>
>     /* Note: we do it here to avoid a gcc bug on Mac OS X when
>        doing it in tb_find_slow */
>     tb_lock();
>     if (tcg_ctx.tb_ctx.tb_invalidated_flag) {
>         /* as some TB could have been invalidated because
>            of memory exceptions while generating the code, we
>            must recompute the hash index here */
>         next_tb = 0;
>         tcg_ctx.tb_ctx.tb_invalidated_flag = 0;
>     }
>     /* see if we can patch the calling TB. When the TB
>        spans two pages, we cannot safely do a direct
>        jump. */
>     if (next_tb != 0 && tb->page_addr[1] == -1
>         && !qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
>         tb_add_jump((TranslationBlock *)(next_tb & ~TB_EXIT_MASK),
>                     next_tb & TB_EXIT_MASK, tb);
>     }
>     tb_unlock();
>
> And this started me down the rabbit hole of the meaning of
> tcg_ctx.tb_ctx.tb_invalidated_flag. So as far as I follow there are two
> places this is set:
>
>  * We've run out of translation memory and we are throwing everything
>    away (tb_alloc == NULL)
>  * We've invalidated the physical pages of some TranslationBlocks
>
> The first case there is a slightly convoluted buffer overflow handing
> code (tb_gen_code):
>
>     if (unlikely(!tb)) {
>  buffer_overflow:
>         /* flush must be done */
>         tb_flush_safe(cpu);
>         /* Don't forget to invalidate previous TB info.  */
>         tcg_ctx.tb_ctx.tb_invalidated_flag = 1;
>         tb_unlock();
>         cpu_loop_exit(cpu);
>     }
>
> Which I'm sure could be more simply handled by just queuing the safe
> tb_flush and returning a NULL tb and letting the execution loop unwind
> before resetting the translation buffers.
>
> The second case has been partially asynced by Fred:
>
>     /* invalidate one TB */
>     void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
>     {
>         CPUState *cpu;
>         PageDesc *p;
>         unsigned int h;
>         tb_page_addr_t phys_pc;
>         struct CPUDiscardTBParams *params;
>
>         assert_tb_lock(); /* added by me because of bellow */
>
>         /* remove the TB from the hash list */
>         phys_pc = tb->page_addr[0] + (tb->pc & ~TARGET_PAGE_MASK);
>         h = tb_phys_hash_func(phys_pc);
>         tb_hash_remove(&tcg_ctx.tb_ctx.tb_phys_hash[h], tb);
>
>         /* remove the TB from the page list */
>         if (tb->page_addr[0] != page_addr) {
>             p = page_find(tb->page_addr[0] >> TARGET_PAGE_BITS);
>             tb_page_remove(&p->first_tb, tb);
>             invalidate_page_bitmap(p);
>         }
>         if (tb->page_addr[1] != -1 && tb->page_addr[1] != page_addr) {
>             p = page_find(tb->page_addr[1] >> TARGET_PAGE_BITS);
>             tb_page_remove(&p->first_tb, tb);
>             invalidate_page_bitmap(p);
>         }
>
>         tcg_ctx.tb_ctx.tb_invalidated_flag = 1;
>
>         CPU_FOREACH(cpu) {
>             params = g_malloc(sizeof(struct CPUDiscardTBParams));
>             params->cpu = cpu;
>             params->tb = tb;
>             async_run_on_cpu(cpu, cpu_discard_tb_from_jmp_cache, params);
>         }
>         async_run_safe_work_on_cpu(first_cpu, tb_invalidate_jmp_remove, tb);
>
>         tcg_ctx.tb_ctx.tb_phys_invalidate_count++;
>     }
>
> But I'm wondering why we can't defer all the page invalidation to safe
> work?
>
> I don't think it matters to the invalidating vCPU as it has to get
> to the end of its block anyway. For other vCPUs as there is no strict
> synchronisation can we not pretend what ever the operation was that
> triggered the invalidation happened just as the block ended?
>
> The final case I don't quite follow is the avoiding invalidation of
> tb_next in cpu_exec_nocache() if we have already caused a tb
> invalidation event:
>
>     tb->orig_tb = tcg_ctx.tb_ctx.tb_invalidated_flag ? NULL : orig_tb;
>
> Which is later (in cpu_io_recompile):
>
>     if (tb->cflags & CF_NOCACHE) {
>         if (tb->orig_tb) {
>             /* Invalidate original TB if this TB was generated in
>              * cpu_exec_nocache() */
>             tb_phys_invalidate(tb->orig_tb, -1);
>         }
>         tb_free(tb);
>     }
>
> My aim in all of this is to see if we can remove another flag from
> tb_ctx (one less thing to mutex access to) and make the code flow easier
> to follow. So remaining question:
>
> * Are there cases where not immediately invalidating the tb_page
>   structures would cause problems for the emulation?

Is that the same issue we might have with the memory barriers?

Fred
>
> Thanks in advance for any elucidation ;-)
>
> --
> Alex Bennée
>