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
