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

[Paolo Bonzini]

On 19/10/2017 00:45, Emilio G. Cota wrote:
> On Mon, Oct 16, 2017 at 10:25:19 -0700, Richard Henderson wrote:
>> I've fixed two bugs within v5 of Emilio's patch set:
>>
>>  - The step_next_tb patch fixes the "rep movsb" bug that appeared
>>    when we included CF_COUNT_MASK into CF_HASH_MASK.  We had been
>>    relying on magic to single-step the next guest insn.
>>
>>  - The original "allocate optimizer temps with tcg_malloc" patch
>>    failed testing on arm32 host.  I didn't really look into exactly
>>    what was wrong because I had an older patch set that touched the
>>    same portion of the optimizer.
> 
> Thanks a lot for fixing these issues and respinning the series.
> 
> 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, 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?

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))

(also please make the iterator macros UPPERCASE)

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

Paolo

> These patches are a subset of the ones that I posted on the
> tb_lock removal patchset [1]. In particular, these patches are
> groundwork that doesn't change anything fundamental wrt locking,
> which does get tricky.
> 
> Given how close we are to the soft freeze for 2.11 [2], do you want
> me to post these patches on the list for review? Otherwise I can wait
> for the 2.12 dev cycle to post them with the complete tb_lock removal
> work.
> 
> That said, I think we should at least cherry-pick "translate-all: exit
> from tb_phys_invalidate if qht_remove fails" for 2.11, since it
> fixes a real bug. Stable should also get it.
> 
> Thanks,
> 
>               Emilio
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg01199.html
> [2] https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg02217.html
> 
>