* [Xen-devel] [RFC PATCH v2 1/3] x86/mm/tlb: Change __flush_tlb_one_user interface
[not found] <20190823225248.15597-1-namit@vmware.com>
@ 2019-08-23 22:52 ` Nadav Amit
2019-08-26 7:51 ` Juergen Gross
0 siblings, 1 reply; 4+ messages in thread
From: Nadav Amit @ 2019-08-23 22:52 UTC (permalink / raw)
To: Andy Lutomirski, Dave Hansen
Cc: Juergen Gross, Stefano Stabellini, Peter Zijlstra, x86,
linux-kernel, Ingo Molnar, Nadav Amit, xen-devel,
Thomas Gleixner, Boris Ostrovsky
__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/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index bc4829c9b3f9..7347328eacd3 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -57,9 +57,10 @@ static inline void __flush_tlb_global(void)
PVOP_VCALL0(mmu.flush_tlb_kernel);
}
-static inline void __flush_tlb_one_user(unsigned long addr)
+static inline void __flush_tlb_range(unsigned long start, unsigned long end,
+ u8 stride_shift)
{
- PVOP_VCALL1(mmu.flush_tlb_one_user, addr);
+ PVOP_VCALL3(mmu.flush_tlb_range, start, end, stride_shift);
}
static inline void flush_tlb_multi(const struct cpumask *cpumask,
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index 63fa751344bf..a87a5f236251 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -205,7 +205,8 @@ struct pv_mmu_ops {
/* TLB operations */
void (*flush_tlb_user)(void);
void (*flush_tlb_kernel)(void);
- void (*flush_tlb_one_user)(unsigned long addr);
+ void (*flush_tlb_range)(unsigned long start, unsigned long end,
+ u8 stride_shift);
void (*flush_tlb_multi)(const struct cpumask *cpus,
const struct flush_tlb_info *info);
diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
index 1f88ea410ff3..421bc82504e2 100644
--- a/arch/x86/include/asm/tlbflush.h
+++ b/arch/x86/include/asm/tlbflush.h
@@ -145,7 +145,7 @@ static inline unsigned long build_cr3_noflush(pgd_t *pgd, u16 asid)
#else
#define __flush_tlb() __native_flush_tlb()
#define __flush_tlb_global() __native_flush_tlb_global()
-#define __flush_tlb_one_user(addr) __native_flush_tlb_one_user(addr)
+#define __flush_tlb_range(start, end, stride_shift) __native_flush_tlb_range(start, end, stride_shift)
#endif
struct tlb_context {
@@ -454,23 +454,13 @@ static inline void __native_flush_tlb_global(void)
/*
* flush one page in the user mapping
*/
-static inline void __native_flush_tlb_one_user(unsigned long addr)
+static inline void __native_flush_tlb_range(unsigned long start,
+ unsigned long end, u8 stride_shift)
{
- u32 loaded_mm_asid = this_cpu_read(cpu_tlbstate.loaded_mm_asid);
+ unsigned long addr;
- asm volatile("invlpg (%0)" ::"r" (addr) : "memory");
-
- if (!static_cpu_has(X86_FEATURE_PTI))
- return;
-
- /*
- * Some platforms #GP if we call invpcid(type=1/2) before CR4.PCIDE=1.
- * Just use invalidate_user_asid() in case we are called early.
- */
- if (!this_cpu_has(X86_FEATURE_INVPCID_SINGLE))
- invalidate_user_asid(loaded_mm_asid);
- else
- invpcid_flush_one(user_pcid(loaded_mm_asid), addr);
+ for (addr = start; addr < end; addr += 1ul << stride_shift)
+ asm volatile("invlpg (%0)" ::"r" (addr) : "memory");
}
/*
@@ -512,7 +502,7 @@ static inline void __flush_tlb_one_kernel(unsigned long addr)
* kernel address space and for its usermode counterpart, but it does
* not flush it for other address spaces.
*/
- __flush_tlb_one_user(addr);
+ __flush_tlb_range(addr, addr + PAGE_SIZE, PAGE_SHIFT);
if (!static_cpu_has(X86_FEATURE_PTI))
return;
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index 5520a04c84ba..195f5577d0d5 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -173,9 +173,10 @@ static void native_flush_tlb_global(void)
__native_flush_tlb_global();
}
-static void native_flush_tlb_one_user(unsigned long addr)
+static void native_flush_tlb_range(unsigned long start, unsigned long end,
+ u8 stride_shift)
{
- __native_flush_tlb_one_user(addr);
+ __native_flush_tlb_range(start, end, stride_shift);
}
struct static_key paravirt_steal_enabled;
@@ -358,7 +359,7 @@ struct paravirt_patch_template pv_ops = {
/* Mmu ops. */
.mmu.flush_tlb_user = native_flush_tlb,
.mmu.flush_tlb_kernel = native_flush_tlb_global,
- .mmu.flush_tlb_one_user = native_flush_tlb_one_user,
+ .mmu.flush_tlb_range = native_flush_tlb_range,
.mmu.flush_tlb_multi = native_flush_tlb_multi,
.mmu.tlb_remove_table =
(void (*)(struct mmu_gather *, void *))tlb_remove_page,
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 0addc6e84126..ad15fc2c0790 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -512,6 +512,35 @@ void initialize_tlbstate_and_flush(void)
this_cpu_write(cpu_tlbstate.ctxs[i].ctx_id, 0);
}
+static void flush_tlb_user_pt_range(u16 asid, const struct flush_tlb_info *f)
+{
+ unsigned long start, end, addr;
+ u8 stride_shift;
+
+ if (!static_cpu_has(X86_FEATURE_PTI))
+ return;
+
+ /*
+ * Read the data into variable on the stack to prevent it from being
+ * reevaluated due to invpcid memory clobber.
+ */
+ start = f->start;
+ end = f->end;
+ stride_shift = f->stride_shift;
+
+ /*
+ * Some platforms #GP if we call invpcid(type=1/2) before CR4.PCIDE=1.
+ * Just use invalidate_user_asid() in case we are called early.
+ */
+ if (!this_cpu_has(X86_FEATURE_INVPCID_SINGLE)) {
+ invalidate_user_asid(asid);
+ return;
+ }
+
+ for (addr = start; addr < end; addr += 1ul << stride_shift)
+ invpcid_flush_one(user_pcid(asid), addr);
+}
+
/*
* flush_tlb_func()'s memory ordering requirement is that any
* TLB fills that happen after we flush the TLB are ordered after we
@@ -624,14 +653,12 @@ static void flush_tlb_func(void *info)
f->new_tlb_gen == local_tlb_gen + 1 &&
f->new_tlb_gen == mm_tlb_gen) {
/* Partial flush */
- unsigned long addr = f->start;
-
nr_invalidate = (f->end - f->start) >> f->stride_shift;
- while (addr < f->end) {
- __flush_tlb_one_user(addr);
- addr += 1UL << f->stride_shift;
- }
+ __flush_tlb_range(f->start, f->end, f->stride_shift);
+
+ flush_tlb_user_pt_range(loaded_mm_asid, f);
+
if (local)
count_vm_tlb_events(NR_TLB_LOCAL_FLUSH_ONE, nr_invalidate);
} else {
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);
+ }
preempt_enable();
}
@@ -2394,7 +2399,7 @@ static const struct pv_mmu_ops xen_mmu_ops __initconst = {
.flush_tlb_user = xen_flush_tlb,
.flush_tlb_kernel = xen_flush_tlb,
- .flush_tlb_one_user = xen_flush_tlb_one_user,
+ .flush_tlb_range = xen_flush_tlb_range,
.flush_tlb_multi = xen_flush_tlb_multi,
.tlb_remove_table = tlb_remove_table,
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Xen-devel] [RFC PATCH v2 1/3] x86/mm/tlb: Change __flush_tlb_one_user interface
2019-08-23 22:52 ` [Xen-devel] [RFC PATCH v2 1/3] x86/mm/tlb: Change __flush_tlb_one_user interface Nadav Amit
@ 2019-08-26 7:51 ` Juergen Gross
2019-08-26 16:38 ` Nadav Amit
0 siblings, 1 reply; 4+ messages in thread
From: Juergen Gross @ 2019-08-26 7:51 UTC (permalink / raw)
To: Nadav Amit, Andy Lutomirski, Dave Hansen
Cc: Stefano Stabellini, Peter Zijlstra, x86, linux-kernel,
Ingo Molnar, xen-devel, Thomas Gleixner, Boris Ostrovsky
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Xen-devel] [RFC PATCH v2 1/3] x86/mm/tlb: Change __flush_tlb_one_user interface
2019-08-26 7:51 ` Juergen Gross
@ 2019-08-26 16:38 ` Nadav Amit
2019-08-27 5:53 ` Juergen Gross
0 siblings, 1 reply; 4+ messages in thread
From: Nadav Amit @ 2019-08-26 16:38 UTC (permalink / raw)
To: Juergen Gross
Cc: Dave Hansen, Peter Zijlstra, the arch/x86 maintainers,
linux-kernel, Stefano Stabellini, Andy Lutomirski, xen-devel,
Thomas Gleixner, Ingo Molnar, Boris Ostrovsky
> On Aug 26, 2019, at 12:51 AM, Juergen Gross <jgross@suse.com> wrote:
>
> 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.
Thanks for the quick response. I don’t think the preempt_disable/enable() is
needed in this case, but I kept them. Does the following match what you had
in mind?
---
arch/x86/xen/mmu_pv.c | 25 ++++++++++++++-----------
include/trace/events/xen.h | 18 ++++++++++++------
2 files changed, 26 insertions(+), 17 deletions(-)
diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
index 48f7c7eb4dbc..faa01591df36 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -1325,20 +1325,23 @@ 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);
+ struct mmuext_op op;
+ 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_batch();
+ op.cmd = MMUEXT_INVLPG_LOCAL;
+
+ trace_xen_mmu_flush_tlb_range(start, end, stride_shift);
+
+ for (addr = start; addr < end; addr += 1ul << stride_shift) {
+ op.arg1.linear_addr = addr & PAGE_MASK;
+ xen_extend_mmuext_op(&op);
+ }
xen_mc_issue(PARAVIRT_LAZY_MMU);
@@ -2394,7 +2397,7 @@ static const struct pv_mmu_ops xen_mmu_ops __initconst = {
.flush_tlb_user = xen_flush_tlb,
.flush_tlb_kernel = xen_flush_tlb,
- .flush_tlb_one_user = xen_flush_tlb_one_user,
+ .flush_tlb_range = xen_flush_tlb_range,
.flush_tlb_multi = xen_flush_tlb_multi,
.tlb_remove_table = tlb_remove_table,
diff --git a/include/trace/events/xen.h b/include/trace/events/xen.h
index 546022acf160..32bf0a94b4d8 100644
--- a/include/trace/events/xen.h
+++ b/include/trace/events/xen.h
@@ -352,14 +352,20 @@ DECLARE_EVENT_CLASS(xen_mmu_pgd,
DEFINE_XEN_MMU_PGD_EVENT(xen_mmu_pgd_pin);
DEFINE_XEN_MMU_PGD_EVENT(xen_mmu_pgd_unpin);
-TRACE_EVENT(xen_mmu_flush_tlb_one_user,
- TP_PROTO(unsigned long addr),
- TP_ARGS(addr),
+TRACE_EVENT(xen_mmu_flush_tlb_range,
+ TP_PROTO(unsigned long start, unsigned long end,
+ unsigned char stride_shift),
+ TP_ARGS(start, end, stride_shift),
TP_STRUCT__entry(
- __field(unsigned long, addr)
+ __field(unsigned long, start)
+ __field(unsigned long, end)
+ __field(unsigned char, stride_shift)
),
- TP_fast_assign(__entry->addr = addr),
- TP_printk("addr %lx", __entry->addr)
+ TP_fast_assign(__entry->start = start;
+ __entry->end = end;
+ __entry->stride_shift = stride_shift),
+ TP_printk("start %lx end %lx stride_shift %d", __entry->start,
+ __entry->end, __entry->stride_shift)
);
TRACE_EVENT(xen_mmu_flush_tlb_multi,
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Xen-devel] [RFC PATCH v2 1/3] x86/mm/tlb: Change __flush_tlb_one_user interface
2019-08-26 16:38 ` Nadav Amit
@ 2019-08-27 5:53 ` Juergen Gross
0 siblings, 0 replies; 4+ messages in thread
From: Juergen Gross @ 2019-08-27 5:53 UTC (permalink / raw)
To: Nadav Amit
Cc: StefanoStabellini, Peter Zijlstra, Dave Hansen,
the arch/x86 maintainers, linux-kernel, Ingo Molnar,
Andy Lutomirski, xen-devel, Thomas Gleixner, Boris Ostrovsky
On 26.08.19 18:38, Nadav Amit wrote:
>> On Aug 26, 2019, at 12:51 AM, Juergen Gross <jgross@suse.com> wrote:
>>
>> 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.
>
> Thanks for the quick response. I don’t think the preempt_disable/enable() is
> needed in this case, but I kept them. Does the following match what you had
> in mind?
Yes, it does.
BTW, preempt_disable/enable() is needed as xen_mc_batch() ...
xen_mc_issue() is using a percpu buffer.
Juergen
>
> ---
> arch/x86/xen/mmu_pv.c | 25 ++++++++++++++-----------
> include/trace/events/xen.h | 18 ++++++++++++------
> 2 files changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
> index 48f7c7eb4dbc..faa01591df36 100644
> --- a/arch/x86/xen/mmu_pv.c
> +++ b/arch/x86/xen/mmu_pv.c
> @@ -1325,20 +1325,23 @@ 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);
> + struct mmuext_op op;
> + 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_batch();
> + op.cmd = MMUEXT_INVLPG_LOCAL;
> +
> + trace_xen_mmu_flush_tlb_range(start, end, stride_shift);
> +
> + for (addr = start; addr < end; addr += 1ul << stride_shift) {
> + op.arg1.linear_addr = addr & PAGE_MASK;
> + xen_extend_mmuext_op(&op);
> + }
>
> xen_mc_issue(PARAVIRT_LAZY_MMU);
>
> @@ -2394,7 +2397,7 @@ static const struct pv_mmu_ops xen_mmu_ops __initconst = {
>
> .flush_tlb_user = xen_flush_tlb,
> .flush_tlb_kernel = xen_flush_tlb,
> - .flush_tlb_one_user = xen_flush_tlb_one_user,
> + .flush_tlb_range = xen_flush_tlb_range,
> .flush_tlb_multi = xen_flush_tlb_multi,
> .tlb_remove_table = tlb_remove_table,
>
> diff --git a/include/trace/events/xen.h b/include/trace/events/xen.h
> index 546022acf160..32bf0a94b4d8 100644
> --- a/include/trace/events/xen.h
> +++ b/include/trace/events/xen.h
> @@ -352,14 +352,20 @@ DECLARE_EVENT_CLASS(xen_mmu_pgd,
> DEFINE_XEN_MMU_PGD_EVENT(xen_mmu_pgd_pin);
> DEFINE_XEN_MMU_PGD_EVENT(xen_mmu_pgd_unpin);
>
> -TRACE_EVENT(xen_mmu_flush_tlb_one_user,
> - TP_PROTO(unsigned long addr),
> - TP_ARGS(addr),
> +TRACE_EVENT(xen_mmu_flush_tlb_range,
> + TP_PROTO(unsigned long start, unsigned long end,
> + unsigned char stride_shift),
> + TP_ARGS(start, end, stride_shift),
> TP_STRUCT__entry(
> - __field(unsigned long, addr)
> + __field(unsigned long, start)
> + __field(unsigned long, end)
> + __field(unsigned char, stride_shift)
> ),
> - TP_fast_assign(__entry->addr = addr),
> - TP_printk("addr %lx", __entry->addr)
> + TP_fast_assign(__entry->start = start;
> + __entry->end = end;
> + __entry->stride_shift = stride_shift),
> + TP_printk("start %lx end %lx stride_shift %d", __entry->start,
> + __entry->end, __entry->stride_shift)
> );
>
> TRACE_EVENT(xen_mmu_flush_tlb_multi,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-08-27 5:54 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20190823225248.15597-1-namit@vmware.com>
2019-08-23 22:52 ` [Xen-devel] [RFC PATCH v2 1/3] x86/mm/tlb: Change __flush_tlb_one_user interface Nadav Amit
2019-08-26 7:51 ` Juergen Gross
2019-08-26 16:38 ` Nadav Amit
2019-08-27 5:53 ` Juergen Gross
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).