[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v3 13/13] softmmu_template.h: move to multithreadi
From: |
alvise rigo |
Subject: |
Re: [Qemu-devel] [RFC v3 13/13] softmmu_template.h: move to multithreading |
Date: |
Fri, 17 Jul 2015 18:19:37 +0200 |
On Fri, Jul 17, 2015 at 5:57 PM, Alex Bennée <address@hidden> wrote:
>
> Alvise Rigo <address@hidden> writes:
>
>> Exploiting the tcg_excl_access_lock, port the helper_{le,be}_st_name to
>> work in real multithreading.
>>
>> - The macro lookup_cpus_ll_addr now uses directly the
>> env->excl_protected_addr to invalidate others' LL/SC operations
>>
>> Suggested-by: Jani Kokkonen <address@hidden>
>> Suggested-by: Claudio Fontana <address@hidden>
>> Signed-off-by: Alvise Rigo <address@hidden>
>> ---
>> softmmu_template.h | 110
>> +++++++++++++++++++++++++++++++++++++++++++----------
>> 1 file changed, 89 insertions(+), 21 deletions(-)
>>
>> diff --git a/softmmu_template.h b/softmmu_template.h
>> index bc767f6..522454f 100644
>> --- a/softmmu_template.h
>> +++ b/softmmu_template.h
>> @@ -141,21 +141,24 @@
>> vidx >= 0;
>> \
>> })
>>
>> +#define EXCLUSIVE_RESET_ADDR ULLONG_MAX
>> +
>> +/* This macro requires the caller to have the tcg_excl_access_lock lock
>> since
>> + * it modifies the excl_protected_hwaddr of a running vCPU.
>> + * The macros scans all the excl_protected_hwaddr of all the vCPUs and
>> compare
>> + * them with the address the current vCPU is writing to. If there is a
>> match,
>> + * we reset the value, making the SC fail. */
>
> It would have been nice if we had started with a comment when the
> function^H^H^H^H^H macro was first introduced and then updated here.
OK. Related to this, I think to refactor the patches in such a way to
drop the "move to multithreading" part, since it makes things more
confusing (even if my intent was the opposite).
>
>> #define lookup_cpus_ll_addr(addr)
>> \
>> ({
>> \
>> CPUState *cpu;
>> \
>> CPUArchState *acpu;
>> \
>> - bool hit = false;
>> \
>>
>> \
>> CPU_FOREACH(cpu) {
>> \
>> acpu = (CPUArchState *)cpu->env_ptr;
>> \
>> if (cpu != current_cpu && acpu->excl_protected_hwaddr == addr) {
>> \
>> - hit = true;
>> \
>> - break;
>> \
>> + acpu->excl_protected_hwaddr = EXCLUSIVE_RESET_ADDR;
>> \
>> }
>> \
>> }
>> \
>> -
>> \
>> - hit;
>> \
>> })
>
> My comment about using an inline function in the earlier patch stands.
>
>>
>> #ifndef SOFTMMU_CODE_ACCESS
>> @@ -439,18 +442,52 @@ void helper_le_st_name(CPUArchState *env, target_ulong
>> addr, DATA_TYPE val,
>> * exclusive-protected memory. */
>> hwaddr hw_addr = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
>>
>> - bool set_to_dirty;
>> -
>> /* Two cases of invalidation: the current vCPU is writing to
>> another
>> * vCPU's exclusive address or the vCPU that issued the
>> LoadLink is
>> * writing to it, but not through a StoreCond. */
>> - set_to_dirty = lookup_cpus_ll_addr(hw_addr);
>> - set_to_dirty |= env->ll_sc_context &&
>> - (env->excl_protected_hwaddr == hw_addr);
>> + qemu_mutex_lock(&tcg_excl_access_lock);
>> +
>> + /* The macro lookup_cpus_ll_addr could have reset the exclusive
>> + * address. Fail the SC in this case.
>> + * N.B.: Here excl_succeeded == 0 means that we don't come from
>> a
>> + * store conditional. */
>> + if (env->excl_succeeded &&
>> + (env->excl_protected_hwaddr ==
>> EXCLUSIVE_RESET_ADDR)) {
>> + env->excl_succeeded = 0;
>> + qemu_mutex_unlock(&tcg_excl_access_lock);
>> +
>> + return;
>> + }
>> +
>> + lookup_cpus_ll_addr(hw_addr);
>
> Add comment for side effect, also confused by the above comment given we
> call lookups_cpus_ll_addr() after the comment about it tweaking
> excl_succedded.
I will improve all the comments to avoid any misinterpretation. I will
also be more verbose in the cover letter about how the whole machinery
works.
>
>> +
>> + if (!env->excl_succeeded) {
>> + if (env->ll_sc_context &&
>> + (env->excl_protected_hwaddr == hw_addr)) {
>> + cpu_physical_memory_set_excl_dirty(hw_addr);
>> + }
>> + } else {
>> + if (cpu_physical_memory_excl_is_dirty(hw_addr) ||
>> + env->excl_protected_hwaddr != hw_addr) {
>> + env->excl_protected_hwaddr = EXCLUSIVE_RESET_ADDR;
>> + qemu_mutex_unlock(&tcg_excl_access_lock);
>> + env->excl_succeeded = 0;
>> +
>> + return;
>> + }
>> + }
>
> I'm wondering if it can be more naturally written the other way round to
> aid comprehension:
>
> if (env->excl_succeeded) {
> if (cpu_physical_memory_excl_is_dirty(hw_addr) ||
> env->excl_protected_hwaddr != hw_addr) {
> ..do stuff..
> return
> }
> } else {
> if (env->ll_sc_context &&
> (env->excl_protected_hwaddr == hw_addr)) {
> cpu_physical_memory_set_excl_dirty(hw_addr);
> }
> }
>
> Although now I'm confused as to why we push on in 3 of the 4 cases.
I will try to find another way to write this code, starting from
adding more comments about the corner cases that we are addressing.
>
>> +
>> + haddr = addr + env->tlb_table[mmu_idx][index].addend;
>> + #if DATA_SIZE == 1
>> + glue(glue(st, SUFFIX), _p)((uint8_t *)haddr, val);
>> + #else
>> + glue(glue(st, SUFFIX), _le_p)((uint8_t *)haddr, val);
>> + #endif
>> +
>> + env->excl_protected_hwaddr = EXCLUSIVE_RESET_ADDR;
>> + qemu_mutex_unlock(&tcg_excl_access_lock);
>>
>> - if (set_to_dirty) {
>> - cpu_physical_memory_set_excl_dirty(hw_addr);
>> - } /* the vCPU is legitimately writing to the protected address
>> */
>> + return;
>> } else {
>> if ((addr & (DATA_SIZE - 1)) != 0) {
>> goto do_unaligned_access;
>> @@ -537,18 +574,49 @@ void helper_be_st_name(CPUArchState *env, target_ulong
>> addr, DATA_TYPE val,
>> * exclusive-protected memory. */
>> hwaddr hw_addr = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
>>
>> - bool set_to_dirty;
>> -
>> /* Two cases of invalidation: the current vCPU is writing to
>> another
>> * vCPU's exclusive address or the vCPU that issued the
>> LoadLink is
>> * writing to it, but not through a StoreCond. */
>> - set_to_dirty = lookup_cpus_ll_addr(hw_addr);
>> - set_to_dirty |= env->ll_sc_context &&
>> - (env->excl_protected_hwaddr == hw_addr);
>> + qemu_mutex_lock(&tcg_excl_access_lock);
>> +
>> + /* The macro lookup_cpus_ll_addr could have reset the exclusive
>> + * address. Fail the SC in this case.
>> + * N.B.: Here excl_succeeded == 0 means that we don't come from
>> a
>> + * store conditional. */
>> + if (env->excl_succeeded &&
>> + (env->excl_protected_hwaddr ==
>> EXCLUSIVE_RESET_ADDR)) {
>> + env->excl_succeeded = 0;
>> + qemu_mutex_unlock(&tcg_excl_access_lock);
>> +
>> + return;
>> + }
>> +
>> + lookup_cpus_ll_addr(hw_addr);
>> +
>> + if (!env->excl_succeeded) {
>> + if (env->ll_sc_context &&
>> + (env->excl_protected_hwaddr == hw_addr)) {
>> + cpu_physical_memory_set_excl_dirty(hw_addr);
>> + }
>> + } else {
>> + if (cpu_physical_memory_excl_is_dirty(hw_addr) ||
>> + env->excl_protected_hwaddr != hw_addr) {
>> + env->excl_protected_hwaddr = EXCLUSIVE_RESET_ADDR;
>> + qemu_mutex_unlock(&tcg_excl_access_lock);
>> + env->excl_succeeded = 0;
>> +
>> + return;
>> + }
>> + }
>
> Given the amount of copy/paste between the two functions I wonder if
> there is some commonality to be re-factored out here?
Either we create inline functions outside this file (like in cputlb.c)
or we define new macros.
I don't see other viable ways to re-factor this code.
Thank you,
alvise
>
>>
>> - if (set_to_dirty) {
>> - cpu_physical_memory_set_excl_dirty(hw_addr);
>> - } /* the vCPU is legitimately writing to the protected address
>> */
>> + haddr = addr + env->tlb_table[mmu_idx][index].addend;
>> +
>> + glue(glue(st, SUFFIX), _be_p)((uint8_t *)haddr, val);
>> +
>> + qemu_mutex_unlock(&tcg_excl_access_lock);
>> + env->excl_protected_hwaddr = EXCLUSIVE_RESET_ADDR;
>> +
>> + return;
>> } else {
>> if ((addr & (DATA_SIZE - 1)) != 0) {
>> goto do_unaligned_access;
>
> --
> Alex Bennée
- Re: [Qemu-devel] [RFC v3 09/13] cpus.c: introduce simple callback support, (continued)
[Qemu-devel] [RFC v3 11/13] Introduce exit_flush_req and tcg_excl_access_lock, Alvise Rigo, 2015/07/10
[Qemu-devel] [RFC v3 10/13] Simple TLB flush wrap to use as exit callback, Alvise Rigo, 2015/07/10
[Qemu-devel] [RFC v3 13/13] softmmu_template.h: move to multithreading, Alvise Rigo, 2015/07/10
[Qemu-devel] [RFC v3 12/13] softmmu_llsc_template.h: move to multithreading, Alvise Rigo, 2015/07/10
Re: [Qemu-devel] [RFC v3 00/13] Slow-path for atomic instruction translation, Mark Burton, 2015/07/10
Re: [Qemu-devel] [RFC v3 00/13] Slow-path for atomic instruction translation, Frederic Konrad, 2015/07/10