Re: [Qemu-devel] [4352] Make sure we flush cached blocks from the tb-jmp-cache when we replace valid tlb entries .

[Edgar E. Iglesias]

On Sat, May 24, 2008 at 01:31:21AM +0200, Fabrice Bellard wrote:
> Please revert this patch: it has a major performance hit because the
> tb_jmp_cache is flushed too often. Flushing the tb_jmp_cache when
> overriding a tlb_cache entry is not necessary, provided a given virtual
> address is always remapped at the same physical address with the same
> rights, which is the assumed API here. Detecting possible
> inconsistencies is useful, but I am sure there is a solution without
> such a performance hit.
> 
> Please avoid doing such modifications without doing regressions tests on
> performance (e.g. a compilation with gcc).


My misstake, unfortunately I only ran performance tests on CRIS. That flush 
was needed at the time in order to avoid a complete tlb_flush() every time the 
guest touched the TLB. IIRC those changes improved performance and got rid of 
bugs were TBs got executed even if their addresses were not in the TLB.

The flush might not be needed once the tb jump cache hash function got fixed,
but I havent had time to verify that.               

Thanks for clarifying the API.

Best regards

> 
> Regards,
> 
> Fabrice.
> 
> Edgar E. Iglesias wrote:
> > Revision: 4352
> >           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4352
> > Author:   edgar_igl
> > Date:     2008-05-06 08:44:21 +0000 (Tue, 06 May 2008)
> > 
> > Log Message:
> > -----------
> > Make sure we flush cached blocks from the tb-jmp-cache when we replace 
> > valid tlb entries.
> > 
> > Modified Paths:
> > --------------
> >     trunk/exec.c
> > 
> > Modified: trunk/exec.c
> > ===================================================================
> > --- trunk/exec.c    2008-05-06 08:38:22 UTC (rev 4351)
> > +++ trunk/exec.c    2008-05-06 08:44:21 UTC (rev 4352)
> > @@ -1366,6 +1366,21 @@
> >  
> >  #if !defined(CONFIG_USER_ONLY)
> >  
> > +static inline void tlb_flush_jmp_cache(CPUState *env, target_ulong addr)
> > +{
> > +    unsigned int i;
> > +
> > +    /* Discard jump cache entries for any tb which might potentially
> > +       overlap the flushed page.  */
> > +    i = tb_jmp_cache_hash_page(addr - TARGET_PAGE_SIZE);
> > +    memset (&env->tb_jmp_cache[i], 0, 
> > +       TB_JMP_PAGE_SIZE * sizeof(TranslationBlock *));
> > +
> > +    i = tb_jmp_cache_hash_page(addr);
> > +    memset (&env->tb_jmp_cache[i], 0, 
> > +       TB_JMP_PAGE_SIZE * sizeof(TranslationBlock *));
> > +}
> > +
> >  /* NOTE: if flush_global is true, also flush global entries (not
> >     implemented yet) */
> >  void tlb_flush(CPUState *env, int flush_global)
> > @@ -1428,7 +1443,6 @@
> >  void tlb_flush_page(CPUState *env, target_ulong addr)
> >  {
> >      int i;
> > -    TranslationBlock *tb;
> >  
> >  #if defined(DEBUG_TLB)
> >      printf("tlb_flush_page: " TARGET_FMT_lx "\n", addr);
> > @@ -1448,14 +1462,8 @@
> >  #endif
> >  #endif
> >  
> > -    /* Discard jump cache entries for any tb which might potentially
> > -       overlap the flushed page.  */
> > -    i = tb_jmp_cache_hash_page(addr - TARGET_PAGE_SIZE);
> > -    memset (&env->tb_jmp_cache[i], 0, TB_JMP_PAGE_SIZE * sizeof(tb));
> > +    tlb_flush_jmp_cache(env, addr);
> >  
> > -    i = tb_jmp_cache_hash_page(addr);
> > -    memset (&env->tb_jmp_cache[i], 0, TB_JMP_PAGE_SIZE * sizeof(tb));
> > -
> >  #if !defined(CONFIG_SOFTMMU)
> >      if (addr < MMAP_AREA_END)
> >          munmap((void *)addr, TARGET_PAGE_SIZE);
> > @@ -1706,6 +1714,10 @@
> >          } else {
> >              te->addr_read = -1;
> >          }
> > +
> > +        if (te->addr_code != -1) {
> > +            tlb_flush_jmp_cache(env, te->addr_code);
> > +        }
> >          if (prot & PAGE_EXEC) {
> >              te->addr_code = address;
> >          } else {
> > 
> > 
> > 
> > 
> > 
> 
> 

-- 
Edgar E. Iglesias
Axis Communications AB