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

[Paolo Bonzini]

On 19/10/2017 22:11, Emilio G. Cota wrote:
> 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.

Ok, this is clearer now.

>> 2) in tb_for_each_tagged_safe, could the "prev" argument instead be
>> "next", like
> 
> 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:

This actually makes a lot of sense.  Maybe we should change queue.h the
other way. ;)

Can you rename "prev" to "pprev" though?

Paolo