Messages in this thread | | | Subject | Re: [RFC PATCH v2 1/3] x86/mm/tlb: Change __flush_tlb_one_user interface | From | Juergen Gross <> | Date | Mon, 26 Aug 2019 09:51:19 +0200 |
| |
On 24.08.19 00:52, Nadav Amit wrote: > __flush_tlb_one_user() currently flushes a single entry, and flushes it > both in the kernel and user page-tables, when PTI is enabled. > > Change __flush_tlb_one_user() and related interfaces into > __flush_tlb_range() that flushes a range and does not flush the user > page-table. > > This refactoring is needed for the next patch, but regardless makes > sense and has several advantages. First, only Xen-PV, which does not > use PTI, implements the paravirtual interface of flush_tlb_one_user() so > nothing is broken by separating the user and kernel page-table flushes, > and the interface is more intuitive. > > Second, INVLPG can flush unrelated mappings, and it is also a > serializing instruction. It is better to have a tight loop that flushes > the entries. > > Third, currently __flush_tlb_one_kernel() also flushes the user > page-tables, which is not needed. This allows to avoid this redundant > flush. > > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: Juergen Gross <jgross@suse.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: xen-devel@lists.xenproject.org > Signed-off-by: Nadav Amit <namit@vmware.com> > --- > arch/x86/include/asm/paravirt.h | 5 ++-- > arch/x86/include/asm/paravirt_types.h | 3 ++- > arch/x86/include/asm/tlbflush.h | 24 +++++------------ > arch/x86/kernel/paravirt.c | 7 ++--- > arch/x86/mm/tlb.c | 39 ++++++++++++++++++++++----- > arch/x86/xen/mmu_pv.c | 21 +++++++++------ > 6 files changed, 62 insertions(+), 37 deletions(-)
...
> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c > index 48f7c7eb4dbc..ed68657f5e77 100644 > --- a/arch/x86/xen/mmu_pv.c > +++ b/arch/x86/xen/mmu_pv.c > @@ -1325,22 +1325,27 @@ static noinline void xen_flush_tlb(void) > preempt_enable(); > } > > -static void xen_flush_tlb_one_user(unsigned long addr) > +static void xen_flush_tlb_range(unsigned long start, unsigned long end, > + u8 stride_shift) > { > struct mmuext_op *op; > struct multicall_space mcs; > - > - trace_xen_mmu_flush_tlb_one_user(addr); > + unsigned long addr; > > preempt_disable(); > > mcs = xen_mc_entry(sizeof(*op)); > op = mcs.args; > - op->cmd = MMUEXT_INVLPG_LOCAL; > - op->arg1.linear_addr = addr & PAGE_MASK; > - MULTI_mmuext_op(mcs.mc, op, 1, NULL, DOMID_SELF); > > - xen_mc_issue(PARAVIRT_LAZY_MMU); > + for (addr = start; addr < end; addr += 1ul << stride_shift) { > + trace_xen_mmu_flush_tlb_one_user(addr); > + > + op->cmd = MMUEXT_INVLPG_LOCAL; > + op->arg1.linear_addr = addr & PAGE_MASK; > + MULTI_mmuext_op(mcs.mc, op, 1, NULL, DOMID_SELF); > + > + xen_mc_issue(PARAVIRT_LAZY_MMU); > + }
For this kind of usage (a loop) you should:
- replace the call of xen_mc_entry() with xen_mc_batch() - use xen_extend_mmuext_op() for each loop iteration - call xen_mc_issue() after the loop
Additionally I'd like you to replace trace_xen_mmu_flush_tlb_one_user() with trace_xen_mmu_flush_tlb_range() taking all three parameters and keep it where it was (out of the loop).
The paravirt parts seem to be okay.
Juergen
| |