Re: [Xen-devel] [PATCH v3 6/7] x86/tlb: allow disabling the TLB clock

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

On Thu, Feb 06, 2020 at 01:31:34PM +0000, Wei Liu wrote:
> On Mon, Jan 27, 2020 at 07:11:14PM +0100, Roger Pau Monne wrote:
> > The TLB clock is helpful when running Xen on bare metal because when
> > doing a TLB flush each CPU is IPI'ed and can keep a timestamp of the
> > last flush.
> > 
> > This is not the case however when Xen is running virtualized, and the
> > underlying hypervisor provides mechanism to assist in performing TLB
> > flushes: Xen itself for example offers a HVMOP_flush_tlbs hypercall in
> > order to perform a TLB flush without having to IPI each CPU. When
> > using such mechanisms it's no longer possible to keep a timestamp of
> > the flushes on each CPU, as they are performed by the underlying
> > hypervisor.
> > 
> > Offer a boolean in order to signal Xen that the timestamped TLB
> > shouldn't be used. This avoids keeping the timestamps of the flushes,
> > and also forces NEED_FLUSH to always return true.
> > 
> > No functional change intended, as this change doesn't introduce any
> > user that disables the timestamped TLB.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/flushtlb.c        | 19 +++++++++++++------
> >  xen/include/asm-x86/flushtlb.h | 17 ++++++++++++++++-
> >  2 files changed, 29 insertions(+), 7 deletions(-)
> > 
> > diff --git a/xen/arch/x86/flushtlb.c b/xen/arch/x86/flushtlb.c
> > index e7ccd4ec7b..3649900793 100644
> > --- a/xen/arch/x86/flushtlb.c
> > +++ b/xen/arch/x86/flushtlb.c
> > @@ -32,6 +32,9 @@
> >  u32 tlbflush_clock = 1U;
> >  DEFINE_PER_CPU(u32, tlbflush_time);
> >  
> > +/* Signals whether the TLB flush clock is in use. */
> > +bool __read_mostly tlb_clk_enabled = true;
> > +
> >  /*
> >   * pre_flush(): Increment the virtual TLB-flush clock. Returns new clock 
> > value.
> >   * 
> > @@ -82,12 +85,13 @@ static void post_flush(u32 t)
> >  static void do_tlb_flush(void)
> >  {
> >      unsigned long flags, cr4;
> > -    u32 t;
> > +    u32 t = 0;
> >  
> >      /* This non-reentrant function is sometimes called in interrupt 
> > context. */
> >      local_irq_save(flags);
> >  
> > -    t = pre_flush();
> > +    if ( tlb_clk_enabled )
> > +        t = pre_flush();
> 
> I think it makes more sense to push the check to pre_flush and
> post_flush -- they are the ones that care about the clock, after all.
> 
> This also has the effect of making this patch a bit shorter, I think.

I added the check here in order to avoid the call just to return 0.
This is a hot path, so I thought that a check here would be less
expensive that a function call when the TLB stamps are disabled.

I don't mind moving it inside {pre/post}_flush functions if that's the
consensus.

Thanks, Roger.

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