Re: [PATCH for-4.14 v3] x86/tlb: fix assisted flush usage

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

On Fri, Jun 26, 2020 at 02:58:21PM +0100, Julien Grall wrote:
> On 26/06/2020 14:21, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Jan Beulich <jbeulich@xxxxxxxx>
> > > Sent: 26 June 2020 14:11
> > > To: Roger Pau Monné <roger.pau@xxxxxxxxxx>; paul@xxxxxxx; Andrew Cooper 
> > > <andrew.cooper3@xxxxxxxxxx>
> > > Cc: Julien Grall <julien@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; Wei 
> > > Liu <wl@xxxxxxx>; George Dunlap
> > > <george.dunlap@xxxxxxxxxx>; Ian Jackson <ian.jackson@xxxxxxxxxxxxx>; 
> > > Stefano Stabellini
> > > <sstabellini@xxxxxxxxxx>; Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> > > Subject: Re: [PATCH for-4.14 v3] x86/tlb: fix assisted flush usage
> > > 
> > > On 26.06.2020 12:07, Roger Pau Monné wrote:
> > > > On Fri, Jun 26, 2020 at 10:38:11AM +0100, Julien Grall wrote:
> > > > > Hi Roger,
> > > > > 
> > > > > Sorry I didn't manage to answer to your question before you sent v3.
> > > > > 
> > > > > On 25/06/2020 12:30, Roger Pau Monne wrote:
> > > > > > diff --git a/xen/include/asm-arm/flushtlb.h 
> > > > > > b/xen/include/asm-arm/flushtlb.h
> > > > > > index ab1aae5c90..7ae0543885 100644
> > > > > > --- a/xen/include/asm-arm/flushtlb.h
> > > > > > +++ b/xen/include/asm-arm/flushtlb.h
> > > > > > @@ -27,6 +27,7 @@ static inline void 
> > > > > > page_set_tlbflush_timestamp(struct page_info *page)
> > > > > >    /* Flush specified CPUs' TLBs */
> > > > > >    void flush_tlb_mask(const cpumask_t *mask);
> > > > > > +#define flush_tlb_mask_sync flush_tlb_mask
> > > > > 
> > > > > Dropping the parameter 'sync' from filtered_flush_tlb_mask() is a nice
> > > > > improvement, but it unfortunately doesn't fully address my concern.
> > > > > 
> > > > > After this patch there is exactly one use of flush_tlb_mask() in 
> > > > > common code
> > > > > (see grant_table.c). But without looking at the x86 code, it is not 
> > > > > clear
> > > > > why this requires a different flush compare to the two other sites.
> > > > 
> > > > It's not dealing with page allocation or page type changes directly,
> > > > and hence doesn't need to use an IPI in order to prevent races with
> > > > spurious_page_fault.
> > > > 
> > > > > IOW, if I want to modify the common code in the future, how do I know 
> > > > > which
> > > > > flush to call?
> > > > 
> > > > Unless you modify one of the specific areas mentioned above (page
> > > > allocation or page type changes) you should use flush_tlb_mask.
> > > > 
> > > > This is not ideal, and my aim will be to be able to use the assisted
> > > > flush everywhere if possible, so I would really like to get rid of the
> > > > interrupt disabling done in spurious_page_fault and this model where
> > > > x86 relies on blocking interrupts in order to prevent page type
> > > > changes or page freeing.
> > > > 
> > > > Such change however doesn't feel appropriate for a release freeze
> > > > period, and hence went with something smaller that restores the
> > > > previous behavior. Another option is to just disable assisted flushes
> > > > for the time being and re-enable them when a suitable solution is
> > > > found.
> > > 
> > > As I can understand Julien's concern, maybe this would indeed be
> > > the better approach for now? Andrew, Paul - thoughts?
> > > 
> > 
> > Julien's concern seems to be about long term usage whereas IIUC this patch 
> > does fix the issue at hand, so can we put this patch in now on the basis 
> > that Roger will do the re-work described after 4.14 (which I think will 
> > address Julien's concern)?
> Bear in mind that while this may be properly fixed in the next release, the
> hack will stay forever in Xen 4.14.
> 
> While I understand that disabling assisted flush is going to have a
> performance impact, we also need to make sure the interface make senses.
> 
> From a generic perspective, a TLB flush should have the exact same guarantee
> regardless where we call it in common/. So I would still strongly prefer if
> we have a single helper.
> 
> Is it possible to consider to replace all the flush_tlb_mask() in common
> code by arch_flush_tlb_mask()? On Arm, this would just be a rename. On x86,
> this would be an alias to flush_tlb_mask_sync()?

The TLB flush call in grant_table.c could still use a flush_tlb_mask,
but it will also work fine with a flush_tlb_mask_sync.

I can prepare a patch if that's acceptable, I guess it would be
slightly better than fully disabling assisted flush.

Roger.