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....@citrix.com>
> > ---
> > 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
