diff mbox series

[v6,1/3] cputlb: do not evict empty entries to the vtlb

Message ID 20190114165017.27298-2-cota@braap.org
State New
Headers show
Series Dynamic TLB sizing | expand

Commit Message

Emilio Cota Jan. 14, 2019, 4:50 p.m. UTC
Currently we evict an entry to the victim TLB when it doesn't match
the current address. But it could be that there's no match because
the current entry is empty (i.e. all -1's, for instance via tlb_flush).
Do not evict the entry to the vtlb in that case.

This change will help us keep track of the TLB's use rate, which
we'll use to implement a policy for dynamic TLB sizing.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 include/exec/cpu-all.h | 9 +++++++++
 accel/tcg/cputlb.c     | 2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

Comments

Alex Bennée Jan. 16, 2019, 10:32 a.m. UTC | #1
Emilio G. Cota <cota@braap.org> writes:

> Currently we evict an entry to the victim TLB when it doesn't match
> the current address. But it could be that there's no match because
> the current entry is empty (i.e. all -1's, for instance via tlb_flush).
> Do not evict the entry to the vtlb in that case.
>
> This change will help us keep track of the TLB's use rate, which
> we'll use to implement a policy for dynamic TLB sizing.
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
>  include/exec/cpu-all.h | 9 +++++++++
>  accel/tcg/cputlb.c     | 2 +-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index 117d2fbbca..e21140049b 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -362,6 +362,15 @@ static inline bool tlb_hit(target_ulong tlb_addr, target_ulong addr)
>      return tlb_hit_page(tlb_addr, addr & TARGET_PAGE_MASK);
>  }
>
> +/**
> + * tlb_entry_is_empty - return true if the entry is not in use
> + * @te: pointer to CPUTLBEntry
> + */
> +static inline bool tlb_entry_is_empty(const CPUTLBEntry *te)
> +{
> +    return te->addr_read == -1 && te->addr_write == -1 && te->addr_code == -1;
> +}
> +

This breaks on a --disable-tcg build. I think the whole block needs to
be in a:

  #if !defined(CONFIG_USER_ONLY) && defined(CONFIG_TCG)

now.

>  void dump_exec_info(FILE *f, fprintf_function cpu_fprintf);
>  void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf);
>  #endif /* !CONFIG_USER_ONLY */
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index af6bd8ccf9..5dc97212a9 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -591,7 +591,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
>       * Only evict the old entry to the victim tlb if it's for a
>       * different page; otherwise just overwrite the stale data.
>       */
> -    if (!tlb_hit_page_anyprot(te, vaddr_page)) {
> +    if (!tlb_hit_page_anyprot(te, vaddr_page) && !tlb_entry_is_empty(te)) {
>          unsigned vidx = env->tlb_d[mmu_idx].vindex++ % CPU_VTLB_SIZE;
>          CPUTLBEntry *tv = &env->tlb_v_table[mmu_idx][vidx];


--
Alex Bennée
Emilio Cota Jan. 16, 2019, 4:53 p.m. UTC | #2
On Wed, Jan 16, 2019 at 10:32:08 +0000, Alex Bennée wrote:
> 
> Emilio G. Cota <cota@braap.org> writes:
> 
> > Currently we evict an entry to the victim TLB when it doesn't match
> > the current address. But it could be that there's no match because
> > the current entry is empty (i.e. all -1's, for instance via tlb_flush).
> > Do not evict the entry to the vtlb in that case.
> >
> > This change will help us keep track of the TLB's use rate, which
> > we'll use to implement a policy for dynamic TLB sizing.
> >
> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Emilio G. Cota <cota@braap.org>
> > ---
> >  include/exec/cpu-all.h | 9 +++++++++
> >  accel/tcg/cputlb.c     | 2 +-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> > index 117d2fbbca..e21140049b 100644
> > --- a/include/exec/cpu-all.h
> > +++ b/include/exec/cpu-all.h
> > @@ -362,6 +362,15 @@ static inline bool tlb_hit(target_ulong tlb_addr, target_ulong addr)
> >      return tlb_hit_page(tlb_addr, addr & TARGET_PAGE_MASK);
> >  }
> >
> > +/**
> > + * tlb_entry_is_empty - return true if the entry is not in use
> > + * @te: pointer to CPUTLBEntry
> > + */
> > +static inline bool tlb_entry_is_empty(const CPUTLBEntry *te)
> > +{
> > +    return te->addr_read == -1 && te->addr_write == -1 && te->addr_code == -1;
> > +}
> > +
> 
> This breaks on a --disable-tcg build. I think the whole block needs to
> be in a:
> 
>   #if !defined(CONFIG_USER_ONLY) && defined(CONFIG_TCG)
> 
> now.

Good catch, thanks! I'll send a v7 with this fixed.

Thanks,

		E.
diff mbox series

Patch

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 117d2fbbca..e21140049b 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -362,6 +362,15 @@  static inline bool tlb_hit(target_ulong tlb_addr, target_ulong addr)
     return tlb_hit_page(tlb_addr, addr & TARGET_PAGE_MASK);
 }
 
+/**
+ * tlb_entry_is_empty - return true if the entry is not in use
+ * @te: pointer to CPUTLBEntry
+ */
+static inline bool tlb_entry_is_empty(const CPUTLBEntry *te)
+{
+    return te->addr_read == -1 && te->addr_write == -1 && te->addr_code == -1;
+}
+
 void dump_exec_info(FILE *f, fprintf_function cpu_fprintf);
 void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf);
 #endif /* !CONFIG_USER_ONLY */
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index af6bd8ccf9..5dc97212a9 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -591,7 +591,7 @@  void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
      * Only evict the old entry to the victim tlb if it's for a
      * different page; otherwise just overwrite the stale data.
      */
-    if (!tlb_hit_page_anyprot(te, vaddr_page)) {
+    if (!tlb_hit_page_anyprot(te, vaddr_page) && !tlb_entry_is_empty(te)) {
         unsigned vidx = env->tlb_d[mmu_idx].vindex++ % CPU_VTLB_SIZE;
         CPUTLBEntry *tv = &env->tlb_v_table[mmu_idx][vidx];