Re: [Qemu-devel] [PATCH v6 00/50] tcg tb_lock removal

[Emilio G. Cota]

On Thu, Oct 19, 2017 at 15:05:17 +0200, Paolo Bonzini wrote:
> On 19/10/2017 00:45, Emilio G. Cota wrote:
> > I have just pushed a branch on top of this series that includes
> > 10 patches that further pave the way for the removal of tb_lock:
> > 
> >   https://github.com/cota/qemu/tree/multi-tcg-v6-plus
> 
> I started reviewing those,

Nice, thanks!

> I have a few questions:
> 
> 1) why is tcg_region_tree separate from tcg_region_state?  Would it make
> sense to prepare a linked list of tcg_region_state structs, and reuse
> the region lock for the region tree?

I think the naming here might be confusing; "tcg_region_state" should be
understood as "tcg_region_global_state". IOW, there is no per-region struct.

That said, the array of per-region trees could be embedded in this global
struct. I was hesitant to do so because then one could think that
region_state.lock and rt.lock are somehow related; they are not.

> 2) in tb_for_each_tagged_safe, could the "prev" argument instead be
> "next", like
> 
> 
> +    for (n = (head) & 1,                                        \
> +             tb = (TranslationBlock *)((head) & ~1);            \
> +         tb && ((next = (TranslationBlock *)tb->field[n]), 1);  \
> +             n = (uintptr_t)next & 1,                           \
> +             tb = (TranslationBlock *)((uintptr_t)next & ~1))

Is this just to make them closer to the macros in queue.h?

In this case tracking *prev in the loop (rather than next) is
useful because it makes removing the "current" element very simple:

static inline void tb_page_remove(PageDesc *pd, TranslationBlock *tb)
{
    TranslationBlock *tb1;
    uintptr_t *prev;
    unsigned int n1;

    page_for_each_tb_safe(pd, tb1, n1, prev) {
        if (tb1 == tb) {
            *prev = tb1->page_next[n1];
            return;
        }
    }
    g_assert_not_reached();
}

If we wanted to use something similar to QSLIST_REMOVE_AFTER, we'd
have to track three pointers instead of two: prev (tracked by the caller),
current and next (these two as part of the for loop).

> (also please make the iterator macros UPPERCASE)

Will do.

> 3) "translate-all: exit from tb_phys_invalidate if qht_remove fails" may
> be worth posting now?

I'll post it to be included in the next iteration of this series.

Thanks,

                Emilio