Re: [PATCH for-7.2 14/21] accel/tcg: Hoist get_page_addr_code out of tb_lookup

[Ilya Leoshkevich]

On Wed, 2022-08-17 at 08:38 -0500, Richard Henderson wrote:
> On 8/17/22 08:27, Ilya Leoshkevich wrote:
> > On Wed, 2022-08-17 at 08:15 -0500, Richard Henderson wrote:
> > > On 8/17/22 06:08, Ilya Leoshkevich wrote:
> > > > @@ -2243,6 +2250,13 @@ void page_set_flags(target_ulong start,
> > > > target_ulong end, int flags)
> > > >                (flags & PAGE_WRITE) &&
> > > >                p->first_tb) {
> > > >                tb_invalidate_phys_page(addr, 0);
> > > > +        } else {
> > > > +            TranslationBlock *tb;
> > > > +            int n;
> > > > +
> > > > +            PAGE_FOR_EACH_TB(p, tb, n) {
> > > > +                cpu_tb_jmp_cache_remove(tb);
> > > > +            }
> > > >            }
> > > 
> > > Here you would use tb_jmp_cache_clear_page(), which should be
> > > moved
> > > out of cputlb.c.
> > 
> > That was actually the first thing I tried.
> > 
> > Unfortunately tb_jmp_cache_clear_page() relies on
> > tb_jmp_cache_hash_func() returning the same top bits for addresses
> > on
> > the same page.  This is not the case for qemu-user: there this
> > property
> > was traded for better hashing with quite impressive performance
> > improvements (6f1653180f570).
> 
> Oh my.  Well, we could
> 
> (1) revert that patch because the premise is wrong,
> (2) go with your per-tb clearing,
> (3) clear the whole thing with cpu_tb_jmp_cache_clear
> 
> Ideally we'd have some benchmark numbers to inform the choice...

FWIW 6f1653180f570 still looks useful.
Reverting it caused 620.omnetpp_s to take ~4% more time.
I ran the benchmark with reduced values in omnetpp.ini so as not to
wait forever, therefore the real figures might be closer to what the
commit message says. In any case this still shows that the patch has
measurable impact.