Re: [Qemu-devel] [PATCH] atomics: add volatile_read/volatile_set

[Sergey Fedorov]

On 18/07/16 20:58, Paolo Bonzini wrote:
>
> On 18/07/2016 19:31, Sergey Fedorov wrote:
>> On 18/07/16 20:28, Paolo Bonzini wrote:
>>> On 18/07/2016 19:25, Sergey Fedorov wrote:
>>>>>> @@ -753,14 +753,14 @@ static inline void 
>>>>>> cpu_get_invalid_tb_cpu_state(target_ulong *pc,
>>>>>>                                                  target_ulong *cs_base,
>>>>>>                                                  uint32_t *flags)
>>>>>>  {
>>>>>> -    *cs_base = -1; /* npc must be a multible of 4 */
>>>>>> +    *flags = TB_FLAG_MMU_MASK;
>>>>>>  }
>>>> Hmm, not sure if it is really simpler to follow. Maybe " |= 1;" anyway?
>>> |= 1 has the problem that tb_mark_invalid doesn't pass TB's tuple into
>>> cpu_get_invalid_tb_cpu_state, and I didn't want to change that.  I'll
>>> add a comment,
>>>
>>>     /* TB_FLAG_MMU_MASK is not a valid MMU index, which makes it is an
>>>      * impossible flag combination for valid TBs.
>>>      */
>>>
>> I wonder if using a dedicated field to mark TBs invalid would be so slow
>> that we couldn't afford it...
> We could, but it would be slower too. :)

How much performance do we really need and how much performance can we
loose introducing such a flag? We should yet gain something reducing
tb_lock contention. Maybe it is worthwhile to use a dedicated flag to
keep code more clear? There's always a question of balance between code
structure and maintainability (future of the TCG) on one hand and
performance (present of the TCG) on the other hand.

Kind regards,
Sergey

>
> "Just make flags=-1 invalid" is probably a valid one too.  There are
> many ways to implement it: use less than 32 bits (or equivalently
> reserve one bit for invalid TBs), ensure some combos are invalid, etc.
> It would probably be valid for all current targets, since no one uses 32
> bits.  ARM is the closest to exhausting flag space probably.
>
> Still, I like your patches very much so I'd like to proceed with them,
> only with some changes to fix compilation on 32-bit hosts.
>
> Paolo