Re: [Xen-devel] [PATCH v3 7/7] x86/tlb: use Xen L0 assisted TLB flush when available

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jan 28, 2020 at 04:24:24PM +0000, Wei Liu wrote:
> On Tue, Jan 28, 2020 at 03:57:04PM +0100, Roger Pau Monné wrote:
> > On Tue, Jan 28, 2020 at 02:17:36PM +0000, Wei Liu wrote:
> > > On Mon, Jan 27, 2020 at 07:11:15PM +0100, Roger Pau Monne wrote:
> > > [...]
> > > >  
> > > >  const struct hypervisor_ops *__init xg_probe(void)
> > > > diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c
> > > > index 65eb7cbda8..9bc925616a 100644
> > > > --- a/xen/arch/x86/smp.c
> > > > +++ b/xen/arch/x86/smp.c
> > > > @@ -15,6 +15,7 @@
> > > >  #include <xen/perfc.h>
> > > >  #include <xen/spinlock.h>
> > > >  #include <asm/current.h>
> > > > +#include <asm/guest.h>
> > > >  #include <asm/smp.h>
> > > >  #include <asm/mc146818rtc.h>
> > > >  #include <asm/flushtlb.h>
> > > > @@ -256,6 +257,16 @@ void flush_area_mask(const cpumask_t *mask, const 
> > > > void *va, unsigned int flags)
> > > >      if ( (flags & ~FLUSH_ORDER_MASK) &&
> > > >           !cpumask_subset(mask, cpumask_of(cpu)) )
> > > >      {
> > > > +        if ( cpu_has_hypervisor &&
> > > > +             !(flags & ~(FLUSH_TLB | FLUSH_TLB_GLOBAL | FLUSH_VA_VALID 
> > > > |
> > > > +                         FLUSH_ORDER_MASK)) &&
> > > > +             !hypervisor_flush_tlb(mask, va, flags & FLUSH_ORDER_MASK) 
> > > > )
> > > > +        {
> > > > +            if ( tlb_clk_enabled )
> > > > +                tlb_clk_enabled = false;
> > > 
> > > You may delete the if here to make the generated machine code shorter.
> > 
> > Hm, but tlb_clk_enabled is marked as read_mostly, which won't be true
> > then, and would likely have a performance impact.
> 
> OK. Fair enough.
> 
> > 
> > > OOI why isn't tlb_clk_enabled set to false when Xen determines to use L0
> > > assisted flush?
> > 
> > L0 assisted flush can fail (ie: return an error), and in that case Xen
> > would be better to continue using the timestamped tlb, as it could
> > avoid some flushes.
> 
> Do you need to set tlb_clk_enabled in that case?

AFAICT it's safe to enable the TLB timestamps after being disabled,
but hypervisor_flush_tlb could fail intermittently with EBUSY in the
Xen implementation, and I don't really want that to cause spurious
enabling of the timestamps periodically.

My expectation would be that such assistance could fail sporadically,
but it shouldn't re-enable the timestamped TLB as failure would be
infrequent.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel