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