[v1,1/3] cputlb: ensure tbl_set_dirty1 updates addr_write atomically

Message ID 20170320153441.2181-2-alex.bennee@linaro.org
State New
Headers show
Series
  • MTTCG regression fixes for 2.9-rc1
Related show

Commit Message

Alex Bennée March 20, 2017, 3:34 p.m.
This was an oversight when the rest of cputlb was being updated. As
before it falls back to the non-atomic version when the host can't
support wider-than-bus atomics.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
 cputlb.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.11.0

Comments

Paolo Bonzini March 20, 2017, 3:43 p.m. | #1
On 20/03/2017 16:34, Alex Bennée wrote:
>  static inline void tlb_set_dirty1(CPUTLBEntry *tlb_entry, target_ulong vaddr)

>  {

> +#if TCG_OVERSIZED_GUEST

>      if (tlb_entry->addr_write == (vaddr | TLB_NOTDIRTY)) {

>          tlb_entry->addr_write = vaddr;

>      }

> +#else

> +    uintptr_t orig_addr = atomic_mb_read(&tlb_entry->addr_write);


atomic_read is enough, since we don't care at all about cases where the
address doesn't match.  Otherwise

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>


Paolo

> +    if (orig_addr == (vaddr | TLB_NOTDIRTY)) {

> +        atomic_cmpxchg(&tlb_entry->addr_write, orig_addr, vaddr);

> +    }

> +#endif

>  }
Alex Bennée March 20, 2017, 4:16 p.m. | #2
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 20/03/2017 16:34, Alex Bennée wrote:

>>  static inline void tlb_set_dirty1(CPUTLBEntry *tlb_entry, target_ulong vaddr)

>>  {

>> +#if TCG_OVERSIZED_GUEST

>>      if (tlb_entry->addr_write == (vaddr | TLB_NOTDIRTY)) {

>>          tlb_entry->addr_write = vaddr;

>>      }

>> +#else

>> +    uintptr_t orig_addr = atomic_mb_read(&tlb_entry->addr_write);

>

> atomic_read is enough, since we don't care at all about cases where the

> address doesn't match.  Otherwise


Good catch. Will fix for my pullreq

>

> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

>

> Paolo

>

>> +    if (orig_addr == (vaddr | TLB_NOTDIRTY)) {

>> +        atomic_cmpxchg(&tlb_entry->addr_write, orig_addr, vaddr);

>> +    }

>> +#endif

>>  }



--
Alex Bennée
Richard Henderson March 20, 2017, 9:49 p.m. | #3
On 03/21/2017 01:34 AM, Alex Bennée wrote:
> This was an oversight when the rest of cputlb was being updated. As

> before it falls back to the non-atomic version when the host can't

> support wider-than-bus atomics.

>

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> ---

>  cputlb.c | 8 ++++++++

>  1 file changed, 8 insertions(+)

>

> diff --git a/cputlb.c b/cputlb.c

> index f5d056cc08..0d52e45dfd 100644

> --- a/cputlb.c

> +++ b/cputlb.c

> @@ -540,9 +540,17 @@ void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length)

>

>  static inline void tlb_set_dirty1(CPUTLBEntry *tlb_entry, target_ulong vaddr)

>  {

> +#if TCG_OVERSIZED_GUEST

>      if (tlb_entry->addr_write == (vaddr | TLB_NOTDIRTY)) {

>          tlb_entry->addr_write = vaddr;

>      }

> +#else

> +    uintptr_t orig_addr = atomic_mb_read(&tlb_entry->addr_write);

> +

> +    if (orig_addr == (vaddr | TLB_NOTDIRTY)) {

> +        atomic_cmpxchg(&tlb_entry->addr_write, orig_addr, vaddr);

> +    }


What's the state machine here?  How can the per-cpu tlb change in a way other 
than dirty->clean / clean->dirty?  AFAIK, we shouldn't be swapping out the tlb 
entry to somthing completely different.

So how does cmpxchg improve over atomic_write?


r~

Patch hide | download patch | download mbox

diff --git a/cputlb.c b/cputlb.c
index f5d056cc08..0d52e45dfd 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -540,9 +540,17 @@  void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length)
 
 static inline void tlb_set_dirty1(CPUTLBEntry *tlb_entry, target_ulong vaddr)
 {
+#if TCG_OVERSIZED_GUEST
     if (tlb_entry->addr_write == (vaddr | TLB_NOTDIRTY)) {
         tlb_entry->addr_write = vaddr;
     }
+#else
+    uintptr_t orig_addr = atomic_mb_read(&tlb_entry->addr_write);
+
+    if (orig_addr == (vaddr | TLB_NOTDIRTY)) {
+        atomic_cmpxchg(&tlb_entry->addr_write, orig_addr, vaddr);
+    }
+#endif
 }
 
 /* update the TLB corresponding to virtual page vaddr