* [PATCH v4 09/10] powerpc64/ftrace: Use the generic version of ftrace_replace_code()
From: Naveen N. Rao @ 2018-04-03 20:12 UTC (permalink / raw)
To: Michael Ellerman, Steven Rostedt
Cc: linuxppc-dev, Paul Mackerras, Nicholas Piggin, Anton Blanchard,
sathnaga
In-Reply-To: <cover.1522784883.git.naveen.n.rao@linux.vnet.ibm.com>
Our implementation matches that of the generic version, which also
handles FTRACE_UPDATE_MODIFY_CALL. So, remove our implementation in
favor of the generic version.
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/kernel/trace/ftrace.c | 36 ------------------------------------
1 file changed, 36 deletions(-)
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 4741fe112f05..80667128db3d 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -485,42 +485,6 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
return ret;
}
-static int __ftrace_replace_code(struct dyn_ftrace *rec, int enable)
-{
- unsigned long ftrace_addr = (unsigned long)FTRACE_ADDR;
- int ret;
-
- ret = ftrace_update_record(rec, enable);
-
- switch (ret) {
- case FTRACE_UPDATE_IGNORE:
- return 0;
- case FTRACE_UPDATE_MAKE_CALL:
- return ftrace_make_call(rec, ftrace_addr);
- case FTRACE_UPDATE_MAKE_NOP:
- return ftrace_make_nop(NULL, rec, ftrace_addr);
- }
-
- return 0;
-}
-
-void ftrace_replace_code(int enable)
-{
- struct ftrace_rec_iter *iter;
- struct dyn_ftrace *rec;
- int ret;
-
- for (iter = ftrace_rec_iter_start(); iter;
- iter = ftrace_rec_iter_next(iter)) {
- rec = ftrace_rec_iter_record(iter);
- ret = __ftrace_replace_code(rec, enable);
- if (ret) {
- ftrace_bug(ret, rec);
- return;
- }
- }
-}
-
/*
* Use the default ftrace_modify_all_code, but without
* stop_machine().
--
2.16.2
^ permalink raw reply related
* [PATCH v4 10/10] powerpc64/ftrace: Implement support for ftrace_regs_caller()
From: Naveen N. Rao @ 2018-04-03 20:12 UTC (permalink / raw)
To: Michael Ellerman, Steven Rostedt
Cc: linuxppc-dev, Paul Mackerras, Nicholas Piggin, Anton Blanchard,
sathnaga
In-Reply-To: <cover.1522784883.git.naveen.n.rao@linux.vnet.ibm.com>
With -mprofile-kernel, we always save the full register state in
ftrace_caller(). While this works, this is inefficient if we're not
interested in the register state, such as when we're using the function
tracer.
Rename the existing ftrace_caller() as ftrace_regs_caller() and provide
a simpler implementation for ftrace_caller() that is used when registers
are not required to be saved.
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/ftrace.h | 2 -
arch/powerpc/include/asm/module.h | 3 +
arch/powerpc/kernel/module_64.c | 28 +++-
arch/powerpc/kernel/trace/ftrace.c | 184 +++++++++++++++++++++++--
arch/powerpc/kernel/trace/ftrace_64_mprofile.S | 71 +++++++++-
5 files changed, 262 insertions(+), 26 deletions(-)
diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index 3b5e85a72e10..f0806a2fd451 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -49,8 +49,6 @@
extern void _mcount(void);
#ifdef CONFIG_DYNAMIC_FTRACE
-# define FTRACE_ADDR ((unsigned long)ftrace_caller)
-# define FTRACE_REGS_ADDR FTRACE_ADDR
static inline unsigned long ftrace_call_adjust(unsigned long addr)
{
/* reloction of mcount call site is the same as the address */
diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
index 7e28442827f1..2d16b6d9147d 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -43,6 +43,9 @@ struct mod_arch_specific {
#ifdef CONFIG_DYNAMIC_FTRACE
unsigned long toc;
unsigned long tramp;
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+ unsigned long tramp_regs;
+#endif
#endif
/* For module function descriptor dereference */
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 8413be31d6a4..f7667e2ebfcb 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -280,6 +280,10 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
#ifdef CONFIG_DYNAMIC_FTRACE
/* make the trampoline to the ftrace_caller */
relocs++;
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+ /* an additional one for ftrace_regs_caller */
+ relocs++;
+#endif
#endif
pr_debug("Looks like a total of %lu stubs, max\n", relocs);
@@ -765,7 +769,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
* via the paca (in r13). The target (ftrace_caller()) is responsible for
* saving and restoring the toc before returning.
*/
-static unsigned long create_ftrace_stub(const Elf64_Shdr *sechdrs, struct module *me)
+static unsigned long create_ftrace_stub(const Elf64_Shdr *sechdrs,
+ struct module *me, unsigned long addr)
{
struct ppc64_stub_entry *entry;
unsigned int i, num_stubs;
@@ -792,9 +797,10 @@ static unsigned long create_ftrace_stub(const Elf64_Shdr *sechdrs, struct module
memcpy(entry->jump, stub_insns, sizeof(stub_insns));
/* Stub uses address relative to kernel toc (from the paca) */
- reladdr = (unsigned long)ftrace_caller - kernel_toc_addr();
+ reladdr = addr - kernel_toc_addr();
if (reladdr > 0x7FFFFFFF || reladdr < -(0x80000000L)) {
- pr_err("%s: Address of ftrace_caller out of range of kernel_toc.\n", me->name);
+ pr_err("%s: Address of %ps out of range of kernel_toc.\n",
+ me->name, (void *)addr);
return 0;
}
@@ -802,22 +808,30 @@ static unsigned long create_ftrace_stub(const Elf64_Shdr *sechdrs, struct module
entry->jump[2] |= PPC_LO(reladdr);
/* Eventhough we don't use funcdata in the stub, it's needed elsewhere. */
- entry->funcdata = func_desc((unsigned long)ftrace_caller);
+ entry->funcdata = func_desc(addr);
entry->magic = STUB_MAGIC;
return (unsigned long)entry;
}
#else
-static unsigned long create_ftrace_stub(const Elf64_Shdr *sechdrs, struct module *me)
+static unsigned long create_ftrace_stub(const Elf64_Shdr *sechdrs,
+ struct module *me, unsigned long addr)
{
- return stub_for_addr(sechdrs, (unsigned long)ftrace_caller, me);
+ return stub_for_addr(sechdrs, addr, me);
}
#endif
int module_finalize_ftrace(struct module *mod, const Elf_Shdr *sechdrs)
{
mod->arch.toc = my_r2(sechdrs, mod);
- mod->arch.tramp = create_ftrace_stub(sechdrs, mod);
+ mod->arch.tramp = create_ftrace_stub(sechdrs, mod,
+ (unsigned long)ftrace_caller);
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+ mod->arch.tramp_regs = create_ftrace_stub(sechdrs, mod,
+ (unsigned long)ftrace_regs_caller);
+ if (!mod->arch.tramp_regs)
+ return -ENOENT;
+#endif
if (!mod->arch.tramp)
return -ENOENT;
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 80667128db3d..79d2924e75d5 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -357,6 +357,8 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
{
unsigned int op[2];
void *ip = (void *)rec->ip;
+ unsigned long entry, ptr, tramp;
+ struct module *mod = rec->arch.mod;
/* read where this goes */
if (probe_kernel_read(op, ip, sizeof(op)))
@@ -368,19 +370,44 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
return -EINVAL;
}
- /* If we never set up a trampoline to ftrace_caller, then bail */
- if (!rec->arch.mod->arch.tramp) {
+ /* If we never set up ftrace trampoline(s), then bail */
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+ if (!mod->arch.tramp || !mod->arch.tramp_regs) {
+#else
+ if (!mod->arch.tramp) {
+#endif
pr_err("No ftrace trampoline\n");
return -EINVAL;
}
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+ if (rec->flags & FTRACE_FL_REGS)
+ tramp = mod->arch.tramp_regs;
+ else
+#endif
+ tramp = mod->arch.tramp;
+
+ if (module_trampoline_target(mod, tramp, &ptr)) {
+ pr_err("Failed to get trampoline target\n");
+ return -EFAULT;
+ }
+
+ pr_devel("trampoline target %lx", ptr);
+
+ entry = ppc_global_function_entry((void *)addr);
+ /* This should match what was called */
+ if (ptr != entry) {
+ pr_err("addr %lx does not match expected %lx\n", ptr, entry);
+ return -EINVAL;
+ }
+
/* Ensure branch is within 24 bits */
- if (!create_branch(ip, rec->arch.mod->arch.tramp, BRANCH_SET_LINK)) {
+ if (!create_branch(ip, tramp, BRANCH_SET_LINK)) {
pr_err("Branch out of range\n");
return -EINVAL;
}
- if (patch_branch(ip, rec->arch.mod->arch.tramp, BRANCH_SET_LINK)) {
+ if (patch_branch(ip, tramp, BRANCH_SET_LINK)) {
pr_err("REL24 out of range!\n");
return -EINVAL;
}
@@ -388,14 +415,6 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
return 0;
}
-#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
-int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
- unsigned long addr)
-{
- return ftrace_make_call(rec, addr);
-}
-#endif
-
#else /* !CONFIG_PPC64: */
static int
__ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
@@ -472,6 +491,137 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
#endif /* CONFIG_MODULES */
}
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#ifdef CONFIG_MODULES
+static int
+__ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
+ unsigned long addr)
+{
+ unsigned int op;
+ unsigned long ip = rec->ip;
+ unsigned long entry, ptr, tramp;
+ struct module *mod = rec->arch.mod;
+
+ /* If we never set up ftrace trampolines, then bail */
+ if (!mod->arch.tramp || !mod->arch.tramp_regs) {
+ pr_err("No ftrace trampoline\n");
+ return -EINVAL;
+ }
+
+ /* read where this goes */
+ if (probe_kernel_read(&op, (void *)ip, sizeof(int))) {
+ pr_err("Fetching opcode failed.\n");
+ return -EFAULT;
+ }
+
+ /* Make sure that that this is still a 24bit jump */
+ if (!is_bl_op(op)) {
+ pr_err("Not expected bl: opcode is %x\n", op);
+ return -EINVAL;
+ }
+
+ /* lets find where the pointer goes */
+ tramp = find_bl_target(ip, op);
+ entry = ppc_global_function_entry((void *)old_addr);
+
+ pr_devel("ip:%lx jumps to %lx", ip, tramp);
+
+ if (tramp != entry) {
+ /* old_addr is not within range, so we must have used a trampoline */
+ if (module_trampoline_target(mod, tramp, &ptr)) {
+ pr_err("Failed to get trampoline target\n");
+ return -EFAULT;
+ }
+
+ pr_devel("trampoline target %lx", ptr);
+
+ /* This should match what was called */
+ if (ptr != entry) {
+ pr_err("addr %lx does not match expected %lx\n", ptr, entry);
+ return -EINVAL;
+ }
+ }
+
+ /* The new target may be within range */
+ if (test_24bit_addr(ip, addr)) {
+ /* within range */
+ if (patch_branch((unsigned int *)ip, addr, BRANCH_SET_LINK)) {
+ pr_err("REL24 out of range!\n");
+ return -EINVAL;
+ }
+
+ return 0;
+ }
+
+ if (rec->flags & FTRACE_FL_REGS)
+ tramp = mod->arch.tramp_regs;
+ else
+ tramp = mod->arch.tramp;
+
+ if (module_trampoline_target(mod, tramp, &ptr)) {
+ pr_err("Failed to get trampoline target\n");
+ return -EFAULT;
+ }
+
+ pr_devel("trampoline target %lx", ptr);
+
+ entry = ppc_global_function_entry((void *)addr);
+ /* This should match what was called */
+ if (ptr != entry) {
+ pr_err("addr %lx does not match expected %lx\n", ptr, entry);
+ return -EINVAL;
+ }
+
+ /* Ensure branch is within 24 bits */
+ if (!create_branch((unsigned int *)ip, tramp, BRANCH_SET_LINK)) {
+ pr_err("Branch out of range\n");
+ return -EINVAL;
+ }
+
+ if (patch_branch((unsigned int *)ip, tramp, BRANCH_SET_LINK)) {
+ pr_err("REL24 out of range!\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+#endif
+
+int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
+ unsigned long addr)
+{
+ unsigned long ip = rec->ip;
+ unsigned int old, new;
+
+ /*
+ * If the calling address is more that 24 bits away,
+ * then we had to use a trampoline to make the call.
+ * Otherwise just update the call site.
+ */
+ if (test_24bit_addr(ip, addr) && test_24bit_addr(ip, old_addr)) {
+ /* within range */
+ old = ftrace_call_replace(ip, old_addr, 1);
+ new = ftrace_call_replace(ip, addr, 1);
+ return ftrace_modify_code(ip, old, new);
+ }
+
+#ifdef CONFIG_MODULES
+ /*
+ * Out of range jumps are called from modules.
+ */
+ if (!rec->arch.mod) {
+ pr_err("No module loaded\n");
+ return -EINVAL;
+ }
+
+ return __ftrace_modify_call(rec, old_addr, addr);
+#else
+ /* We should not get here without modules */
+ return -EINVAL;
+#endif /* CONFIG_MODULES */
+}
+#endif
+
int ftrace_update_ftrace_func(ftrace_func_t func)
{
unsigned long ip = (unsigned long)(&ftrace_call);
@@ -482,6 +632,16 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
new = ftrace_call_replace(ip, (unsigned long)func, 1);
ret = ftrace_modify_code(ip, old, new);
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+ /* Also update the regs callback function */
+ if (!ret) {
+ ip = (unsigned long)(&ftrace_regs_call);
+ old = *(unsigned int *)&ftrace_regs_call;
+ new = ftrace_call_replace(ip, (unsigned long)func, 1);
+ ret = ftrace_modify_code(ip, old, new);
+ }
+#endif
+
return ret;
}
diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
index ae1cbe783ab6..ed9d7a46c3af 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
+++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
@@ -20,8 +20,8 @@
#ifdef CONFIG_DYNAMIC_FTRACE
/*
*
- * ftrace_caller() is the function that replaces _mcount() when ftrace is
- * active.
+ * ftrace_caller()/ftrace_regs_caller() is the function that replaces _mcount()
+ * when ftrace is active.
*
* We arrive here after a function A calls function B, and we are the trace
* function for B. When we enter r1 points to A's stack frame, B has not yet
@@ -37,7 +37,7 @@
* Our job is to save the register state into a struct pt_regs (on the stack)
* and then arrange for the ftrace function to be called.
*/
-_GLOBAL(ftrace_caller)
+_GLOBAL(ftrace_regs_caller)
/* Save the original return address in A's stack frame */
std r0,LRSAVE(r1)
@@ -100,8 +100,8 @@ _GLOBAL(ftrace_caller)
addi r6, r1 ,STACK_FRAME_OVERHEAD
/* ftrace_call(r3, r4, r5, r6) */
-.globl ftrace_call
-ftrace_call:
+.globl ftrace_regs_call
+ftrace_regs_call:
bl ftrace_stub
nop
@@ -162,6 +162,7 @@ ftrace_call:
bne- livepatch_handler
#endif
+ftrace_caller_common:
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
.globl ftrace_graph_call
ftrace_graph_call:
@@ -182,6 +183,66 @@ ftrace_no_trace:
mtlr r0
bctr
+_GLOBAL(ftrace_caller)
+ /* Save the original return address in A's stack frame */
+ std r0, LRSAVE(r1)
+
+ /* Create our stack frame + pt_regs */
+ stdu r1, -SWITCH_FRAME_SIZE(r1)
+
+ /* Save all gprs to pt_regs */
+ SAVE_8GPRS(3, r1)
+
+ lbz r3, PACA_FTRACE_ENABLED(r13)
+ cmpdi r3, 0
+ beq ftrace_no_trace
+
+ /* Get the _mcount() call site out of LR */
+ mflr r7
+ std r7, _NIP(r1)
+
+ /* Save callee's TOC in the ABI compliant location */
+ std r2, 24(r1)
+ ld r2, PACATOC(r13) /* get kernel TOC in r2 */
+
+ addis r3, r2, function_trace_op@toc@ha
+ addi r3, r3, function_trace_op@toc@l
+ ld r5, 0(r3)
+
+ /* Calculate ip from nip-4 into r3 for call below */
+ subi r3, r7, MCOUNT_INSN_SIZE
+
+ /* Put the original return address in r4 as parent_ip */
+ mr r4, r0
+
+ /* Set pt_regs to NULL */
+ li r6, 0
+
+ /* ftrace_call(r3, r4, r5, r6) */
+.globl ftrace_call
+ftrace_call:
+ bl ftrace_stub
+ nop
+
+ ld r3, _NIP(r1)
+ mtctr r3
+
+ /* Restore gprs */
+ REST_8GPRS(3,r1)
+
+ /* Restore callee's TOC */
+ ld r2, 24(r1)
+
+ /* Pop our stack frame */
+ addi r1, r1, SWITCH_FRAME_SIZE
+
+ /* Reload original LR */
+ ld r0, LRSAVE(r1)
+ mtlr r0
+
+ /* Handle function_graph or go back */
+ b ftrace_caller_common
+
#ifdef CONFIG_LIVEPATCH
/*
* This function runs in the mcount context, between two functions. As
--
2.16.2
^ permalink raw reply related
* Re: [PATCH v9 00/24] Speculative page faults
From: Jerome Glisse @ 2018-04-03 20:37 UTC (permalink / raw)
To: Laurent Dufour
Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner, Ingo Molnar,
hpa, Will Deacon, Sergey Senozhatsky, Andrea Arcangeli,
Alexei Starovoitov, kemi.wang, sergey.senozhatsky.work,
Daniel Jordan, linux-kernel, linux-mm, haren, khandual, npiggin,
bsingharora, Tim Chen, linuxppc-dev, x86
In-Reply-To: <1520963994-28477-1-git-send-email-ldufour@linux.vnet.ibm.com>
On Tue, Mar 13, 2018 at 06:59:30PM +0100, Laurent Dufour wrote:
> This is a port on kernel 4.16 of the work done by Peter Zijlstra to
> handle page fault without holding the mm semaphore [1].
>
> The idea is to try to handle user space page faults without holding the
> mmap_sem. This should allow better concurrency for massively threaded
> process since the page fault handler will not wait for other threads memory
> layout change to be done, assuming that this change is done in another part
> of the process's memory space. This type page fault is named speculative
> page fault. If the speculative page fault fails because of a concurrency is
> detected or because underlying PMD or PTE tables are not yet allocating, it
> is failing its processing and a classic page fault is then tried.
>
> The speculative page fault (SPF) has to look for the VMA matching the fault
> address without holding the mmap_sem, this is done by introducing a rwlock
> which protects the access to the mm_rb tree. Previously this was done using
> SRCU but it was introducing a lot of scheduling to process the VMA's
> freeing
> operation which was hitting the performance by 20% as reported by Kemi Wang
> [2].Using a rwlock to protect access to the mm_rb tree is limiting the
> locking contention to these operations which are expected to be in a O(log
> n)
> order. In addition to ensure that the VMA is not freed in our back a
> reference count is added and 2 services (get_vma() and put_vma()) are
> introduced to handle the reference count. When a VMA is fetch from the RB
> tree using get_vma() is must be later freeed using put_vma(). Furthermore,
> to allow the VMA to be used again by the classic page fault handler a
> service is introduced can_reuse_spf_vma(). This service is expected to be
> called with the mmap_sem hold. It checked that the VMA is still matching
> the specified address and is releasing its reference count as the mmap_sem
> is hold it is ensure that it will not be freed in our back. In general, the
> VMA's reference count could be decremented when holding the mmap_sem but it
> should not be increased as holding the mmap_sem is ensuring that the VMA is
> stable. I can't see anymore the overhead I got while will-it-scale
> benchmark anymore.
>
> The VMA's attributes checked during the speculative page fault processing
> have to be protected against parallel changes. This is done by using a per
> VMA sequence lock. This sequence lock allows the speculative page fault
> handler to fast check for parallel changes in progress and to abort the
> speculative page fault in that case.
>
> Once the VMA is found, the speculative page fault handler would check for
> the VMA's attributes to verify that the page fault has to be handled
> correctly or not. Thus the VMA is protected through a sequence lock which
> allows fast detection of concurrent VMA changes. If such a change is
> detected, the speculative page fault is aborted and a *classic* page fault
> is tried. VMA sequence lockings are added when VMA attributes which are
> checked during the page fault are modified.
>
> When the PTE is fetched, the VMA is checked to see if it has been changed,
> so once the page table is locked, the VMA is valid, so any other changes
> leading to touching this PTE will need to lock the page table, so no
> parallel change is possible at this time.
What would have been nice is some pseudo highlevel code before all the
above detailed description. Something like:
speculative_fault(addr) {
mm_lock_for_vma_snapshot()
vma_snapshot = snapshot_vma_infos(addr)
mm_unlock_for_vma_snapshot()
...
if (!vma_can_speculatively_fault(vma_snapshot, addr))
return;
...
/* Do fault ie alloc memory, read from file ... */
page = ...;
preempt_disable();
if (vma_snapshot_still_valid(vma_snapshot, addr) &&
vma_pte_map_lock(vma_snapshot, addr)) {
if (pte_same(ptep, orig_pte)) {
/* Setup new pte */
page = NULL;
}
}
preempt_enable();
if (page)
put(page)
}
I just find pseudo code easier for grasping the highlevel view of the
expected code flow.
>
> The locking of the PTE is done with interrupts disabled, this allows to
> check for the PMD to ensure that there is not an ongoing collapsing
> operation. Since khugepaged is firstly set the PMD to pmd_none and then is
> waiting for the other CPU to have catch the IPI interrupt, if the pmd is
> valid at the time the PTE is locked, we have the guarantee that the
> collapsing opertion will have to wait on the PTE lock to move foward. This
> allows the SPF handler to map the PTE safely. If the PMD value is different
> than the one recorded at the beginning of the SPF operation, the classic
> page fault handler will be called to handle the operation while holding the
> mmap_sem. As the PTE lock is done with the interrupts disabled, the lock is
> done using spin_trylock() to avoid dead lock when handling a page fault
> while a TLB invalidate is requested by an other CPU holding the PTE.
>
> Support for THP is not done because when checking for the PMD, we can be
> confused by an in progress collapsing operation done by khugepaged. The
> issue is that pmd_none() could be true either if the PMD is not already
> populated or if the underlying PTE are in the way to be collapsed. So we
> cannot safely allocate a PMD if pmd_none() is true.
Might be a good topic fo LSF/MM, should we set the pmd to something
else then 0 when collapsing pmd (apply to pud too) ? This would allow
support THP.
[...]
>
> Ebizzy:
> -------
> The test is counting the number of records per second it can manage, the
> higher is the best. I run it like this 'ebizzy -mTRp'. To get consistent
> result I repeated the test 100 times and measure the average result. The
> number is the record processes per second, the higher is the best.
>
> BASE SPF delta
> 16 CPUs x86 VM 14902.6 95905.16 543.55%
> 80 CPUs P8 node 37240.24 78185.67 109.95%
I find those results interesting as it seems that SPF do not scale well
on big configuration. Note that it still have a sizeable improvement so
it is still a very interesting feature i believe.
Still understanding what is happening here might a good idea. From the
numbers below it seems there is 2 causes to the scaling issue. First
pte lock contention (kind of expected i guess). Second changes to vma
while faulting.
Have you thought about this ? Do i read those numbers in the wrong way ?
>
> Here are the performance counter read during a run on a 16 CPUs x86 VM:
> Performance counter stats for './ebizzy -mRTp':
> 888157 faults
> 884773 spf
> 92 pagefault:spf_pte_lock
> 2379 pagefault:spf_vma_changed
> 0 pagefault:spf_vma_noanon
> 80 pagefault:spf_vma_notsup
> 0 pagefault:spf_vma_access
> 0 pagefault:spf_pmd_changed
>
> And the ones captured during a run on a 80 CPUs Power node:
> Performance counter stats for './ebizzy -mRTp':
> 762134 faults
> 728663 spf
> 19101 pagefault:spf_pte_lock
> 13969 pagefault:spf_vma_changed
> 0 pagefault:spf_vma_noanon
> 272 pagefault:spf_vma_notsup
> 0 pagefault:spf_vma_access
> 0 pagefault:spf_pmd_changed
There is one aspect that i would like to see cover. Maybe i am not
understanding something fundamental, but it seems to me that SPF can
trigger OOM or at very least over stress page allocation.
Assume you have a lot of concurrent SPF to anonymous vma and they all
allocate new pages, then you might overallocate for a single address
by a factor correlated with the number of CPUs in your system. Now,
multiply this for several distinc address and you might be allocating
a lot of memory transiently ie just for a short period time. While
the fact that you quickly free when you fail should prevent the OOM
reaper. But still this might severly stress the memory allocation
path.
Am i missing something in how this all work ? Or is the above some-
thing that might be of concern ? Should there be some boundary on the
maximum number of concurrent SPF (and thus boundary on maximum page
temporary page allocation) ?
Cheers,
Jérôme
^ permalink raw reply
* Re: [PATCH v9 06/24] mm: make pte_unmap_same compatible with SPF
From: David Rientjes @ 2018-04-03 20:40 UTC (permalink / raw)
To: Jerome Glisse
Cc: Laurent Dufour, paulmck, peterz, akpm, kirill, ak, mhocko, dave,
jack, Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner,
Ingo Molnar, hpa, Will Deacon, Sergey Senozhatsky,
Andrea Arcangeli, Alexei Starovoitov, kemi.wang,
sergey.senozhatsky.work, Daniel Jordan, linux-kernel, linux-mm,
haren, khandual, npiggin, bsingharora, Tim Chen, linuxppc-dev,
x86
In-Reply-To: <20180403191005.GC5935@redhat.com>
On Tue, 3 Apr 2018, Jerome Glisse wrote:
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 21b1212a0892..4bc7b0bdcb40 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -2309,21 +2309,29 @@ static bool pte_map_lock(struct vm_fault *vmf)
> > * parts, do_swap_page must check under lock before unmapping the pte and
> > * proceeding (but do_wp_page is only called after already making such a check;
> > * and do_anonymous_page can safely check later on).
> > + *
> > + * pte_unmap_same() returns:
> > + * 0 if the PTE are the same
> > + * VM_FAULT_PTNOTSAME if the PTE are different
> > + * VM_FAULT_RETRY if the VMA has changed in our back during
> > + * a speculative page fault handling.
> > */
> > -static inline int pte_unmap_same(struct mm_struct *mm, pmd_t *pmd,
> > - pte_t *page_table, pte_t orig_pte)
> > +static inline int pte_unmap_same(struct vm_fault *vmf)
> > {
> > - int same = 1;
> > + int ret = 0;
> > +
> > #if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT)
> > if (sizeof(pte_t) > sizeof(unsigned long)) {
> > - spinlock_t *ptl = pte_lockptr(mm, pmd);
> > - spin_lock(ptl);
> > - same = pte_same(*page_table, orig_pte);
> > - spin_unlock(ptl);
> > + if (pte_spinlock(vmf)) {
> > + if (!pte_same(*vmf->pte, vmf->orig_pte))
> > + ret = VM_FAULT_PTNOTSAME;
> > + spin_unlock(vmf->ptl);
> > + } else
> > + ret = VM_FAULT_RETRY;
> > }
> > #endif
> > - pte_unmap(page_table);
> > - return same;
> > + pte_unmap(vmf->pte);
> > + return ret;
> > }
> >
> > static inline void cow_user_page(struct page *dst, struct page *src, unsigned long va, struct vm_area_struct *vma)
> > @@ -2913,7 +2921,8 @@ int do_swap_page(struct vm_fault *vmf)
> > int exclusive = 0;
> > int ret = 0;
> >
> > - if (!pte_unmap_same(vma->vm_mm, vmf->pmd, vmf->pte, vmf->orig_pte))
> > + ret = pte_unmap_same(vmf);
> > + if (ret)
> > goto out;
> >
>
> This change what do_swap_page() returns ie before it was returning 0
> when locked pte lookup was different from orig_pte. After this patch
> it returns VM_FAULT_PTNOTSAME but this is a new return value for
> handle_mm_fault() (the do_swap_page() return value is what ultimately
> get return by handle_mm_fault())
>
> Do we really want that ? This might confuse some existing user of
> handle_mm_fault() and i am not sure of the value of that information
> to caller.
>
> Note i do understand that you want to return retry if anything did
> change from underneath and thus need to differentiate from when the
> pte value are not the same.
>
I think VM_FAULT_RETRY should be handled appropriately for any user of
handle_mm_fault() already, and would be surprised to learn differently.
Khugepaged has the appropriate handling. I think the concern is whether a
user is handling anything other than VM_FAULT_RETRY and VM_FAULT_ERROR
(which VM_FAULT_PTNOTSAME is not set in)? I haven't done a full audit of
the users.
^ permalink raw reply
* Re: [PATCH 2/4] libnvdimm: Add device-tree based driver
From: kbuild test robot @ 2018-04-03 20:39 UTC (permalink / raw)
To: Oliver O'Halloran
Cc: kbuild-all, linuxppc-dev, 000linux-nvdimm, Oliver O'Halloran
In-Reply-To: <20180403142031.28629-2-oohall@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1222 bytes --]
Hi Oliver,
I love your patch! Yet something to improve:
[auto build test ERROR on linux-nvdimm/libnvdimm-for-next]
[also build test ERROR on v4.16 next-20180403]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Oliver-O-Halloran/libnvdimm-Add-of_node-to-region-and-bus-descriptors/20180404-015544
base: https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git libnvdimm-for-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64
All errors (new ones prefixed by >>):
ERROR: "ia64_delay_loop" [drivers/spi/spi-thunderx.ko] undefined!
>> ERROR: "of_node_to_nid" [drivers/nvdimm/of_pmem.ko] undefined!
ERROR: "ia64_delay_loop" [drivers/net/phy/mdio-cavium.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50211 bytes --]
^ permalink raw reply
* Re: [PATCH v9 15/24] mm: Introduce __vm_normal_page()
From: David Rientjes @ 2018-04-03 20:45 UTC (permalink / raw)
To: Jerome Glisse
Cc: Laurent Dufour, paulmck, peterz, akpm, kirill, ak, mhocko, dave,
jack, Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner,
Ingo Molnar, hpa, Will Deacon, Sergey Senozhatsky,
Andrea Arcangeli, Alexei Starovoitov, kemi.wang,
sergey.senozhatsky.work, Daniel Jordan, linux-kernel, linux-mm,
haren, khandual, npiggin, bsingharora, Tim Chen, linuxppc-dev,
x86
In-Reply-To: <20180403193927.GD5935@redhat.com>
On Tue, 3 Apr 2018, Jerome Glisse wrote:
> > When dealing with the speculative fault path we should use the VMA's field
> > cached value stored in the vm_fault structure.
> >
> > Currently vm_normal_page() is using the pointer to the VMA to fetch the
> > vm_flags value. This patch provides a new __vm_normal_page() which is
> > receiving the vm_flags flags value as parameter.
> >
> > Note: The speculative path is turned on for architecture providing support
> > for special PTE flag. So only the first block of vm_normal_page is used
> > during the speculative path.
>
> Might be a good idea to explicitly have SPECULATIVE Kconfig option depends
> on ARCH_PTE_SPECIAL and a comment for !HAVE_PTE_SPECIAL in the function
> explaining that speculative page fault should never reach that point.
Yeah, I think that's appropriate but in a follow-up patch since this is
only propagating vma_flags. It will require that __HAVE_ARCH_PTE_SPECIAL
become an actual Kconfig entry, however.
^ permalink raw reply
* Re: [PATCH v9 06/24] mm: make pte_unmap_same compatible with SPF
From: Jerome Glisse @ 2018-04-03 21:04 UTC (permalink / raw)
To: David Rientjes
Cc: Laurent Dufour, paulmck, peterz, akpm, kirill, ak, mhocko, dave,
jack, Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner,
Ingo Molnar, hpa, Will Deacon, Sergey Senozhatsky,
Andrea Arcangeli, Alexei Starovoitov, kemi.wang,
sergey.senozhatsky.work, Daniel Jordan, linux-kernel, linux-mm,
haren, khandual, npiggin, bsingharora, Tim Chen, linuxppc-dev,
x86
In-Reply-To: <alpine.DEB.2.20.1804031338260.172772@chino.kir.corp.google.com>
On Tue, Apr 03, 2018 at 01:40:18PM -0700, David Rientjes wrote:
> On Tue, 3 Apr 2018, Jerome Glisse wrote:
>
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index 21b1212a0892..4bc7b0bdcb40 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -2309,21 +2309,29 @@ static bool pte_map_lock(struct vm_fault *vmf)
> > > * parts, do_swap_page must check under lock before unmapping the pte and
> > > * proceeding (but do_wp_page is only called after already making such a check;
> > > * and do_anonymous_page can safely check later on).
> > > + *
> > > + * pte_unmap_same() returns:
> > > + * 0 if the PTE are the same
> > > + * VM_FAULT_PTNOTSAME if the PTE are different
> > > + * VM_FAULT_RETRY if the VMA has changed in our back during
> > > + * a speculative page fault handling.
> > > */
[...]
> > >
> >
> > This change what do_swap_page() returns ie before it was returning 0
> > when locked pte lookup was different from orig_pte. After this patch
> > it returns VM_FAULT_PTNOTSAME but this is a new return value for
> > handle_mm_fault() (the do_swap_page() return value is what ultimately
> > get return by handle_mm_fault())
> >
> > Do we really want that ? This might confuse some existing user of
> > handle_mm_fault() and i am not sure of the value of that information
> > to caller.
> >
> > Note i do understand that you want to return retry if anything did
> > change from underneath and thus need to differentiate from when the
> > pte value are not the same.
> >
>
> I think VM_FAULT_RETRY should be handled appropriately for any user of
> handle_mm_fault() already, and would be surprised to learn differently.
> Khugepaged has the appropriate handling. I think the concern is whether a
> user is handling anything other than VM_FAULT_RETRY and VM_FAULT_ERROR
> (which VM_FAULT_PTNOTSAME is not set in)? I haven't done a full audit of
> the users.
I am not worried about VM_FAULT_RETRY and barely have any worry about
VM_FAULT_PTNOTSAME either as they are other comparable new return value
(VM_FAULT_NEEDDSYNC for instance which is quite recent).
I wonder if adding a new value is really needed here. I don't see any
value to it for caller of handle_mm_fault() except for stats.
Note that I am not oppose, but while today we have free bits, maybe
tomorrow we will run out, i am always worried about thing like that :)
Cheers,
Jérôme
^ permalink raw reply
* Re: [PATCH v9 04/24] mm: Prepare for FAULT_FLAG_SPECULATIVE
From: David Rientjes @ 2018-04-03 21:57 UTC (permalink / raw)
To: Laurent Dufour
Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner, Ingo Molnar,
hpa, Will Deacon, Sergey Senozhatsky, Andrea Arcangeli,
Alexei Starovoitov, kemi.wang, sergey.senozhatsky.work,
Daniel Jordan, linux-kernel, linux-mm, haren, khandual, npiggin,
bsingharora, Tim Chen, linuxppc-dev, x86
In-Reply-To: <361fa6e7-3c17-e1b8-8046-af72c4459613@linux.vnet.ibm.com>
On Wed, 28 Mar 2018, Laurent Dufour wrote:
> >> diff --git a/include/linux/mm.h b/include/linux/mm.h
> >> index 4d02524a7998..2f3e98edc94a 100644
> >> --- a/include/linux/mm.h
> >> +++ b/include/linux/mm.h
> >> @@ -300,6 +300,7 @@ extern pgprot_t protection_map[16];
> >> #define FAULT_FLAG_USER 0x40 /* The fault originated in userspace */
> >> #define FAULT_FLAG_REMOTE 0x80 /* faulting for non current tsk/mm */
> >> #define FAULT_FLAG_INSTRUCTION 0x100 /* The fault was during an instruction fetch */
> >> +#define FAULT_FLAG_SPECULATIVE 0x200 /* Speculative fault, not holding mmap_sem */
> >>
> >> #define FAULT_FLAG_TRACE \
> >> { FAULT_FLAG_WRITE, "WRITE" }, \
> >
> > I think FAULT_FLAG_SPECULATIVE should be introduced in the patch that
> > actually uses it.
>
> I think you're right, I'll move down this define in the series.
>
> >> diff --git a/mm/memory.c b/mm/memory.c
> >> index e0ae4999c824..8ac241b9f370 100644
> >> --- a/mm/memory.c
> >> +++ b/mm/memory.c
> >> @@ -2288,6 +2288,13 @@ int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
> >> }
> >> EXPORT_SYMBOL_GPL(apply_to_page_range);
> >>
> >> +static bool pte_map_lock(struct vm_fault *vmf)
> >
> > inline?
>
> Agreed.
>
Ignore this, the final form of the function after the full patchset
shouldn't be inline.
> >> +{
> >> + vmf->pte = pte_offset_map_lock(vmf->vma->vm_mm, vmf->pmd,
> >> + vmf->address, &vmf->ptl);
> >> + return true;
> >> +}
> >> +
> >> /*
> >> * handle_pte_fault chooses page fault handler according to an entry which was
> >> * read non-atomically. Before making any commitment, on those architectures
> >> @@ -2477,6 +2484,7 @@ static int wp_page_copy(struct vm_fault *vmf)
> >> const unsigned long mmun_start = vmf->address & PAGE_MASK;
> >> const unsigned long mmun_end = mmun_start + PAGE_SIZE;
> >> struct mem_cgroup *memcg;
> >> + int ret = VM_FAULT_OOM;
> >>
> >> if (unlikely(anon_vma_prepare(vma)))
> >> goto oom;
> >> @@ -2504,7 +2512,11 @@ static int wp_page_copy(struct vm_fault *vmf)
> >> /*
> >> * Re-check the pte - we dropped the lock
> >> */
> >> - vmf->pte = pte_offset_map_lock(mm, vmf->pmd, vmf->address, &vmf->ptl);
> >> + if (!pte_map_lock(vmf)) {
> >> + mem_cgroup_cancel_charge(new_page, memcg, false);
> >> + ret = VM_FAULT_RETRY;
> >> + goto oom_free_new;
> >> + }
> >
> > Ugh, but we aren't oom here, so maybe rename oom_free_new so that it makes
> > sense for return values other than VM_FAULT_OOM?
>
> You're right, now this label name is not correct, I'll rename it to
> "out_free_new" and rename also the label "oom" to "out" since it is generic too
> now.
>
I think it would just be better to introduce a out_uncharge that handles
the mem_cgroup_cancel_charge() in the exit path.
diff --git a/mm/memory.c b/mm/memory.c
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2645,9 +2645,8 @@ static int wp_page_copy(struct vm_fault *vmf)
* Re-check the pte - we dropped the lock
*/
if (!pte_map_lock(vmf)) {
- mem_cgroup_cancel_charge(new_page, memcg, false);
ret = VM_FAULT_RETRY;
- goto oom_free_new;
+ goto out_uncharge;
}
if (likely(pte_same(*vmf->pte, vmf->orig_pte))) {
if (old_page) {
@@ -2735,6 +2734,8 @@ static int wp_page_copy(struct vm_fault *vmf)
put_page(old_page);
}
return page_copied ? VM_FAULT_WRITE : 0;
+out_uncharge:
+ mem_cgroup_cancel_charge(new_page, memcg, false);
oom_free_new:
put_page(new_page);
oom:
^ permalink raw reply
* Re: [mm] b1f0502d04: INFO:trying_to_register_non-static_key
From: David Rientjes @ 2018-04-04 0:48 UTC (permalink / raw)
To: Laurent Dufour
Cc: kernel test robot, paulmck, peterz, akpm, kirill, ak, mhocko,
dave, jack, Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner,
Ingo Molnar, hpa, Will Deacon, Sergey Senozhatsky,
Andrea Arcangeli, Alexei Starovoitov, kemi.wang,
sergey.senozhatsky.work, Daniel Jordan, linux-kernel, linux-mm,
haren, khandual, npiggin, bsingharora, Tim Chen, linuxppc-dev,
x86, lkp
In-Reply-To: <aa6f2ff1-ff67-106a-e0e4-522ac82a7bf0@linux.vnet.ibm.com>
On Wed, 28 Mar 2018, Laurent Dufour wrote:
> On 26/03/2018 00:10, David Rientjes wrote:
> > On Wed, 21 Mar 2018, Laurent Dufour wrote:
> >
> >> I found the root cause of this lockdep warning.
> >>
> >> In mmap_region(), unmap_region() may be called while vma_link() has not been
> >> called. This happens during the error path if call_mmap() failed.
> >>
> >> The only to fix that particular case is to call
> >> seqcount_init(&vma->vm_sequence) when initializing the vma in mmap_region().
> >>
> >
> > Ack, although that would require a fixup to dup_mmap() as well.
>
> You're right, I'll fix that too.
>
I also think the following is needed:
diff --git a/fs/exec.c b/fs/exec.c
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -312,6 +312,10 @@ static int __bprm_mm_init(struct linux_binprm *bprm)
vma->vm_flags = VM_SOFTDIRTY | VM_STACK_FLAGS | VM_STACK_INCOMPLETE_SETUP;
vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
INIT_LIST_HEAD(&vma->anon_vma_chain);
+#ifdef CONFIG_SPECULATIVE_PAGE_FAULT
+ seqcount_init(&vma->vm_sequence);
+ atomic_set(&vma->vm_ref_count, 0);
+#endif
err = insert_vm_struct(mm, vma);
if (err)
^ permalink raw reply
* Re: [mm] b1f0502d04: INFO:trying_to_register_non-static_key
From: David Rientjes @ 2018-04-04 1:03 UTC (permalink / raw)
To: Laurent Dufour
Cc: kernel test robot, paulmck, peterz, akpm, kirill, ak, mhocko,
dave, jack, Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner,
Ingo Molnar, hpa, Will Deacon, Sergey Senozhatsky,
Andrea Arcangeli, Alexei Starovoitov, kemi.wang,
sergey.senozhatsky.work, Daniel Jordan, linux-kernel, linux-mm,
haren, khandual, npiggin, bsingharora, Tim Chen, linuxppc-dev,
x86, lkp
In-Reply-To: <alpine.DEB.2.20.1804031748120.27686@chino.kir.corp.google.com>
On Tue, 3 Apr 2018, David Rientjes wrote:
> > >> I found the root cause of this lockdep warning.
> > >>
> > >> In mmap_region(), unmap_region() may be called while vma_link() has not been
> > >> called. This happens during the error path if call_mmap() failed.
> > >>
> > >> The only to fix that particular case is to call
> > >> seqcount_init(&vma->vm_sequence) when initializing the vma in mmap_region().
> > >>
> > >
> > > Ack, although that would require a fixup to dup_mmap() as well.
> >
> > You're right, I'll fix that too.
> >
>
> I also think the following is needed:
>
> diff --git a/fs/exec.c b/fs/exec.c
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -312,6 +312,10 @@ static int __bprm_mm_init(struct linux_binprm *bprm)
> vma->vm_flags = VM_SOFTDIRTY | VM_STACK_FLAGS | VM_STACK_INCOMPLETE_SETUP;
> vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> INIT_LIST_HEAD(&vma->anon_vma_chain);
> +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT
> + seqcount_init(&vma->vm_sequence);
> + atomic_set(&vma->vm_ref_count, 0);
> +#endif
>
> err = insert_vm_struct(mm, vma);
> if (err)
>
Ugh, I think there are a number of other places where this is needed as
well in mm/mmap.c. I think it would be better to just create a new
alloc_vma(unsigned long flags) that all vma allocators can use and for
CONFIG_SPECULATIVE_PAGE_FAULT will initialize the seqcount_t and atomic_t.
^ permalink raw reply
* [RFC 1/2] powerpc/swiotlb: Dont free up allocated SWIOTLB slab on POWER
From: Anshuman Khandual @ 2018-04-04 3:03 UTC (permalink / raw)
To: linuxppc-dev
Even though SWIOTLB slab gets allocated and initialized on powerpc with
swiotlb_init() called during mem_init(), it gets released away again on
POWER platform because 'ppc_swiotlb_enable' never gets set. The function
swiotlb_detect_4g() checks for 4GB memory and then sets the variable
'ppc_swiotlb_enable' which prevents freeing up the SWIOTLB slab. Lets
make POWER platform call swiotlb_detect_4g() during setup_arch() which
will keep the SWIOTLB slab through out the runtime.
A previous commit cf5621032f ("powerpc/64: Limit ZONE_DMA32 to 4GiB in
swiotlb_detect_4g()") enforced 4GB limit on ZONE_DMA32 which is is not
applicable on POWER (CONFIG_PPC_BOOK3S_64) platform. Lets remove this
unnecessary restriction.
After the patch, SWIOTLB slab does not get released.
[0.410992] software IO TLB [mem 0xfbff0000-0xffff0000] (64MB) mapped
at [00000000767f6cb3-000000004a10114f]
Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
arch/powerpc/kernel/dma-swiotlb.c | 2 +-
arch/powerpc/kernel/setup-common.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/dma-swiotlb.c b/arch/powerpc/kernel/dma-swiotlb.c
index 88f3963ca30f..2255c6dc89db 100644
--- a/arch/powerpc/kernel/dma-swiotlb.c
+++ b/arch/powerpc/kernel/dma-swiotlb.c
@@ -110,7 +110,7 @@ void __init swiotlb_detect_4g(void)
{
if ((memblock_end_of_DRAM() - 1) > 0xffffffff) {
ppc_swiotlb_enable = 1;
-#ifdef CONFIG_ZONE_DMA32
+#if defined(CONFIG_ZONE_DMA32) && !defined(CONFIG_PPC_BOOK3S_64)
limit_zone_pfn(ZONE_DMA32, (1ULL << 32) >> PAGE_SHIFT);
#endif
}
diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index d73ec518ef80..c4db844e0b0d 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -944,6 +944,7 @@ void __init setup_arch(char **cmdline_p)
/* Initialize the MMU context management stuff. */
mmu_context_init();
+ swiotlb_detect_4g();
#ifdef CONFIG_PPC64
/* Interrupt code needs to be 64K-aligned. */
if ((unsigned long)_stext & 0xffff)
--
2.14.1
^ permalink raw reply related
* [RFC 2/2] powerpc/swiotlb: Make swiotlb_dma_ops available on POWER
From: Anshuman Khandual @ 2018-04-04 3:03 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20180404030301.25019-1-khandual@linux.vnet.ibm.com>
Generic swiotlb_dma_ops structure is available under CONFIG_DMA_DIRECT_OPS
configuration. Hence select it on POWER platform.
Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
arch/powerpc/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 73ce5dd07642..61a55f9928d9 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -235,6 +235,7 @@ config PPC
select SPARSE_IRQ
select SYSCTL_EXCEPTION_TRACE
select VIRT_TO_BUS if !PPC64
+ select DMA_DIRECT_OPS if PPC_BOOK3S_64
#
# Please keep this list sorted alphabetically.
#
--
2.14.1
^ permalink raw reply related
* Re: [PATCH 2/2] smp: introduce kick_active_cpus_sync()
From: Yury Norov @ 2018-04-04 3:36 UTC (permalink / raw)
To: Mark Rutland
Cc: Will Deacon, Paul E. McKenney, Chris Metcalf, Christopher Lameter,
Russell King - ARM Linux, Steven Rostedt, Mathieu Desnoyers,
Catalin Marinas, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, linux-arm-kernel, linuxppc-dev, kvm-ppc,
linux-mm, linux-kernel
In-Reply-To: <20180403134832.2cdae64uwuot6ryz@lakrids.cambridge.arm.com>
Hi Mark,
Thank you for review.
On Tue, Apr 03, 2018 at 02:48:32PM +0100, Mark Rutland wrote:
> Hi Yury,
>
> On Sun, Apr 01, 2018 at 02:11:08PM +0300, Yury Norov wrote:
> > +/*
> > + * Flush I-cache if CPU is in extended quiescent state
> > + */
>
> This comment is misleading. An ISB doesn't touch the I-cache; it forces
> a context synchronization event.
>
> > + .macro isb_if_eqs
> > +#ifndef CONFIG_TINY_RCU
> > + bl rcu_is_watching
> > + tst w0, #0xff
> > + b.ne 1f
>
> The TST+B.NE can be a CBNZ:
>
> bl rcu_is_watching
> cbnz x0, 1f
> isb
> 1:
>
> > + /* Pairs with aarch64_insn_patch_text for EQS CPUs. */
> > + isb
> > +1:
> > +#endif
> > + .endm
> > +
> > el0_sync_invalid:
> > inv_entry 0, BAD_SYNC
> > ENDPROC(el0_sync_invalid)
> > @@ -840,8 +861,10 @@ el0_svc:
> > mov wsc_nr, #__NR_syscalls
> > el0_svc_naked: // compat entry point
> > stp x0, xscno, [sp, #S_ORIG_X0] // save the original x0 and syscall number
> > + isb_if_eqs
> > enable_dbg_and_irq
> > - ct_user_exit 1
> > + ct_user_exit
>
> I don't think this is safe. here we issue the ISB *before* exiting a
> quiesecent state, so I think we can race with another CPU that calls
> kick_all_active_cpus_sync, e.g.
>
> CPU0 CPU1
>
> ISB
> patch_some_text()
> kick_all_active_cpus_sync()
> ct_user_exit
>
> // not synchronized!
> use_of_patched_text()
>
> ... and therefore the ISB has no effect, which could be disasterous.
>
> I believe we need the ISB *after* we transition into a non-quiescent
> state, so that we can't possibly miss a context synchronization event.
I decided to put isb() in entry because there's a chance that there will
be patched code prior to exiting a quiescent state. But after some
headscratching, I think it's safe. I'll do like you suggested here.
Thanks,
Yury
^ permalink raw reply
* Re: [PATCH] powerpc: Clear branch trap (MSR.BE) before delivering SIGTRAP
From: Michael Ellerman @ 2018-04-04 6:11 UTC (permalink / raw)
To: Matt Evans; +Cc: linuxppc-dev
In-Reply-To: <71F58B2E-149D-41E4-A5BF-5A26E3491EA7@ozlabs.org>
Matt Evans <matt@ozlabs.org> writes:
>> On 28 Mar 2018, at 11:36, Matt Evans <matt@ozlabs.org> wrote:
>>> On 28 Mar 2018, at 06:54, Michael Ellerman <mpe@ellerman.id.au> wrote:
>>> Matt Evans <matt@ozlabs.org> writes:
>>>> When using SIG_DBG_BRANCH_TRACING, MSR.BE is left enabled in the
>>>> user context when single_step_exception() prepares the SIGTRAP
>>>> delivery. The resulting branch-trap-within-the-SIGTRAP-handler
>>>> isn't healthy.
>>>>=20
>>>> Commit 2538c2d08f46141550a1e68819efa8fe31c6e3dc broke this, by
>>>> replacing an MSR mask operation of ~(MSR_SE | MSR_BE) with a call
>>>> to clear_single_step() which only clears MSR_SE.
>>>>=20
>>>> This patch adds a new helper, clear_br_trace(), which clears the
>>>> debug trap before invoking the signal handler. This helper is a
>>>> NOP for BookE as SIG_DBG_BRANCH_TRACING isn't supported on BookE.
>>>>=20
>>>> Signed-off-by: Matt Evans <matt@ozlabs.org>
>>>=20
>>> Hi Matt!
>>>=20
>>> It seems we might not be regularly testing this code :}
>>=20
>> I know, rite? ;-)
>>=20
>>> How did you hit/find the bug? And do you have a test case by any chance?
>>>=20
>>> I found the test code at the bottom of:
>>> https://lwn.net/Articles/114587/
>>>=20
>>> But it didn't immediately work.
>>=20
>> I'm using this feature as part of a debug harness I wrote to log a
>> program=E2=80=99s control flow (to create a =E2=80=9Cknown good=E2=80=9D=
pattern to compare a
>> PPC interpreter against). So at least the feature has /one/ user. ;-)
One is enough for us :)
=20
>> The symptoms of the bug are that if you use single-stepping you get a
>> sequence of SIGTRAPs representing each instruction completion (good),
>> but if you use branch tracing the process just dies with SIGTRAP
>> (looks like it=E2=80=99s never caught by the signal handler). What=E2=80=
=99s really
>> happening is that there /is/ a signal delivered to the handler, but
>> (because branch tracing is left on) that then causes a second debug
>> exception from the handler itself, i.e. whilst SIGTRAP=E2=80=99s masked.
>>=20
>> OK, let me have a dig to reduce my program to something very basic
>> and I=E2=80=99ll post something =E2=80=94 sorry, I should=E2=80=99ve got=
a PoC ready before.
>> (I did start out inspired by that post you linked to, but IIRC I
>> don=E2=80=99t think it worked out of the box for me either.)
>
> I=E2=80=99ve put a simple SIG_DBG_BRANCH_TRACING test program here:
>
> http://ozlabs.org/~matt/files/sig_dbg_brtrace_test.c
Thanks.
> It=E2=80=99s commented regarding expected output. I=E2=80=99ve only teste=
d this on a
> G4 =E2=80=94 it should work on PPC64 too but the ISA says support for bra=
nch
> tracing is optional for an implementation. I=E2=80=99d be interested in w=
hat
> POWERx does. :)
I get no traps on any 64-bit machine I tried, including 970FX, Power6,
Power7, Power8, Power9.
So I guess it's never been implemented on "server" CPUs.
I'd be happy to turn your test program into a selftest, though it won't
actually catch bugs unless someone runs it on actual 32-bit systems :)
cheers
^ permalink raw reply
* Re: [PATCH v9 00/24] Speculative page faults
From: Laurent Dufour @ 2018-04-04 7:59 UTC (permalink / raw)
To: Jerome Glisse
Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner, Ingo Molnar,
hpa, Will Deacon, Sergey Senozhatsky, Andrea Arcangeli,
Alexei Starovoitov, kemi.wang, sergey.senozhatsky.work,
Daniel Jordan, linux-kernel, linux-mm, haren, khandual, npiggin,
bsingharora, Tim Chen, linuxppc-dev, x86
In-Reply-To: <20180403203718.GE5935@redhat.com>
Hi Jerome,
Thanks for reviewing this series.
On 03/04/2018 22:37, Jerome Glisse wrote:
> On Tue, Mar 13, 2018 at 06:59:30PM +0100, Laurent Dufour wrote:
>> This is a port on kernel 4.16 of the work done by Peter Zijlstra to
>> handle page fault without holding the mm semaphore [1].
>>
>> The idea is to try to handle user space page faults without holding the
>> mmap_sem. This should allow better concurrency for massively threaded
>> process since the page fault handler will not wait for other threads memory
>> layout change to be done, assuming that this change is done in another part
>> of the process's memory space. This type page fault is named speculative
>> page fault. If the speculative page fault fails because of a concurrency is
>> detected or because underlying PMD or PTE tables are not yet allocating, it
>> is failing its processing and a classic page fault is then tried.
>>
>> The speculative page fault (SPF) has to look for the VMA matching the fault
>> address without holding the mmap_sem, this is done by introducing a rwlock
>> which protects the access to the mm_rb tree. Previously this was done using
>> SRCU but it was introducing a lot of scheduling to process the VMA's
>> freeing
>> operation which was hitting the performance by 20% as reported by Kemi Wang
>> [2].Using a rwlock to protect access to the mm_rb tree is limiting the
>> locking contention to these operations which are expected to be in a O(log
>> n)
>> order. In addition to ensure that the VMA is not freed in our back a
>> reference count is added and 2 services (get_vma() and put_vma()) are
>> introduced to handle the reference count. When a VMA is fetch from the RB
>> tree using get_vma() is must be later freeed using put_vma(). Furthermore,
>> to allow the VMA to be used again by the classic page fault handler a
>> service is introduced can_reuse_spf_vma(). This service is expected to be
>> called with the mmap_sem hold. It checked that the VMA is still matching
>> the specified address and is releasing its reference count as the mmap_sem
>> is hold it is ensure that it will not be freed in our back. In general, the
>> VMA's reference count could be decremented when holding the mmap_sem but it
>> should not be increased as holding the mmap_sem is ensuring that the VMA is
>> stable. I can't see anymore the overhead I got while will-it-scale
>> benchmark anymore.
>>
>> The VMA's attributes checked during the speculative page fault processing
>> have to be protected against parallel changes. This is done by using a per
>> VMA sequence lock. This sequence lock allows the speculative page fault
>> handler to fast check for parallel changes in progress and to abort the
>> speculative page fault in that case.
>>
>> Once the VMA is found, the speculative page fault handler would check for
>> the VMA's attributes to verify that the page fault has to be handled
>> correctly or not. Thus the VMA is protected through a sequence lock which
>> allows fast detection of concurrent VMA changes. If such a change is
>> detected, the speculative page fault is aborted and a *classic* page fault
>> is tried. VMA sequence lockings are added when VMA attributes which are
>> checked during the page fault are modified.
>>
>> When the PTE is fetched, the VMA is checked to see if it has been changed,
>> so once the page table is locked, the VMA is valid, so any other changes
>> leading to touching this PTE will need to lock the page table, so no
>> parallel change is possible at this time.
>
> What would have been nice is some pseudo highlevel code before all the
> above detailed description. Something like:
> speculative_fault(addr) {
> mm_lock_for_vma_snapshot()
> vma_snapshot = snapshot_vma_infos(addr)
> mm_unlock_for_vma_snapshot()
> ...
> if (!vma_can_speculatively_fault(vma_snapshot, addr))
> return;
> ...
> /* Do fault ie alloc memory, read from file ... */
> page = ...;
>
> preempt_disable();
> if (vma_snapshot_still_valid(vma_snapshot, addr) &&
> vma_pte_map_lock(vma_snapshot, addr)) {
> if (pte_same(ptep, orig_pte)) {
> /* Setup new pte */
> page = NULL;
> }
> }
> preempt_enable();
> if (page)
> put(page)
> }
>
> I just find pseudo code easier for grasping the highlevel view of the
> expected code flow.
Fair enough, I agree that sounds easier this way, but one might argue that the
pseudo code is not more valid or accurate at one time :)
As always, the updated documentation is the code itself.
I'll try to put one inspired by yours in the next series's header.
>>
>> The locking of the PTE is done with interrupts disabled, this allows to
>> check for the PMD to ensure that there is not an ongoing collapsing
>> operation. Since khugepaged is firstly set the PMD to pmd_none and then is
>> waiting for the other CPU to have catch the IPI interrupt, if the pmd is
>> valid at the time the PTE is locked, we have the guarantee that the
>> collapsing opertion will have to wait on the PTE lock to move foward. This
>> allows the SPF handler to map the PTE safely. If the PMD value is different
>> than the one recorded at the beginning of the SPF operation, the classic
>> page fault handler will be called to handle the operation while holding the
>> mmap_sem. As the PTE lock is done with the interrupts disabled, the lock is
>> done using spin_trylock() to avoid dead lock when handling a page fault
>> while a TLB invalidate is requested by an other CPU holding the PTE.
>>
>> Support for THP is not done because when checking for the PMD, we can be
>> confused by an in progress collapsing operation done by khugepaged. The
>> issue is that pmd_none() could be true either if the PMD is not already
>> populated or if the underlying PTE are in the way to be collapsed. So we
>> cannot safely allocate a PMD if pmd_none() is true.
>
> Might be a good topic fo LSF/MM, should we set the pmd to something
> else then 0 when collapsing pmd (apply to pud too) ? This would allow
> support THP.
Absolutely !
> [...]
>
>>
>> Ebizzy:
>> -------
>> The test is counting the number of records per second it can manage, the
>> higher is the best. I run it like this 'ebizzy -mTRp'. To get consistent
>> result I repeated the test 100 times and measure the average result. The
>> number is the record processes per second, the higher is the best.
>>
>> BASE SPF delta
>> 16 CPUs x86 VM 14902.6 95905.16 543.55%
>> 80 CPUs P8 node 37240.24 78185.67 109.95%
>
> I find those results interesting as it seems that SPF do not scale well
> on big configuration. Note that it still have a sizeable improvement so
> it is still a very interesting feature i believe.
>
> Still understanding what is happening here might a good idea. From the
> numbers below it seems there is 2 causes to the scaling issue. First
> pte lock contention (kind of expected i guess). Second changes to vma
> while faulting.
>
> Have you thought about this ? Do i read those numbers in the wrong way ?
Your reading of the numbers is correct, but there is also another point to keep
in mind, on ppc64, the default page size is 64K, and since we are mapping new
pages for user space, those pages have to be cleared, leading to more time
spent clearing pages on ppc64 which leads to less page fault ratio on ppc64.
And since the VMA is checked again once the cleared page is allocated, there is
a major chance for that VMA to be touched in the ebizzy case.
>>
>> Here are the performance counter read during a run on a 16 CPUs x86 VM:
>> Performance counter stats for './ebizzy -mRTp':
>> As always, the updated documentation is the code itself.
>> 888157 faults
>> 884773 spf
>> 92 pagefault:spf_pte_lock
>> 2379 pagefault:spf_vma_changed
>> 0 pagefault:spf_vma_noanon
>> 80 pagefault:spf_vma_notsup
>> 0 pagefault:spf_vma_access
>> 0 pagefault:spf_pmd_changed
>>
>> And the ones captured during a run on a 80 CPUs Power node:
>> Performance counter stats for './ebizzy -mRTp':
>> 762134 faults
>> 728663 spf
>> 19101 pagefault:spf_pte_lock
>> 13969 pagefault:spf_vma_changed
>> 0 pagefault:spf_vma_noanon
>> 272 pagefault:spf_vma_notsup
>> 0 pagefault:spf_vma_access
>> 0 pagefault:spf_pmd_changed
>
>
> There is one aspect that i would like to see cover. Maybe i am not
> understanding something fundamental, but it seems to me that SPF can
> trigger OOM or at very least over stress page allocation.
>
> Assume you have a lot of concurrent SPF to anonymous vma and they all
> allocate new pages, then you might overallocate for a single address
> by a factor correlated with the number of CPUs in your system. Now,
> multiply this for several distinc address and you might be allocating
> a lot of memory transiently ie just for a short period time. While
> the fact that you quickly free when you fail should prevent the OOM
> reaper. But still this might severly stress the memory allocation
> path.
That's an interesting point, and you're right, SPF may lead to page allocation
that will not be used.
But as you mentioned this will be a factor of CPU numbers, so the max page
overhead, assuming that all minus one threads of the same process are dealing
with page on the same VMA and the last one is touching that VMA parallel,
is (nrcpus-1) page allocated at one time which may not be used immediately.
I'm not sure this will be a major risk, but I might be too optimistic.
This raises also the question of the cleared page cache, I'd have to see if
there is such a cache is in place.
> Am i missing something in how this all work ? Or is the above some-
> thing that might be of concern ? Should there be some boundary on the
> maximum number of concurrent SPF (and thus boundary on maximum page
> temporary page allocation) ?
I don't think you're missing anything ;)
It would be easy to introduce such a limit in the case OOM are trigger too many
times due to SPF handling.
Cheers,
Laurent.
^ permalink raw reply
* Re: [PATCH v9 09/24] mm: protect mremap() against SPF hanlder
From: Laurent Dufour @ 2018-04-04 8:24 UTC (permalink / raw)
To: David Rientjes
Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner, Ingo Molnar,
hpa, Will Deacon, Sergey Senozhatsky, Andrea Arcangeli,
Alexei Starovoitov, kemi.wang, sergey.senozhatsky.work,
Daniel Jordan, linux-kernel, linux-mm, haren, khandual, npiggin,
bsingharora, Tim Chen, linuxppc-dev, x86
In-Reply-To: <alpine.DEB.2.20.1803281419460.167685@chino.kir.corp.google.com>
On 28/03/2018 23:21, David Rientjes wrote:
> On Wed, 28 Mar 2018, Laurent Dufour wrote:
>
>>>> @@ -326,7 +336,10 @@ static unsigned long move_vma(struct vm_area_struct *vma,
>>>> mremap_userfaultfd_prep(new_vma, uf);
>>>> arch_remap(mm, old_addr, old_addr + old_len,
>>>> new_addr, new_addr + new_len);
>>>> + if (vma != new_vma)
>>>> + vm_raw_write_end(vma);
>>>> }
>>>> + vm_raw_write_end(new_vma);
>>>
>>> Just do
>>>
>>> vm_raw_write_end(vma);
>>> vm_raw_write_end(new_vma);
>>>
>>> here.
>>
>> Are you sure ? we can have vma = new_vma done if (unlikely(err))
>>
>
> Sorry, what I meant was do
>
> if (vma != new_vma)
> vm_raw_write_end(vma);
> vm_raw_write_end(new_vma);
>
> after the conditional. Having the locking unnecessarily embedded in the
> conditional has been an issue in the past with other areas of core code,
> unless you have a strong reason for it.
Unfortunately, I can't see how doing this in another way since vma = new_vma is
done in the error branch.
So releasing the VMAs outside of the conditional may lead to miss 'vma' if the
error branch is taken.
Here is the code snippet as a reminder:
new_vma = copy_vma(&vma, new_addr, new_len, new_pgoff,
&need_rmap_locks);
[...]
if (vma != new_vma)
vm_raw_write_begin(vma);
[...]
if (unlikely(err)) {
[...]
if (vma != new_vma)
vm_raw_write_end(vma);
vma = new_vma; <<<< here we lost reference to vma
[...]
} else {
[...]
if (vma != new_vma)
vm_raw_write_end(vma);
}
vm_raw_write_end(new_vma);
^ permalink raw reply
* Re: [PATCH 2/2] smp: introduce kick_active_cpus_sync()
From: Mark Rutland @ 2018-04-04 9:08 UTC (permalink / raw)
To: Yury Norov
Cc: Will Deacon, Paul E. McKenney, Chris Metcalf, Christopher Lameter,
Russell King - ARM Linux, Steven Rostedt, Mathieu Desnoyers,
Catalin Marinas, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, linux-arm-kernel, linuxppc-dev, kvm-ppc,
linux-mm, linux-kernel
In-Reply-To: <20180404033625.gkn4q7kb2xf6d6mo@yury-thinkpad>
On Wed, Apr 04, 2018 at 06:36:25AM +0300, Yury Norov wrote:
> On Tue, Apr 03, 2018 at 02:48:32PM +0100, Mark Rutland wrote:
> > On Sun, Apr 01, 2018 at 02:11:08PM +0300, Yury Norov wrote:
> > > @@ -840,8 +861,10 @@ el0_svc:
> > > mov wsc_nr, #__NR_syscalls
> > > el0_svc_naked: // compat entry point
> > > stp x0, xscno, [sp, #S_ORIG_X0] // save the original x0 and syscall number
> > > + isb_if_eqs
> > > enable_dbg_and_irq
> > > - ct_user_exit 1
> > > + ct_user_exit
> >
> > I don't think this is safe. here we issue the ISB *before* exiting a
> > quiesecent state, so I think we can race with another CPU that calls
> > kick_all_active_cpus_sync, e.g.
> >
> > CPU0 CPU1
> >
> > ISB
> > patch_some_text()
> > kick_all_active_cpus_sync()
> > ct_user_exit
> >
> > // not synchronized!
> > use_of_patched_text()
> >
> > ... and therefore the ISB has no effect, which could be disasterous.
> >
> > I believe we need the ISB *after* we transition into a non-quiescent
> > state, so that we can't possibly miss a context synchronization event.
>
> I decided to put isb() in entry because there's a chance that there will
> be patched code prior to exiting a quiescent state.
If we do patch entry text, then I think we have no option but to use
kick_all_active_cpus_sync(), or we risk races similar to the above.
> But after some headscratching, I think it's safe. I'll do like you
> suggested here.
Sounds good.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v9 04/24] mm: Prepare for FAULT_FLAG_SPECULATIVE
From: Laurent Dufour @ 2018-04-04 9:23 UTC (permalink / raw)
To: David Rientjes
Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner, Ingo Molnar,
hpa, Will Deacon, Sergey Senozhatsky, Andrea Arcangeli,
Alexei Starovoitov, kemi.wang, sergey.senozhatsky.work,
Daniel Jordan, linux-kernel, linux-mm, haren, khandual, npiggin,
bsingharora, Tim Chen, linuxppc-dev, x86
In-Reply-To: <alpine.DEB.2.20.1804031449130.153232@chino.kir.corp.google.com>
On 03/04/2018 23:57, David Rientjes wrote:
> On Wed, 28 Mar 2018, Laurent Dufour wrote:
>
>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>> index 4d02524a7998..2f3e98edc94a 100644
>>>> --- a/include/linux/mm.h
>>>> +++ b/include/linux/mm.h
>>>> @@ -300,6 +300,7 @@ extern pgprot_t protection_map[16];
>>>> #define FAULT_FLAG_USER 0x40 /* The fault originated in userspace */
>>>> #define FAULT_FLAG_REMOTE 0x80 /* faulting for non current tsk/mm */
>>>> #define FAULT_FLAG_INSTRUCTION 0x100 /* The fault was during an instruction fetch */
>>>> +#define FAULT_FLAG_SPECULATIVE 0x200 /* Speculative fault, not holding mmap_sem */
>>>>
>>>> #define FAULT_FLAG_TRACE \
>>>> { FAULT_FLAG_WRITE, "WRITE" }, \
>>>
>>> I think FAULT_FLAG_SPECULATIVE should be introduced in the patch that
>>> actually uses it.
>>
>> I think you're right, I'll move down this define in the series.
>>
>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>> index e0ae4999c824..8ac241b9f370 100644
>>>> --- a/mm/memory.c
>>>> +++ b/mm/memory.c
>>>> @@ -2288,6 +2288,13 @@ int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
>>>> }
>>>> EXPORT_SYMBOL_GPL(apply_to_page_range);
>>>>
>>>> +static bool pte_map_lock(struct vm_fault *vmf)
>>>
>>> inline?
>>
>> Agreed.
>>
>
> Ignore this, the final form of the function after the full patchset
> shouldn't be inline.
Indeed, I only kept as inlined the small pte_map_lock() and later
pte_spinlock() defined when CONFIG_SPECULATIVE_PAGE_FAULT is not set.
>>>> +{
>>>> + vmf->pte = pte_offset_map_lock(vmf->vma->vm_mm, vmf->pmd,
>>>> + vmf->address, &vmf->ptl);
>>>> + return true;
>>>> +}
>>>> +
>>>> /*
>>>> * handle_pte_fault chooses page fault handler according to an entry which was
>>>> * read non-atomically. Before making any commitment, on those architectures
>>>> @@ -2477,6 +2484,7 @@ static int wp_page_copy(struct vm_fault *vmf)
>>>> const unsigned long mmun_start = vmf->address & PAGE_MASK;
>>>> const unsigned long mmun_end = mmun_start + PAGE_SIZE;
>>>> struct mem_cgroup *memcg;
>>>> + int ret = VM_FAULT_OOM;
>>>>
>>>> if (unlikely(anon_vma_prepare(vma)))
>>>> goto oom;
>>>> @@ -2504,7 +2512,11 @@ static int wp_page_copy(struct vm_fault *vmf)
>>>> /*
>>>> * Re-check the pte - we dropped the lock
>>>> */
>>>> - vmf->pte = pte_offset_map_lock(mm, vmf->pmd, vmf->address, &vmf->ptl);
>>>> + if (!pte_map_lock(vmf)) {
>>>> + mem_cgroup_cancel_charge(new_page, memcg, false);
>>>> + ret = VM_FAULT_RETRY;
>>>> + goto oom_free_new;
>>>> + }
>>>
>>> Ugh, but we aren't oom here, so maybe rename oom_free_new so that it makes
>>> sense for return values other than VM_FAULT_OOM?
>>
>> You're right, now this label name is not correct, I'll rename it to
>> "out_free_new" and rename also the label "oom" to "out" since it is generic too
>> now.
>>
>
> I think it would just be better to introduce a out_uncharge that handles
> the mem_cgroup_cancel_charge() in the exit path.
Yes adding an out_uncharge label sounds good too. I'll add it and also rename
oom_* ones to out_*.
>
> diff --git a/mm/memory.c b/mm/memory.c
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2645,9 +2645,8 @@ static int wp_page_copy(struct vm_fault *vmf)
> * Re-check the pte - we dropped the lock
> */
> if (!pte_map_lock(vmf)) {
> - mem_cgroup_cancel_charge(new_page, memcg, false);
> ret = VM_FAULT_RETRY;
> - goto oom_free_new;
> + goto out_uncharge;
> }
> if (likely(pte_same(*vmf->pte, vmf->orig_pte))) {
> if (old_page) {
> @@ -2735,6 +2734,8 @@ static int wp_page_copy(struct vm_fault *vmf)
> put_page(old_page);
> }
> return page_copied ? VM_FAULT_WRITE : 0;
> +out_uncharge:
> + mem_cgroup_cancel_charge(new_page, memcg, false);
> oom_free_new:
> put_page(new_page);
> oom:
>
^ permalink raw reply
* Re: [PATCH v9 06/24] mm: make pte_unmap_same compatible with SPF
From: Laurent Dufour @ 2018-04-04 9:53 UTC (permalink / raw)
To: Jerome Glisse
Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner, Ingo Molnar,
hpa, Will Deacon, Sergey Senozhatsky, Andrea Arcangeli,
Alexei Starovoitov, kemi.wang, sergey.senozhatsky.work,
Daniel Jordan, linux-kernel, linux-mm, haren, khandual, npiggin,
bsingharora, Tim Chen, linuxppc-dev, x86
In-Reply-To: <20180403191005.GC5935@redhat.com>
On 03/04/2018 21:10, Jerome Glisse wrote:
> On Tue, Mar 13, 2018 at 06:59:36PM +0100, Laurent Dufour wrote:
>> pte_unmap_same() is making the assumption that the page table are still
>> around because the mmap_sem is held.
>> This is no more the case when running a speculative page fault and
>> additional check must be made to ensure that the final page table are still
>> there.
>>
>> This is now done by calling pte_spinlock() to check for the VMA's
>> consistency while locking for the page tables.
>>
>> This is requiring passing a vm_fault structure to pte_unmap_same() which is
>> containing all the needed parameters.
>>
>> As pte_spinlock() may fail in the case of a speculative page fault, if the
>> VMA has been touched in our back, pte_unmap_same() should now return 3
>> cases :
>> 1. pte are the same (0)
>> 2. pte are different (VM_FAULT_PTNOTSAME)
>> 3. a VMA's changes has been detected (VM_FAULT_RETRY)
>>
>> The case 2 is handled by the introduction of a new VM_FAULT flag named
>> VM_FAULT_PTNOTSAME which is then trapped in cow_user_page().
>> If VM_FAULT_RETRY is returned, it is passed up to the callers to retry the
>> page fault while holding the mmap_sem.
>>
>> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>> ---
>> include/linux/mm.h | 1 +
>> mm/memory.c | 29 +++++++++++++++++++----------
>> 2 files changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 2f3e98edc94a..b6432a261e63 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1199,6 +1199,7 @@ static inline void clear_page_pfmemalloc(struct page *page)
>> #define VM_FAULT_NEEDDSYNC 0x2000 /* ->fault did not modify page tables
>> * and needs fsync() to complete (for
>> * synchronous page faults in DAX) */
>> +#define VM_FAULT_PTNOTSAME 0x4000 /* Page table entries have changed */
>>
>> #define VM_FAULT_ERROR (VM_FAULT_OOM | VM_FAULT_SIGBUS | VM_FAULT_SIGSEGV | \
>> VM_FAULT_HWPOISON | VM_FAULT_HWPOISON_LARGE | \
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 21b1212a0892..4bc7b0bdcb40 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -2309,21 +2309,29 @@ static bool pte_map_lock(struct vm_fault *vmf)
>> * parts, do_swap_page must check under lock before unmapping the pte and
>> * proceeding (but do_wp_page is only called after already making such a check;
>> * and do_anonymous_page can safely check later on).
>> + *
>> + * pte_unmap_same() returns:
>> + * 0 if the PTE are the same
>> + * VM_FAULT_PTNOTSAME if the PTE are different
>> + * VM_FAULT_RETRY if the VMA has changed in our back during
>> + * a speculative page fault handling.
>> */
>> -static inline int pte_unmap_same(struct mm_struct *mm, pmd_t *pmd,
>> - pte_t *page_table, pte_t orig_pte)
>> +static inline int pte_unmap_same(struct vm_fault *vmf)
>> {
>> - int same = 1;
>> + int ret = 0;
>> +
>> #if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT)
>> if (sizeof(pte_t) > sizeof(unsigned long)) {
>> - spinlock_t *ptl = pte_lockptr(mm, pmd);
>> - spin_lock(ptl);
>> - same = pte_same(*page_table, orig_pte);
>> - spin_unlock(ptl);
>> + if (pte_spinlock(vmf)) {
>> + if (!pte_same(*vmf->pte, vmf->orig_pte))
>> + ret = VM_FAULT_PTNOTSAME;
>> + spin_unlock(vmf->ptl);
>> + } else
>> + ret = VM_FAULT_RETRY;
>> }
>> #endif
>> - pte_unmap(page_table);
>> - return same;
>> + pte_unmap(vmf->pte);
>> + return ret;
>> }
>>
>> static inline void cow_user_page(struct page *dst, struct page *src, unsigned long va, struct vm_area_struct *vma)
>> @@ -2913,7 +2921,8 @@ int do_swap_page(struct vm_fault *vmf)
>> int exclusive = 0;
>> int ret = 0;
>>
>> - if (!pte_unmap_same(vma->vm_mm, vmf->pmd, vmf->pte, vmf->orig_pte))
>> + ret = pte_unmap_same(vmf);
>> + if (ret)
>> goto out;
>>
>
> This change what do_swap_page() returns ie before it was returning 0
> when locked pte lookup was different from orig_pte. After this patch
> it returns VM_FAULT_PTNOTSAME but this is a new return value for
> handle_mm_fault() (the do_swap_page() return value is what ultimately
> get return by handle_mm_fault())
>
> Do we really want that ? This might confuse some existing user of
> handle_mm_fault() and i am not sure of the value of that information
> to caller.
>
> Note i do understand that you want to return retry if anything did
> change from underneath and thus need to differentiate from when the
> pte value are not the same.
You're right, do_swap_page() should still return 0 in the case the lookup pte
is different from orig_pte, assuming that the swap operation has been handled
in our back by another concurrent thread.
So in do_swap_page(), VM_FAULT_PTNOTSAME should be translated in ret = 0.
ret = pte_unmap_same(vmf);
if (ret) {
/*
* If pte != orig_pte, this means another thread did the
* swap operation in our back.
* So nothing else to do.
*/
if (ret == VM_FAULT_PTNOTSAME)
ret = 0;
goto out;
}
This means that VM_FAULT_PTNOTSAME will never been reported up and limited to
do_swap_page().
Doing this will make easier to understand why when pte_unmap_same() is
returning 0, do_swap_page() is done.
Cheers,
Laurent.
^ permalink raw reply
* Re: [mm] b1f0502d04: INFO:trying_to_register_non-static_key
From: Laurent Dufour @ 2018-04-04 10:19 UTC (permalink / raw)
To: David Rientjes
Cc: kernel test robot, paulmck, peterz, akpm, kirill, ak, mhocko,
dave, jack, Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner,
Ingo Molnar, hpa, Will Deacon, Sergey Senozhatsky,
Andrea Arcangeli, Alexei Starovoitov, kemi.wang,
sergey.senozhatsky.work, Daniel Jordan, linux-kernel, linux-mm,
haren, khandual, npiggin, bsingharora, Tim Chen, linuxppc-dev,
x86, lkp
In-Reply-To: <alpine.DEB.2.20.1804031748120.27686@chino.kir.corp.google.com>
On 04/04/2018 02:48, David Rientjes wrote:
> On Wed, 28 Mar 2018, Laurent Dufour wrote:
>
>> On 26/03/2018 00:10, David Rientjes wrote:
>>> On Wed, 21 Mar 2018, Laurent Dufour wrote:
>>>
>>>> I found the root cause of this lockdep warning.
>>>>
>>>> In mmap_region(), unmap_region() may be called while vma_link() has not been
>>>> called. This happens during the error path if call_mmap() failed.
>>>>
>>>> The only to fix that particular case is to call
>>>> seqcount_init(&vma->vm_sequence) when initializing the vma in mmap_region().
>>>>
>>>
>>> Ack, although that would require a fixup to dup_mmap() as well.
>>
>> You're right, I'll fix that too.
>>
>
> I also think the following is needed:
>
> diff --git a/fs/exec.c b/fs/exec.c
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -312,6 +312,10 @@ static int __bprm_mm_init(struct linux_binprm *bprm)
> vma->vm_flags = VM_SOFTDIRTY | VM_STACK_FLAGS | VM_STACK_INCOMPLETE_SETUP;
> vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> INIT_LIST_HEAD(&vma->anon_vma_chain);
> +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT
> + seqcount_init(&vma->vm_sequence);
> + atomic_set(&vma->vm_ref_count, 0);
> +#endif
>
> err = insert_vm_struct(mm, vma);
> if (err)
No, this not needed because the vma is allocated with kmem_cache_zalloc() so
vm_ref_count is 0, and insert_vm_struc() will later call
__vma_link_rb() which will call seqcount_init().
Furhtermore, in case of error, the vma structure is freed without calling
get_vma() so there is risk of lockdep warning.
^ permalink raw reply
* Re: [mm] b1f0502d04: INFO:trying_to_register_non-static_key
From: Laurent Dufour @ 2018-04-04 10:28 UTC (permalink / raw)
To: David Rientjes
Cc: kernel test robot, paulmck, peterz, akpm, kirill, ak, mhocko,
dave, jack, Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner,
Ingo Molnar, hpa, Will Deacon, Sergey Senozhatsky,
Andrea Arcangeli, Alexei Starovoitov, kemi.wang,
sergey.senozhatsky.work, Daniel Jordan, linux-kernel, linux-mm,
haren, khandual, npiggin, bsingharora, Tim Chen, linuxppc-dev,
x86, lkp
In-Reply-To: <alpine.DEB.2.20.1804031802110.50064@chino.kir.corp.google.com>
On 04/04/2018 03:03, David Rientjes wrote:
> On Tue, 3 Apr 2018, David Rientjes wrote:
>
>>>>> I found the root cause of this lockdep warning.
>>>>>
>>>>> In mmap_region(), unmap_region() may be called while vma_link() has not been
>>>>> called. This happens during the error path if call_mmap() failed.
>>>>>
>>>>> The only to fix that particular case is to call
>>>>> seqcount_init(&vma->vm_sequence) when initializing the vma in mmap_region().
>>>>>
>>>>
>>>> Ack, although that would require a fixup to dup_mmap() as well.
>>>
>>> You're right, I'll fix that too.
>>>
>>
>> I also think the following is needed:
>>
>> diff --git a/fs/exec.c b/fs/exec.c
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -312,6 +312,10 @@ static int __bprm_mm_init(struct linux_binprm *bprm)
>> vma->vm_flags = VM_SOFTDIRTY | VM_STACK_FLAGS | VM_STACK_INCOMPLETE_SETUP;
>> vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>> INIT_LIST_HEAD(&vma->anon_vma_chain);
>> +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT
>> + seqcount_init(&vma->vm_sequence);
>> + atomic_set(&vma->vm_ref_count, 0);
>> +#endif
>>
>> err = insert_vm_struct(mm, vma);
>> if (err)
>>
>
> Ugh, I think there are a number of other places where this is needed as
> well in mm/mmap.c. I think it would be better to just create a new
> alloc_vma(unsigned long flags) that all vma allocators can use and for
> CONFIG_SPECULATIVE_PAGE_FAULT will initialize the seqcount_t and atomic_t.
>
I don't think this is really needed, most of the time the vma structure is
allocated cleared and is then link to rb tree via vma_link.
The only case generating a locked warning is when the vma is referenced in the
error path before being linked in the mm tree and that is not usual.
^ permalink raw reply
* [PATCH] powerpc/hw_breakpoint: Only disable hw breakpoint if cpu supports it
From: Naveen N. Rao @ 2018-04-04 10:41 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
We get the below warning if we try to use kexec on P9:
kexec_core: Starting new kernel
WARNING: CPU: 0 PID: 1223 at arch/powerpc/kernel/process.c:826 __set_breakpoint+0xb4/0x140
[snip]
NIP [c00000000001bf44] __set_breakpoint+0xb4/0x140
LR [c000000000061268] kexec_prepare_cpus_wait+0x58/0x150
Call Trace:
[c0000000ee70fad0] [c0000000ee70fb20] 0xc0000000ee70fb20 (unreliable)
[c0000000ee70faf0] [c0000000ee70fb20] 0xc0000000ee70fb20
[c0000000ee70fbb0] [c000000000061884] default_machine_kexec+0x234/0x2c0
[c0000000ee70fc40] [c0000000000605c4] machine_kexec+0x84/0x90
[c0000000ee70fc70] [c000000000206548] kernel_kexec+0xd8/0xe0
[c0000000ee70fce0] [c000000000169e74] SyS_reboot+0x214/0x2c0
[c0000000ee70fe30] [c00000000000bd60] system_call+0x58/0x6c
This happens since we are trying to clear hw breakpoint on POWER9, though
we don't have CPU_FTR_DAWR enabled. Guard __set_breakpoint() within
hw_breakpoint_disable() with ppc_breakpoint_available() to address this.
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/hw_breakpoint.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index 90c708e5e7c4..8e7b09703ca4 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -80,7 +80,8 @@ static inline void hw_breakpoint_disable(void)
brk.address = 0;
brk.type = 0;
brk.len = 0;
- __set_breakpoint(&brk);
+ if (ppc_breakpoint_available())
+ __set_breakpoint(&brk);
}
extern void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs);
int hw_breakpoint_handler(struct die_args *args);
--
2.16.2
^ permalink raw reply related
* Re: [RESEND v2 1/4] libnvdimm: Add of_node to region and bus descriptors
From: Balbir Singh @ 2018-04-04 11:30 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linuxppc-dev, linux-nvdimm
In-Reply-To: <20180403142415.30083-1-oohall@gmail.com>
On Wed, 4 Apr 2018 00:24:12 +1000
Oliver O'Halloran <oohall@gmail.com> wrote:
> We want to be able to cross reference the region and bus devices
> with the device tree node that they were spawned from. libNVDIMM
> handles creating the actual devices for these internally, so we
> need to pass in a pointer to the relevant node in the descriptor.
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> Acked-by: Dan Williams <dan.j.williams@intel.com>
> ---
Acked-by: Balbir Singh <bsingharora@gmail.com>
^ permalink raw reply
* Re: [RESEND v2 3/4] doc/devicetree: Persistent memory region bindings
From: Balbir Singh @ 2018-04-04 12:07 UTC (permalink / raw)
To: Dan Williams
Cc: Oliver O'Halloran, Device Tree, linuxppc-dev, linux-nvdimm
In-Reply-To: <CAPcyv4jW5H8A96MFD7-UazNSZq-t1_NfrChNB3p9=somdBpsNw@mail.gmail.com>
On Tue, 3 Apr 2018 10:37:51 -0700
Dan Williams <dan.j.williams@intel.com> wrote:
> On Tue, Apr 3, 2018 at 7:24 AM, Oliver O'Halloran <oohall@gmail.com> wrote:
> > Add device-tree binding documentation for the nvdimm region driver.
> >
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> > ---
> > v2: Changed name from nvdimm-region to pmem-region.
> > Cleaned up the example binding and fixed the overlapping regions.
> > Added support for multiple regions in a single reg.
> > ---
> > .../devicetree/bindings/pmem/pmem-region.txt | 80 ++++++++++++++++++++++
> > MAINTAINERS | 1 +
> > 2 files changed, 81 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/pmem/pmem-region.txt
>
> Device-tree folks, does this look, ok?
>
> Oliver, is there any concept of a management interface to the
> device(s) backing these regions? libnvdimm calls these "nmem" devices
> and support operations like health status and namespace label
> management.
We would need a way to have nmem and pmem-regions find each other. Since we
don't have the ACPI abstractions, the nmem region would need to add the
ability for a driver to have a phandle to the interleaving and nmem properties.
I guess that would be a separate driver, that would manage the nmem devices
and there would be a way to relate the pmem and nmems. Oliver?
Balbir Singh.
^ permalink raw reply
* Re: [RESEND v2 4/4] powerpc/powernv: Create platform devs for nvdimm buses
From: Balbir Singh @ 2018-04-04 12:20 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linuxppc-dev, linux-nvdimm
In-Reply-To: <20180403142415.30083-4-oohall@gmail.com>
On Wed, 4 Apr 2018 00:24:15 +1000
Oliver O'Halloran <oohall@gmail.com> wrote:
> Scan the devicetree for an nvdimm-bus compatible and create
> a platform device for them.
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
Acked-by: Balbir Singh <bsingharora@gmail.com>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox