* Re: [RFC PATCH v2 1/1] of: introduce event tracepoints for dynamic device_node lifecyle
From: Frank Rowand @ 2018-01-25 6:48 UTC (permalink / raw)
To: Wolfram Sang, devicetree
Cc: Tyrel Datwyler, Geert Uytterhoeven, linux-renesas-soc,
linuxppc-dev, Rob Herring, Steven Rostedt, linux-kernel
In-Reply-To: <20180121143117.19805-2-wsa+renesas@sang-engineering.com>
On 01/21/18 06:31, Wolfram Sang wrote:
> From: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
>
> This patch introduces event tracepoints for tracking a device_nodes
> reference cycle as well as reconfig notifications generated in response
> to node/property manipulations.
>
> With the recent upstreaming of the refcount API several device_node
> underflows and leaks have come to my attention in the pseries (DLPAR)
> dynamic logical partitioning code (ie. POWER speak for hotplugging
> virtual and physcial resources at runtime such as cpus or IOAs). These
> tracepoints provide a easy and quick mechanism for validating the
> reference counting of device_nodes during their lifetime.
>
> Further, when pseries lpars are migrated to a different machine we
> perform a live update of our device tree to bring it into alignment with
> the configuration of the new machine. The of_reconfig_notify trace point
> provides a mechanism that can be turned for debuging the device tree
> modifications with out having to build a custom kernel to get at the
> DEBUG code introduced by commit 00aa37206e1a54 ("of/reconfig: Add debug
> output for OF_RECONFIG notifiers").
>
> The following trace events are provided: of_node_get, of_node_put,
> of_node_release, and of_reconfig_notify. These trace points require a
Please add a note that the of_reconfig_notify trace event is not an
added bit of debug info, but is instead replacing information that
was previously available via pr_debug() when DEBUG was defined.
> kernel built with ftrace support to be enabled. In a typical environment
> where debugfs is mounted at /sys/kernel/debug the entire set of
> tracepoints can be set with the following:
>
> echo "of:*" > /sys/kernel/debug/tracing/set_event
>
> or
>
> echo 1 > /sys/kernel/debug/tracing/events/of/enable
>
> The following shows the trace point data from a DLPAR remove of a cpu
> from a pseries lpar:
>
> cat /sys/kernel/debug/tracing/trace | grep "POWER8@10"
>
> cpuhp/23-147 [023] .... 128.324827:
> of_node_put: refcount=5, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147 [023] .... 128.324829:
> of_node_put: refcount=4, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147 [023] .... 128.324829:
> of_node_put: refcount=3, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147 [023] .... 128.324831:
> of_node_put: refcount=2, dn->full_name=/cpus/PowerPC,POWER8@10
> drmgr-7284 [009] .... 128.439000:
> of_node_put: refcount=1, dn->full_name=/cpus/PowerPC,POWER8@10
> drmgr-7284 [009] .... 128.439002:
> of_reconfig_notify: action=DETACH_NODE, dn->full_name=/cpus/PowerPC,POWER8@10,
> prop->name=null, old_prop->name=null
> drmgr-7284 [009] .... 128.439015:
> of_node_put: refcount=0, dn->full_name=/cpus/PowerPC,POWER8@10
> drmgr-7284 [009] .... 128.439016:
> of_node_release: dn->full_name=/cpus/PowerPC,POWER8@10, dn->_flags=4
>
> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
The following belongs in a list of version 2 changes, below the "---" line:
> [wsa: fixed commit abbrev and one of the sysfs paths in commit desc,
> removed trailing space and fixed pointer declaration in code]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> drivers/of/dynamic.c | 32 ++++++----------
> include/trace/events/of.h | 93 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 105 insertions(+), 20 deletions(-)
> create mode 100644 include/trace/events/of.h
mode looks incorrect. Existing files in include/trace/events/ are -rw-rw----
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index ab988d88704da0..b0d6ab5a35b8c6 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -21,6 +21,9 @@ static struct device_node *kobj_to_device_node(struct kobject *kobj)
> return container_of(kobj, struct device_node, kobj);
> }
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/of.h>
> +
> /**
> * of_node_get() - Increment refcount of a node
> * @node: Node to inc refcount, NULL is supported to simplify writing of
> @@ -30,8 +33,10 @@ static struct device_node *kobj_to_device_node(struct kobject *kobj)
> */
> struct device_node *of_node_get(struct device_node *node)
> {
> - if (node)
> + if (node) {
> kobject_get(&node->kobj);
> + trace_of_node_get(refcount_read(&node->kobj.kref.refcount), node->full_name);
See the comment from Ron that I mentioned in my previous email.
Also, the path has been removed from node->full_name. Does using it here
still give all of the information that is desired? Same for all others uses
of full_name in this patch.
The trace point should have a single argument, node. Accessing the two
fields can be done in the tracepoint assignment. Or is there some
reason that can't be done? Same for the trace_of_node_put() tracepoint.
> + }
> return node;
> }
> EXPORT_SYMBOL(of_node_get);
> @@ -43,8 +48,10 @@ EXPORT_SYMBOL(of_node_get);
> */
> void of_node_put(struct device_node *node)
> {
> - if (node)
> + if (node) {
> + trace_of_node_put(refcount_read(&node->kobj.kref.refcount) - 1, node->full_name);
If this is moved down one line to after kobject_put(), then no need
to subtract 1.
> kobject_put(&node->kobj);
> + }
> }
> EXPORT_SYMBOL(of_node_put);
>
> @@ -75,24 +82,7 @@ const char *action_names[] = {
> int of_reconfig_notify(unsigned long action, struct of_reconfig_data *p)
> {
> int rc;
> -#ifdef DEBUG
> - struct of_reconfig_data *pr = p;
> -
> - switch (action) {
> - case OF_RECONFIG_ATTACH_NODE:
> - case OF_RECONFIG_DETACH_NODE:
> - pr_debug("notify %-15s %pOF\n", action_names[action],
> - pr->dn);
> - break;
> - case OF_RECONFIG_ADD_PROPERTY:
> - case OF_RECONFIG_REMOVE_PROPERTY:
> - case OF_RECONFIG_UPDATE_PROPERTY:
> - pr_debug("notify %-15s %pOF:%s\n", action_names[action],
> - pr->dn, pr->prop->name);
> - break;
> -
> - }
> -#endif
> + trace_of_reconfig_notify(action, p);
> rc = blocking_notifier_call_chain(&of_reconfig_chain, action, p);
> return notifier_to_errno(rc);
> }
> @@ -320,6 +310,8 @@ void of_node_release(struct kobject *kobj)
> {
> struct device_node *node = kobj_to_device_node(kobj);
>
> + trace_of_node_release(node);
> +
> /* We should never be releasing nodes that haven't been detached. */
> if (!of_node_check_flag(node, OF_DETACHED)) {
> pr_err("ERROR: Bad of_node_put() on %pOF\n", node);
> diff --git a/include/trace/events/of.h b/include/trace/events/of.h
> new file mode 100644
> index 00000000000000..e8b1302a6f0129
> --- /dev/null
> +++ b/include/trace/events/of.h
> @@ -0,0 +1,93 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM of
> +
> +#if !defined(_TRACE_OF_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_OF_H
> +
> +#include <linux/of.h>
> +#include <linux/tracepoint.h>
> +
> +DECLARE_EVENT_CLASS(of_node_ref_template,
> +
> + TP_PROTO(int refcount, const char *dn_name),
> +
> + TP_ARGS(refcount, dn_name),
> +
> + TP_STRUCT__entry(
> + __string(dn_name, dn_name)
> + __field(int, refcount)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(dn_name, dn_name);
> + __entry->refcount = refcount;
> + ),
> +
> + TP_printk("refcount=%d, dn->full_name=%s",
For version 1 of the patch, the example trace output was reformatted
by hand because the lines were so long. it seems that it is easy to
create long lines and trace output, so extra effort on keeping the
lines shorter is well spent.
A little verbose. I would prefer to get rid of "refcount=", ",",
and "dn->full_name=" entirely.
> + __entry->refcount, __get_str(dn_name))
> +);
> +
> +DEFINE_EVENT(of_node_ref_template, of_node_get,
> + TP_PROTO(int refcount, const char *dn_name),
> + TP_ARGS(refcount, dn_name));
> +
> +DEFINE_EVENT(of_node_ref_template, of_node_put,
> + TP_PROTO(int refcount, const char *dn_name),
> + TP_ARGS(refcount, dn_name));
> +
> +TRACE_EVENT(of_node_release,
> +
> + TP_PROTO(struct device_node *dn),
> +
> + TP_ARGS(dn),
> +
> + TP_STRUCT__entry(
> + __string(dn_name, dn->full_name)
> + __field(unsigned long, flags)
Does not follow the alignment conventions for files in this directory.
Steve: have you dropped the conventions or are they still relevant?
> + ),
> +
> + TP_fast_assign(
> + __assign_str(dn_name, dn->full_name);
> + __entry->flags = dn->_flags;
> + ),
> +
> + TP_printk("dn->full_name=%s, dn->_flags=%lu",
> + __get_str(dn_name), __entry->flags)
That the first field is the node name should be obvious to the
person reading the trace, so ditch "dn->full_name=". One
could argue that _flags is not so obvious, but if someone
is looking at why of_node_release() is being called, they
will either care about flag, in which case it will be obvious
that the flag value is being printed, or they won't care
about the flag value and will be ignoring it anyway. So
I would ditch "dn->-flags=" also.
> +);
> +
> +#define of_reconfig_action_names \
> + {OF_RECONFIG_ATTACH_NODE, "ATTACH_NODE"}, \
> + {OF_RECONFIG_DETACH_NODE, "DETACH_NODE"}, \
> + {OF_RECONFIG_ADD_PROPERTY, "ADD_PROPERTY"}, \
> + {OF_RECONFIG_REMOVE_PROPERTY, "REMOVE_PROPERTY"}, \
> + {OF_RECONFIG_UPDATE_PROPERTY, "UPDATE_PROPERTY"}
> +
> +TRACE_EVENT(of_reconfig_notify,
> +
> + TP_PROTO(unsigned long action, struct of_reconfig_data *ord),
> +
> + TP_ARGS(action, ord),
> +
> + TP_STRUCT__entry(
> + __field(unsigned long, action)
> + __string(dn_name, ord->dn->full_name)
> + __string(prop_name, ord->prop ? ord->prop->name : "null")
> + __string(oldprop_name, ord->old_prop ? ord->old_prop->name : "null")
Does not follow the alignment conventions for files in this directory.
> + ),
> +
> + TP_fast_assign(
> + __entry->action = action;
> + __assign_str(dn_name, ord->dn->full_name);
> + __assign_str(prop_name, ord->prop ? ord->prop->name : "null");
> + __assign_str(oldprop_name, ord->old_prop ? ord->old_prop->name : "null");
> + ),
> +
> + TP_printk("action=%s, dn->full_name=%s, prop->name=%s, old_prop->name=%s",
Isn't old_prop->name the same as prop->name, if old_prop exists? If so, drop
it.
This is more verbose than the pr_debug() that is being replaced. Please make
it more compact.
> + __print_symbolic(__entry->action, of_reconfig_action_names),
> + __get_str(dn_name), __get_str(prop_name), __get_str(oldprop_name))
> +);
> +
> +#endif /* _TRACE_OF_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
>
^ permalink raw reply
* Re: [RFC PATCH v2 0/1] of: easier debugging for node life cycle issues
From: Frank Rowand @ 2018-01-25 6:48 UTC (permalink / raw)
To: Wolfram Sang, devicetree
Cc: Tyrel Datwyler, Geert Uytterhoeven, linux-renesas-soc,
linuxppc-dev, Rob Herring, Steven Rostedt, linux-kernel
In-Reply-To: <20180121143117.19805-1-wsa+renesas@sang-engineering.com>
On 01/21/18 06:31, Wolfram Sang wrote:
> I got a bug report for a DT node refcounting problem in the I2C subsystem. This
> patch was a huge help in validating the bug report and the proposed solution.
> So, I thought I bring it to attention again. Thanks Tyrel, for the initial
> work!
>
> Note that I did not test the dynamic updates, only of_node_{get|put} so far. I
> read that Tyrel checked dynamic updates extensively with this patch. And since
> DT overlays are also used within our Renesas dev team, this will help there, as
> well.
It's been nine months since version 1. If you are going to include the
dynamic updates part of the patch then please test them.
> Tested on a Renesas Salvator-XS board (R-Car H3).
>
> Changes since RFC v1:
> * rebased to v4.15-rc8
> * fixed commit abbrev and one of the sysfs paths in commit desc
> * removed trailing space and fixed pointer declaration in code
>
> I consider all the remaining checkpatch issues irrelevant for this patch.
I am OK with the line length warnings in this patch.
Why can't the macro error be fixed?
A file entry needs to be added to MAINTAINERS.
>
> So what about applying it?
>
> Kind regards,
>
> Wolfram
>
>
> Tyrel Datwyler (1):
> of: introduce event tracepoints for dynamic device_node lifecyle
>
> drivers/of/dynamic.c | 32 ++++++----------
> include/trace/events/of.h | 93 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 105 insertions(+), 20 deletions(-)
> create mode 100644 include/trace/events/of.h
>
^ permalink raw reply
* Re: [RFC PATCH v2 0/1] of: easier debugging for node life cycle issues
From: Frank Rowand @ 2018-01-25 6:47 UTC (permalink / raw)
To: Wolfram Sang
Cc: Wolfram Sang, devicetree, Tyrel Datwyler, Geert Uytterhoeven,
linux-renesas-soc, linuxppc-dev, Rob Herring, Steven Rostedt,
linux-kernel
In-Reply-To: <20180122114948.mm5mg6zqw3hmjj4o@katana>
On 01/22/18 03:49, Wolfram Sang wrote:
> Hi Frank,
>
>> Please go back and read the thread for version 1. Simply resubmitting a
>> forward port is ignoring that whole conversation.
>>
>> There is a lot of good info in that thread. I certainly learned stuff in it.
>
> Yes, I did that and learned stuff, too. My summary of the discussion was:
>
> - you mentioned some drawbacks you saw (like the mixture of trace output
> and printk output)> - most of them look like addressed to me? (e.g. Steven showed a way to redirect
> printk to trace
> - you posted your version (which was, however, marked as "not user friendly"
> even by yourself)
Not exactly a fair quoting. There were two things I said:
"Here is a patch that I have used. It is not as user friendly in terms
of human readable stack traces (though a very small user space program
should be able to fix that)."
So easy to fix using existing userspace programs to convert kernel
addresses to symbols.
"FIXME: Currently using pr_err() so I don't need to set loglevel on boot.
So obviously not a user friendly tool!!!
The process is:
- apply patch
- configure, build, boot kernel
- analyze data
- remove patch"
So not friendly because it uses pr_err() instead of pr_debug(). In
a reply I said if I submitted my patches I would change it to use
pr_debug() instead. So not an issue.
And not user friendly because it requires patching the kernel.
Again a NOP if I submitted my patch, because the patch would
already be in the kernel.
But whatever, let's ignore that - a poor quoting is not a reason to
reject this version of the patch.
> - The discussion stalled over having two approaches
Then you should have stated such when you resubmitted.
> So, I thought reposting would be a good way of finding out if your
> concerns were addressed in the discussion or not. If I overlooked
Then you should have stated that there were concerns raised in the
discussion and asked me if my concerns were addressed.
> something, I am sorry for that. Still, my intention is to continue the
> discussion, not to ignore it. Because as it stands, we don't have such a
> debugging mechanism in place currently, and with people working with DT
> overlays, I'd think it would be nice to have.
>
> Kind regards,
>
> Wolfram
>
Rob suggested:
>
> @@ -25,8 +28,10 @@
> */
> struct device_node *of_node_get(struct device_node *node)
> {
> - if (node)
> + if (node) {
> kobject_get(&node->kobj);
> + trace_of_node_get(refcount_read(&node->kobj.kref.refcount), node->full_name);
Seems like there should be a kobj wrapper to read the refcount.
As far as I noticed, that was never addressed. I don't know the answer, but
the question was asked. And if there is no such function, then there is at
least kref_read(), which would improve the code a little bit.
I'll reply to the patch 0/1 and patch 1/1 emails with review comments.
-Frank
^ permalink raw reply
* Re: [PATCH] powerpc: pseries: use irq_of_parse_and_map helper
From: Michael Ellerman @ 2018-01-25 6:08 UTC (permalink / raw)
To: Rob Herring
Cc: linux-kernel@vger.kernel.org, Benjamin Herrenschmidt,
Paul Mackerras, linuxppc-dev
In-Reply-To: <CAL_JsqLS_7TiQDwAxrDnP=v+63ge0+2SaomDgXQDSfOvwh7XiQ@mail.gmail.com>
Rob Herring <robh@kernel.org> writes:
> On Tue, Jan 23, 2018 at 12:53 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Rob Herring <robh@kernel.org> writes:
>>
>>> Instead of calling both of_irq_parse_one and irq_create_of_mapping, call
>>> of_irq_parse_and_map instead which does the same thing. This gets us closer
>>> to making the former 2 functions static.
...
>> Are you trying to remove the low-level routines or is this just a
>> cleanup?
>
> The former, but I'm not sure that will happen. There's a handful of
> others left, but they aren't simply a call to of_irq_parse_one and
> then irq_create_of_mapping.
>
>> The patch below works, it loses the error handling if the interrupts
>> property is corrupt/empty, but that's probably overly paranoid anyway.
>
> Not quite. Previously, it was silent if parsing failed. Only the
> mapping would give an error which would mean the interrupt parent had
> some error.
>
> Actually, we could use of_irq_get here to preserve the error handling.
> It will return error codes from parsing, 0 on mapping failure, or the
> Linux irq number. It adds an irq_find_host call for deferred probe,
> but that should be harmless. I'll respin it.
OK thanks.
cheers
^ permalink raw reply
* [RFC PATCH] powerpc/powernv: Provide a way to force a core into SMT4 mode
From: Paul Mackerras @ 2018-01-25 5:05 UTC (permalink / raw)
To: linuxppc-dev
POWER9 processors up to and including "Nimbus" v2.2 have hardware
bugs relating to transactional memory and thread reconfiguration.
One of these bugs has a workaround which is to get the core into
SMT4 state temporarily. This workaround is only needed when
running bare-metal.
This patch provides a function which gets the core into SMT4 mode
by preventing threads from going to a stop state, and waking up
those which are already in a stop state. Once at least 3 threads
are not in a stop state, the core will be in SMT4 and we can
continue.
To do this, we add a "dont_stop" flag to the paca to tell the
thread not to go into a stop state. If this flag is set,
power9_idle_stop() just returns immediately with a return value
of 0. The pnv_power9_force_smt4() function does the following:
1. Set the dont_stop flag for each thread in the core, except
ourselves (in fact we use an atomic_inc() in case more than
one thread is calling this function concurrently).
2. See how many threads are awake, indicated by their
requested_psscr field in the paca being 0. If this is at
least 3, skip to step 5.
3. Send a doorbell interrupt to each thread that was seen as
being in a stop state in step 2.
4. Until at least 3 threads are awake, scan the threads to which
we sent a doorbell interrupt and check if they are awake now.
5. Clear (actually atomic_dec()) the dont_stop flag for each
thread in the core, except for ourselves.
This relies on the following properties:
- Once dont_stop is non-zero, requested_psccr can't go from zero to
non-zero, except transiently (and without the thread doing stop).
- requested_psscr being zero guarantees that the thread isn't in
a state-losing stop state where thread reconfiguration could occur.
- Doing stop with a PSSCR value of 0 won't be a state-losing stop
and thus won't allow thread reconfiguration.
This does add a sync to power9_idle_stop(), which is necessary to
provide the correct ordering between setting requested_psscr and
checking dont_stop. The overhead of the sync should be unnoticeable
compared to the latency of going into and out of a stop state.
Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
---
arch/powerpc/include/asm/paca.h | 3 ++
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/kernel/idle_book3s.S | 15 +++++++++
arch/powerpc/platforms/powernv/idle.c | 62 +++++++++++++++++++++++++++++++++++
4 files changed, 81 insertions(+)
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index 23ac7fc..71b5c34 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -32,6 +32,7 @@
#include <asm/accounting.h>
#include <asm/hmi.h>
#include <asm/cpuidle.h>
+#include <asm/atomic.h>
register struct paca_struct *local_paca asm("r13");
@@ -177,6 +178,8 @@ struct paca_struct {
u8 thread_mask;
/* Mask to denote subcore sibling threads */
u8 subcore_sibling_mask;
+ /* Flag to request this thread not to stop */
+ atomic_t dont_stop;
/*
* Pointer to an array which contains pointer
* to the sibling threads' paca.
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index ff6ce2f..91cb8df 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -758,6 +758,7 @@ int main(void)
OFFSET(PACA_SUBCORE_SIBLING_MASK, paca_struct, subcore_sibling_mask);
OFFSET(PACA_SIBLING_PACA_PTRS, paca_struct, thread_sibling_pacas);
OFFSET(PACA_REQ_PSSCR, paca_struct, requested_psscr);
+ OFFSET(PACA_DONT_STOP, paca_struct, dont_stop);
#define STOP_SPR(x, f) OFFSET(x, paca_struct, stop_sprs.f)
STOP_SPR(STOP_PID, pid);
STOP_SPR(STOP_LDBAR, ldbar);
diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
index 01e1c19..4a7f88c 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -430,10 +430,23 @@ ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_ARCH_207S, 66); \
*/
_GLOBAL(power9_idle_stop)
std r3, PACA_REQ_PSSCR(r13)
+ sync
+ lwz r5, PACA_DONT_STOP(r13)
+ cmpwi r5, 0
+ bne 1f
mtspr SPRN_PSSCR,r3
LOAD_REG_ADDR(r4,power_enter_stop)
b pnv_powersave_common
/* No return */
+1:
+ /*
+ * We get here when TM / thread reconfiguration bug workaround
+ * code wants to get the CPU into SMT4 mode, and therefore
+ * we are being asked not to stop.
+ */
+ li r3, 0
+ std r3, PACA_REQ_PSSCR(r13)
+ blr /* return 0 for wakeup cause / SRR1 value */
/*
* On waking up from stop 0,1,2 with ESL=1 on POWER9 DD1,
@@ -584,6 +597,8 @@ FTR_SECTION_ELSE_NESTED(71)
mfspr r5, SPRN_PSSCR
rldicl r5,r5,4,60
ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_POWER9_DD1, 71)
+ li r0, 0 /* clear requested_psscr to say we're awake */
+ std r0, PACA_REQ_PSSCR(r13)
cmpd cr4,r5,r4
bge cr4,pnv_wakeup_tb_loss /* returns to caller */
diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
index 443d5ca..72d5a85 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -24,6 +24,7 @@
#include <asm/code-patching.h>
#include <asm/smp.h>
#include <asm/runlatch.h>
+#include <asm/dbell.h>
#include "powernv.h"
#include "subcore.h"
@@ -387,6 +388,67 @@ void power9_idle(void)
power9_idle_type(pnv_default_stop_val, pnv_default_stop_mask);
}
+#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
+/*
+ * This is used in working around bugs in thread reconfiguration
+ * on POWER9 (at least up to Nimbus DD2.2) relating to transactional
+ * memory and the way that XER[SO] is checkpointed.
+ * This function forces the core into SMT4 in order by asking
+ * all other threads not to stop, and sending a message to any
+ * that are in a stop state.
+ * Must be called with preemption disabled.
+ */
+void pnv_power9_force_smt4(void)
+{
+ int cpu, cpu0, thr;
+ struct paca_struct *tpaca;
+ int awake_threads = 1; /* this thread is awake */
+ int poke_threads = 0;
+
+ cpu = smp_processor_id();
+ cpu0 = cpu & ~(threads_per_core - 1);
+ tpaca = &paca[cpu0];
+ for (thr = 0; thr < threads_per_core; ++thr) {
+ if (cpu != cpu0 + thr)
+ atomic_inc(&tpaca[thr].dont_stop);
+ }
+ /* order setting dont_stop vs testing requested_psscr */
+ mb();
+ for (thr = 0; thr < threads_per_core; ++thr) {
+ if (!tpaca[thr].requested_psscr)
+ ++awake_threads;
+ else
+ poke_threads |= (1 << thr);
+ }
+
+ /* If at least 3 threads are awake, the core is in SMT4 already */
+ if (awake_threads < threads_per_core - 1) {
+ /* We have to wake some threads; we'll use msgsnd */
+ for (thr = 0; thr < threads_per_core; ++thr) {
+ if (poke_threads & (1 << thr))
+ ppc_msgsnd(PPC_DBELL_MSGTYPE, 0,
+ tpaca[thr].hw_cpu_id);
+ }
+ /* now spin until at least 3 threads are awake */
+ do {
+ for (thr = 0; thr < threads_per_core; ++thr) {
+ if ((poke_threads & (1 << thr)) &&
+ !tpaca[thr].requested_psscr) {
+ ++awake_threads;
+ poke_threads &= ~(1 << thr);
+ }
+ }
+ } while (awake_threads < threads_per_core - 1);
+ }
+ /* clear all the dont_stop flags */
+ for (thr = 0; thr < threads_per_core; ++thr) {
+ if (cpu != cpu0 + thr)
+ atomic_dec(&tpaca[thr].dont_stop);
+ }
+}
+EXPORT_SYMBOL_GPL(pnv_power9_force_smt4);
+#endif /* CONFIG_KVM_BOOK3S_HV_POSSIBLE */
+
#ifdef CONFIG_HOTPLUG_CPU
static void pnv_program_cpu_hotplug_lpcr(unsigned int cpu, u64 lpcr_val)
{
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 5/5] powerpc/ftw: Document FTW API/usage
From: Sukadev Bhattiprolu @ 2018-01-25 3:05 UTC (permalink / raw)
To: Randy Dunlap
Cc: Michael Ellerman, Benjamin Herrenschmidt, mikey, hbabu,
linuxppc-dev, linux-kernel
In-Reply-To: <57a1be39-df84-d678-7560-e2543824472d@infradead.org>
Randy Dunlap [rdunlap@infradead.org] wrote:
> > + struct ftw_setup_attr ftwattr;
> > +
> > + fd = open("/dev/ftw", O_RDWR);
> > +
> > + memset(&rxattr, 0, sizeof(rxattr));
>
> Is that supposed to be ftwattr (2x above)?
Yes. I agree with your other comments as well and will send a new version.
Thanks for the detailed review.
Sukadev
^ permalink raw reply
* Re: [PATCH 5/5] powerpc/ftw: Document FTW API/usage
From: Randy Dunlap @ 2018-01-25 0:48 UTC (permalink / raw)
To: Sukadev Bhattiprolu, Michael Ellerman
Cc: Benjamin Herrenschmidt, mikey, hbabu, linuxppc-dev, linux-kernel
In-Reply-To: <1516157443-17716-6-git-send-email-sukadev@linux.vnet.ibm.com>
On 01/16/2018 06:50 PM, Sukadev Bhattiprolu wrote:
> Document the usage of the VAS Fast thread-wakeup API and add an entry in
> MAINTAINERS file.
>
> Thanks for input/comments from Benjamin Herrenschmidt, Michael Neuling,
> Michael Ellerman, Robert Blackmore, Ian Munsie, Haren Myneni and Paul
> Mackerras.
>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> ---
>
> Changelog[v2]
> - [Michael Neuling] Update API to use a single, VAS_FTW_SEUTP ioctl
> rather than two ioctls.
> - [Michael Neuling] Drop "nx" from name "nx-ftw".
>
> ---
> Documentation/powerpc/ftw-api.txt | 283 ++++++++++++++++++++++++++++++++++++++
> MAINTAINERS | 8 ++
> 2 files changed, 291 insertions(+)
> create mode 100644 Documentation/powerpc/ftw-api.txt
>
> diff --git a/Documentation/powerpc/ftw-api.txt b/Documentation/powerpc/ftw-api.txt
> new file mode 100644
> index 0000000..a107628
> --- /dev/null
> +++ b/Documentation/powerpc/ftw-api.txt
> @@ -0,0 +1,283 @@
> +Virtual Accelerator Switchboard and Fast Thread-Wakeup API
> +
...
> +
> + Application access to the FTW mechanism is provided through the FTW
> + device node (/dev/ftw) implemented by the FTW device driver.
> +
> + A multi-threaded software processes that intends to use the FTW
process
> + mechanism must first setup a channel (consisting of a pair of VAS
> + windows) for the waiting and waking threads to communicate. The
> + channel is set up by opening the FTW device and issuing the FTW_SETUP
> + ioctl. Upon successful return from the ioctl, the waiting side of
> + channel is complete and a thread can issue the "Wait" instruction
> + to wait for an event.
> +
> + After the successful return from the FTW_SETUP ioctl, the waking
> + thread must use mmap() system call on the same file descriptor and
> + obtain a virtual address known as the "paste address".
> +
> + Once the mmap() call succeeds the setup of "waking" side of the channel
> + is complete. To wake up a waiting thread, the waking thread should use
> + the "COPY" and "PASTE" instructions to write a zero-filled CRB to the
> + paste-address.
> +
> + The wait and wake up operations can be repeated as long as the paste
> + address and the FTW file descriptor are valid (i.e until munmap() of
> + the paste address or a close() of the FTW fd).
> +
> +1. FTW Device Node
> +
> + There is one /dev/ftw node in the system and it provides access to the
> + VAS/FTW functionality.
> +
> + The only valid operations (system calls) on the FTW node are:
> +
> + - open() the device for read and write.
> +
> + - issue the FTW_SETUP ioctl to set up a channel.
> +
> + - mmap() the file descriptor
> +
> + - close the device node.
> +
> + Other file operations on the FTW node are undefined.
> +
> + Note that the COPY and PASTE operations go directly to the hardware
> + and do not involve system calls or go through the FTW device.
> +
> + Although a system may have several instances of the VAS in the system
> + (typically, one per P9 chip) there is just one FTW device node in
> + the system.
> +
> + When the FTW device node is opened, the kernel assigns a suitable
> + instance of VAS to the process. Kernel will make a best-effort attempt
> + to assign an optimal instance of VAS for the process - based on the CPU/
> + chip that the process is running on. In the initial release, the kernel
> + does not support migrating the VAS instance if the process migrates from
> + a CPU on one chip to a CPU on another chip.
> +
> + Applications may chose a specific instance of the VAS using the 'vas_id'
choose
> + field in the FTW_SETUP ioctl as detailed below.
> +
> +2. Open FTW node
> +
> + The device should be opened for read and write. No special privileges
> + are needed to open the device. The device may be opened multiple times.
> +
> + Each open() of the FTW device is associated with one channel of
> + communication. There is a system-wide limit (currently 64K windows per
> + chip and since some are reserved for hardware, there are about 32K
> + channels per chip). If no more channels are available, the open() system
> + call will fail.
> +
> + See open(2) system call man pages for other details such as return
> + values, error codes and restrictions.
> +
> +3. Setup a communication channel (FTW_SETUP ioctl)
> +
> + A process that intends to use the Fast Thread-wakeup mechanism must
> + first setup a channel by issuing the FTW_SETUP ioctl.
> +
> + #include <misc/ftw.h>
> +
> + struct ftw_setup_attr ftwattr;
> +
> + rc = ioctl(fd, FTW_SETUP, &ftwattr);
> +
> + The attributes of ftwattr are as follows:
> +
> + struct ftw_setup_attr {
> + int16_t version;
> + int16_t vas_id;
> + uint32_t reserved;
> +
> + int64_t reserved1;
> + int64_t flags;
> + int64_t reserved2;
> + };
> +
> + The version field identifies the version of the API and must currently
> + be set to 1.
> +
> + The vas_id field identifies a specific instance of the VAS that the
> + application wishes to access. See section on VAS ID below.
> +
> + The reserved fields must all be set to zeroes.
> +
> + The flags field specifies additional attributes to the channel. The
> + only valid bit in the flags for Fast thread-wakeup usage are:
> +
> + FTW_FLAGS_PIN_WINDOW if set, indicates that the channel should be
> + pinned in cache. This flag is restricted
> + to privileged users. See Pinning windows
> + below.
> +
> + All the other bits in the flags field must be set to 0.
> +
> + Return value:
> +
> + The FTW_SETUP ioctl returns 0 on success. On error, it returns -1
> + and sets the errno variable to indicate the error.
> +
> + Error codes:
> +
> + EINVAL version is invalid
> +
> + EINVAL vas_id is invalid
> +
> + EINVAL fd does not refer to a valid VAS device.
> +
> + ENOSPC System has too many active channels (windows) open,
s/,/./ or s/,//
> +
> + EPERM FTW_FLAGS_PIN_WINDOW is set in 'flags' field and process
> + is not privileged.
> +
> + EINVAL reserved fields are not set to 0.
> +
> + See the ioctl(2) man page for more details, error codes and restrictions.
> +
> +4. mmap() FTW device fd
> +
> + The mmap() system call for a FTW device fd returns a "paste address"
> + that the application can use to COPY/PASTE a CRB to the waiting thread.
> +
> + paste_addr = mmap(NULL, size, prot, flags, fd, offset);
> +
> + Only restrictions on mmap for a FTW device fd are:
> +
> + - size parameter should be one page size
> +
> + - offset parameter should be 0ULL.
> +
> + Refer to mmap(2) man page for additional details/restrictions.
> +
> + In addition to the error conditions listed on the mmap(2) man page,
> + mmap() can also fail with one of following error codes:
> +
> + EINVAL fd is not associated with an open channel (window)
> + (i.e mmap() does not follow a successful call to the
> + FTW_SETUP ioctl).
> +
> + EINVAL offset field is not 0ULL.
> +
> +
> +5. VAS ID
...
> +
> +6. COPY/PASTE operations:
> +
> + Applications should use the COPY and PASTE instructions defined in
> + the RFC to copy/paste the CRB. For VAS/FTW usage, the contents of
> + CRB, are ignored and can be zero, but CRB should point to a valid buffer
CRB are ignored buffer.
> +
> +7. Interrupt completion and signal handling
> +
> + No VAS-specific signals will be generated to the application threads
> + with the VAS/FTW usage.
> +
> +8. Example/Proposed usage of the VAS/FTW API
> +
> + In the following example we use two threads that use the VAS/FTW API.
> + Thread T1 sets up the channel and uses the WAIT instruction to wait for
> + an event. Thread T2 uses copy/paste instructions to wake up T1.
> + Note that the pthread_cond_wait() calls must be in a loop for spurious
> + wake ups, but are simplified here.
> +
> + Common interfaces:
> +
> + static bool paste_done;
> +
> + #define WAIT .long (0x7C00003C)
> +
> + static inline int do_wait(void)
> + {
> + __asm__ __volatile(stringify_in_c(WAIT)";");
> + }
> +
> + /*
> + * Check if paste_done is true
> + */
> + static bool is_paste_done(void)
> + {
> + return __sync_bool_compare_and_swap(&paste_done, 1, 0);
> +
> + }
> +
> + /*
> + * Set paste_done to true
> + */
> + static inline void set_paste_done(void)
> + {
> + __sync_bool_compare_and_swap(&paste_done, 0, 1);
> + }
> +
> +
> + int fd = -1; // global, visible to both T1 and T2
> +
> + Thread T1:
> +
> + struct ftw_setup_attr ftwattr;
> +
> + fd = open("/dev/ftw", O_RDWR);
> +
> + memset(&rxattr, 0, sizeof(rxattr));
Is that supposed to be ftwattr (2x above)?
> + ftwattr.version = 1;
> + ftwattr.vas_id = -1;
> +
> + rc = ioctl(fd, FTW_SETUP, &ftwattr);
> +
> + /* Tell T2 that waiter side of channel is ready */
> + pthread_cond_signal(&rx_win_ready);
> +
> + /* Rx set up done */
> +
> + /* later, wait for an event to occur */
> +
> + while(!is_paste_done())
> + do_wait();
> +
> + Thread T2:
> +
> + /* Wait for waiter side of channel to be set up first */
> + pthread_cond_wait(&rx_win_ready);
> +
> + prot = PROT_READ|PROT_WRITE;
> + paste_addr = mmap(NULL, 4096, prot, MAP_SHARED, fd, 0ULL);
> +
> + /* Tx setup done */
> +
> + /* later ... */
> +
> + set_paste_done(); /* ... event occurred */
> + write_empty_crb(paste_addr); /* wake up T1 */
--
~Randy
^ permalink raw reply
* Re: Are those hacks still valid on powerpc kernel ?
From: Benjamin Herrenschmidt @ 2018-01-24 23:49 UTC (permalink / raw)
To: Christophe LEROY, Aneesh Kumar K.V, Michael Ellerman,
Anton Blanchard
Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <c0b2f3ad-3247-1b80-8789-e120c3ebd8bc@c-s.fr>
On Wed, 2018-01-24 at 11:17 +0100, Christophe LEROY wrote:
> Below comments are very old.
>
> Aren't new glibc and binutils now able to go without this ?
>
> Note that the code inside the #if 0 is wrong as we have no vma defined
> in the function.
>
> Or does it just have no performance impact anyway ?
>
>
> From /arch/powerpc/mm/mem.c:
>
> void clear_user_page(void *page, unsigned long vaddr, struct page *pg)
> {
> clear_page(page);
>
> /*
> * We shouldn't have to do this, but some versions of glibc
> * require it (ld.so assumes zero filled pages are icache clean)
> * - Anton
> */
> flush_dcache_page(pg);
> }
> EXPORT_SYMBOL(clear_user_page);
Well, I think it would be a security issue to potentially leave garbage
icache content (possibly instructions from another process) accessible
to userspace. So I don't think we can avoid that one.
> void copy_user_page(void *vto, void *vfrom, unsigned long vaddr,
> struct page *pg)
> {
> copy_page(vto, vfrom);
>
> /*
> * We should be able to use the following optimisation, however
> * there are two problems.
> * Firstly a bug in some versions of binutils meant PLT sections
> * were not marked executable.
> * Secondly the first word in the GOT section is blrl, used
> * to establish the GOT address. Until recently the GOT was
> * not marked executable.
> * - Anton
> */
> #if 0
> if (!vma->vm_file && ((vma->vm_flags & VM_EXEC) == 0))
> return;
> #endif
Well, we try not to break userspace.... This doesn't affect newer CPUs
that much because they have CPU_FTR_COHERENT_ICACHE, so
flush_dcache_page is pretty much a nop on them.
Cheers,
Ben.
> flush_dcache_page(pg);
> }
>
> Christophe
^ permalink raw reply
* [PATCH] powerpc: dts: use 'atmel' as at24 manufacturer for kmcent2
From: Bartosz Golaszewski @ 2018-01-24 21:38 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Russell King, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman, Valentin Longchamp,
Holger Brunck, David S . Miller, Scott Wood
Cc: devicetree, linux-kernel, linuxppc-dev, Bartosz Golaszewski
Using compatible strings without the <manufacturer> part for at24 is
now deprecated. Use a correct 'atmel,<model>' value.
Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
arch/powerpc/boot/dts/fsl/kmcent2.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/boot/dts/fsl/kmcent2.dts b/arch/powerpc/boot/dts/fsl/kmcent2.dts
index 5922c1ea0e96..3094df05f5ea 100644
--- a/arch/powerpc/boot/dts/fsl/kmcent2.dts
+++ b/arch/powerpc/boot/dts/fsl/kmcent2.dts
@@ -130,7 +130,7 @@
#size-cells = <0>;
eeprom@54 {
- compatible = "24c02";
+ compatible = "atmel,24c02";
reg = <0x54>;
pagesize = <2>;
read-only;
--
2.16.1
^ permalink raw reply related
* [PATCH] powerpc: dts: use a correct at24 compatible fallback in ac14xx
From: Bartosz Golaszewski @ 2018-01-24 21:36 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Russell King, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman
Cc: devicetree, linux-kernel, linuxppc-dev, Bartosz Golaszewski
Using 'at24' as fallback is now deprecated - use the full
'atmel,<model>' string.
Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
arch/powerpc/boot/dts/ac14xx.dts | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/boot/dts/ac14xx.dts b/arch/powerpc/boot/dts/ac14xx.dts
index 83bcfd865167..0be5c4f3265d 100644
--- a/arch/powerpc/boot/dts/ac14xx.dts
+++ b/arch/powerpc/boot/dts/ac14xx.dts
@@ -176,12 +176,12 @@
clock-frequency = <400000>;
at24@30 {
- compatible = "at24,24c01";
+ compatible = "atmel,24c01";
reg = <0x30>;
};
at24@31 {
- compatible = "at24,24c01";
+ compatible = "atmel,24c01";
reg = <0x31>;
};
@@ -191,42 +191,42 @@
};
at24@50 {
- compatible = "at24,24c01";
+ compatible = "atmel,24c01";
reg = <0x50>;
};
at24@51 {
- compatible = "at24,24c01";
+ compatible = "atmel,24c01";
reg = <0x51>;
};
at24@52 {
- compatible = "at24,24c01";
+ compatible = "atmel,24c01";
reg = <0x52>;
};
at24@53 {
- compatible = "at24,24c01";
+ compatible = "atmel,24c01";
reg = <0x53>;
};
at24@54 {
- compatible = "at24,24c01";
+ compatible = "atmel,24c01";
reg = <0x54>;
};
at24@55 {
- compatible = "at24,24c01";
+ compatible = "atmel,24c01";
reg = <0x55>;
};
at24@56 {
- compatible = "at24,24c01";
+ compatible = "atmel,24c01";
reg = <0x56>;
};
at24@57 {
- compatible = "at24,24c01";
+ compatible = "atmel,24c01";
reg = <0x57>;
};
--
2.16.1
^ permalink raw reply related
* [PATCH] powerpc: dts: use 'atmel' as at24 anufacturer for pdm360ng
From: Bartosz Golaszewski @ 2018-01-24 21:36 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Russell King, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman
Cc: devicetree, linux-kernel, linuxppc-dev, Bartosz Golaszewski
Using 'at' as the <manufacturer> part of the compatible string is now
deprecated. Use a correct string: 'atmel,<model>'.
Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
arch/powerpc/boot/dts/pdm360ng.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/boot/dts/pdm360ng.dts b/arch/powerpc/boot/dts/pdm360ng.dts
index 445b88114009..df1283b63d9b 100644
--- a/arch/powerpc/boot/dts/pdm360ng.dts
+++ b/arch/powerpc/boot/dts/pdm360ng.dts
@@ -98,7 +98,7 @@
fsl,preserve-clocking;
eeprom@50 {
- compatible = "at,24c01";
+ compatible = "atmel,24c01";
reg = <0x50>;
};
--
2.16.1
^ permalink raw reply related
* Re: [PATCH v4 3/7] platforms/pseries: Set eeh_pe of EEH_PE_VF type
From: Bryant G. Ly @ 2018-01-24 14:53 UTC (permalink / raw)
To: Michael Ellerman, benh, paulus
Cc: seroyer, jjalvare, alex.williamson, helgaas, aik, ruscur,
linux-pci, linuxppc-dev, bodong, eli, saeedm
In-Reply-To: <87shawukgv.fsf@concordia.ellerman.id.au>
On 1/23/18 7:14 PM, Michael Ellerman wrote:
> "Bryant G. Ly" <bryantly@linux.vnet.ibm.com> writes:
>
>> To correctly use EEH code one has to make
>> sure that the EEH_PE_VF is set for dynamic created
>> VFs. Therefore this patch allocates an eeh_pe of
>> eeh type EEH_PE_VF and associates PE with parent.
>>
>> Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
>> Signed-off-by: Juan J. Alvarez <jjalvare@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/include/asm/pci-bridge.h | 5 ++++-
>> arch/powerpc/platforms/pseries/eeh_pseries.c | 17 +++++++++++++++++
>> 2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
>> index 9f66ddebb799..16d70740a76f 100644
>> --- a/arch/powerpc/include/asm/pci-bridge.h
>> +++ b/arch/powerpc/include/asm/pci-bridge.h
>> @@ -211,7 +211,10 @@ struct pci_dn {
>> unsigned int *pe_num_map; /* PE# for the first VF PE or array */
>> bool m64_single_mode; /* Use M64 BAR in Single Mode */
>> #define IODA_INVALID_M64 (-1)
>> - int (*m64_map)[PCI_SRIOV_NUM_BARS];
>> + union {
>> + int (*m64_map)[PCI_SRIOV_NUM_BARS]; /*Only used in powernv */
>> + int last_allow_rc; /* Only used in pSeries */
>> + };
>> #endif /* CONFIG_PCI_IOV */
>> int mps; /* Maximum Payload Size */
>> struct list_head child_list;
> I don't see the point of using a union to save 4 bytes.
>
> And if you look at the current layout of the struct there's actually a 4
> byte hole after mps, so it doesn't actually save any space at all.
>
> I can remove it before applying, unless there's some compelling reason
> for it I'm not seeing.
>
> cheers
No specific reason for the union, you can go ahead and remove it before applying.
Thanks!
Bryant
^ permalink raw reply
* Re: [PATCH v3 5/5] powerpc/mm: Fix growth direction for hugepages mmaps with slice
From: Christophe LEROY @ 2018-01-24 10:19 UTC (permalink / raw)
To: Aneesh Kumar K.V, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Scott Wood
Cc: linux-kernel, linuxppc-dev
In-Reply-To: <c80d6037-25f9-0b3a-361d-3ddac8c9d7e0@linux.vnet.ibm.com>
Le 24/01/2018 à 11:08, Aneesh Kumar K.V a écrit :
>
>
> On 01/24/2018 03:33 PM, Christophe LEROY wrote:
>>
>>
>> Le 24/01/2018 à 10:51, Aneesh Kumar K.V a écrit :
>>>
>>>
>>> On 01/24/2018 03:09 PM, Christophe LEROY wrote:
>>>>
>>>>
>>>> Le 24/01/2018 à 10:35, Aneesh Kumar K.V a écrit :
>>>>>
>>>
>>>>>> Did you try with HUGETLB_MORECORE_HEAPBASE=0x11000000 on PPC64 as
>>>>>> I suggested in my last email on this subject (22/01/2018 9:22) ?
>>>>>
>>>>>
>>>>> yes. The test ran fine for me
>>>>
>>>> You tried with 0x30000000, it works as well on PPC32.
>>>>
>>>> I'd really like you to try with 0x11000000 which is in the same
>>>> slice as the 10020000-10030000 range.
>>>>
>>>>
>>>
>>> Now that explains is better. But then the requested HEAPBASE was not
>>> free and hence topdown search got an address in the below range.
>>>
>>> 7efffd000000-7f0000000000 rw-p 00000000 00:0d 1082770 /anon_hugepage
>>> (deleted)
>>>
>>>
>>> The new range allocated is such that there is no scope for expansion
>>> of heap if we do a topdown search. But why should that require us to
>>> change from topdown/bottomup search?
>>>
>>>
>>> 10000000-10010000 r-xp 00000000 fc:00 9044312 /home/kvaneesh/a.out
>>> 10010000-10020000 r--p 00000000 fc:00 9044312 /home/kvaneesh/a.out
>>> 10020000-10030000 rw-p 00010000 fc:00 9044312 /home/kvaneesh/a.out
>>> 7efffd000000-7f0000000000 rw-p 00000000 00:0d 1082770 /anon_hugepage
>>> (deleted)
>>> 7ffff2d40000-7ffff7d60000 rw-p 00000000 00:00 0
>>> 7ffff7d60000-7ffff7f10000 r-xp 00000000 fc:00 9250090
>>> /lib/powerpc64le-linux-gnu/libc-2.23.so
>>> 7ffff7f10000-7ffff7f20000 r--p 001a0000 fc:00 9250090
>>> /lib/powerpc64le-linux-gnu/libc-2.23.so
>>> 7ffff7f20000-7ffff7f30000 rw-p 001b0000 fc:00 9250090
>>> /lib/powerpc64le-linux-gnu/libc-2.23.so
>>> 7ffff7f40000-7ffff7f60000 r-xp 00000000 fc:00 10754812
>>> /usr/lib/libhugetlbfs.so.0
>>> 7ffff7f60000-7ffff7f70000 r--p 00010000 fc:00 10754812
>>> /usr/lib/libhugetlbfs.so.0
>>> 7ffff7f70000-7ffff7f80000 rw-p 00020000 fc:00 10754812
>>> /usr/lib/libhugetlbfs.so.0
>>> 7ffff7f80000-7ffff7fa0000 r-xp 00000000 00:00 0 [vdso]
>>> 7ffff7fa0000-7ffff7fe0000 r-xp 00000000 fc:00 9250107
>>> /lib/powerpc64le-linux-gnu/ld-2.23.so
>>> 7ffff7fe0000-7ffff7ff0000 r--p 00030000 fc:00 9250107
>>> /lib/powerpc64le-linux-gnu/ld-2.23.so
>>> 7ffff7ff0000-7ffff8000000 rw-p 00040000 fc:00 9250107
>>> /lib/powerpc64le-linux-gnu/ld-2.23.so
>>> 7ffffffd0000-800000000000 rw-p 00000000 00:00 0 [stack]
>>>
>>>
>>> For the specific test, one should pass the HEAPBASE value such that
>>> it can be expanded if required isn't it ?
>>
>> For the test, yes, it is dumb to pass an unusable HEAPBASE, but what
>> happens in real life:
>> * PPC32: No HEAPBASE, hugetlbfs defines a HEAPBASE at sbrk(0) +
>> PAGE_SIZE = 0x10800000 ==> This is in the same slice as already
>> allocated ==> the kernel does as if mmap() had been called with no
>> hint address and allocates something unusable instead.
>> * PPC64: No HEAPBASE, hugetlbfs seems to define a HEAPBASE at
>> 100000000000, which doesn't conflict with an already allocated mapping
>> ==> it works.
>>
>> Now, when we take the generic case, ie when slice is not activated,
>> when you call mmap() without a hint address, it allocates a suitable
>> address because it does bottom-up. Why do differently with slices ?
>>
>
> IIUC that is largely arch dependent, PPC64 always did topdown search.
> Even for regular non hugetlb mmap it did topdown search. If you set
> legacy mmap we selected bottom up approach. You can check
> arch_pick_mmap_layout() for more details. Now x86 is slightly different.
> For the default search if we can't find a mapping address it will try a
> bottomup search. Having said that if you think libhugetlbfs made
> assumptions with respect to 8xx and you don't want to break it make
> 8xx unmapped area search bottomup.
>
Or would there be a way to make libhugetlbfs aware of the slices
constraints and make it choose a suitable hint address at first try ?
Christophe
^ permalink raw reply
* Are those hacks still valid on powerpc kernel ?
From: Christophe LEROY @ 2018-01-24 10:17 UTC (permalink / raw)
To: Aneesh Kumar K.V, Michael Ellerman, Benjamin Herrenschmidt,
Anton Blanchard
Cc: linuxppc-dev@lists.ozlabs.org
Below comments are very old.
Aren't new glibc and binutils now able to go without this ?
Note that the code inside the #if 0 is wrong as we have no vma defined
in the function.
Or does it just have no performance impact anyway ?
From /arch/powerpc/mm/mem.c:
void clear_user_page(void *page, unsigned long vaddr, struct page *pg)
{
clear_page(page);
/*
* We shouldn't have to do this, but some versions of glibc
* require it (ld.so assumes zero filled pages are icache clean)
* - Anton
*/
flush_dcache_page(pg);
}
EXPORT_SYMBOL(clear_user_page);
void copy_user_page(void *vto, void *vfrom, unsigned long vaddr,
struct page *pg)
{
copy_page(vto, vfrom);
/*
* We should be able to use the following optimisation, however
* there are two problems.
* Firstly a bug in some versions of binutils meant PLT sections
* were not marked executable.
* Secondly the first word in the GOT section is blrl, used
* to establish the GOT address. Until recently the GOT was
* not marked executable.
* - Anton
*/
#if 0
if (!vma->vm_file && ((vma->vm_flags & VM_EXEC) == 0))
return;
#endif
flush_dcache_page(pg);
}
Christophe
^ permalink raw reply
* Re: [PATCH v3 5/5] powerpc/mm: Fix growth direction for hugepages mmaps with slice
From: Aneesh Kumar K.V @ 2018-01-24 10:08 UTC (permalink / raw)
To: Christophe LEROY, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Scott Wood
Cc: linux-kernel, linuxppc-dev
In-Reply-To: <03c7f5ce-c2ed-7038-3a8b-3bb7a9a4a2dc@c-s.fr>
On 01/24/2018 03:33 PM, Christophe LEROY wrote:
>
>
> Le 24/01/2018 à 10:51, Aneesh Kumar K.V a écrit :
>>
>>
>> On 01/24/2018 03:09 PM, Christophe LEROY wrote:
>>>
>>>
>>> Le 24/01/2018 à 10:35, Aneesh Kumar K.V a écrit :
>>>>
>>
>>>>> Did you try with HUGETLB_MORECORE_HEAPBASE=0x11000000 on PPC64 as I
>>>>> suggested in my last email on this subject (22/01/2018 9:22) ?
>>>>
>>>>
>>>> yes. The test ran fine for me
>>>
>>> You tried with 0x30000000, it works as well on PPC32.
>>>
>>> I'd really like you to try with 0x11000000 which is in the same slice
>>> as the 10020000-10030000 range.
>>>
>>>
>>
>> Now that explains is better. But then the requested HEAPBASE was not
>> free and hence topdown search got an address in the below range.
>>
>> 7efffd000000-7f0000000000 rw-p 00000000 00:0d 1082770 /anon_hugepage
>> (deleted)
>>
>>
>> The new range allocated is such that there is no scope for expansion
>> of heap if we do a topdown search. But why should that require us to
>> change from topdown/bottomup search?
>>
>>
>> 10000000-10010000 r-xp 00000000 fc:00 9044312 /home/kvaneesh/a.out
>> 10010000-10020000 r--p 00000000 fc:00 9044312 /home/kvaneesh/a.out
>> 10020000-10030000 rw-p 00010000 fc:00 9044312 /home/kvaneesh/a.out
>> 7efffd000000-7f0000000000 rw-p 00000000 00:0d 1082770 /anon_hugepage
>> (deleted)
>> 7ffff2d40000-7ffff7d60000 rw-p 00000000 00:00 0
>> 7ffff7d60000-7ffff7f10000 r-xp 00000000 fc:00 9250090
>> /lib/powerpc64le-linux-gnu/libc-2.23.so
>> 7ffff7f10000-7ffff7f20000 r--p 001a0000 fc:00 9250090
>> /lib/powerpc64le-linux-gnu/libc-2.23.so
>> 7ffff7f20000-7ffff7f30000 rw-p 001b0000 fc:00 9250090
>> /lib/powerpc64le-linux-gnu/libc-2.23.so
>> 7ffff7f40000-7ffff7f60000 r-xp 00000000 fc:00 10754812
>> /usr/lib/libhugetlbfs.so.0
>> 7ffff7f60000-7ffff7f70000 r--p 00010000 fc:00 10754812
>> /usr/lib/libhugetlbfs.so.0
>> 7ffff7f70000-7ffff7f80000 rw-p 00020000 fc:00 10754812
>> /usr/lib/libhugetlbfs.so.0
>> 7ffff7f80000-7ffff7fa0000 r-xp 00000000 00:00 0 [vdso]
>> 7ffff7fa0000-7ffff7fe0000 r-xp 00000000 fc:00 9250107
>> /lib/powerpc64le-linux-gnu/ld-2.23.so
>> 7ffff7fe0000-7ffff7ff0000 r--p 00030000 fc:00 9250107
>> /lib/powerpc64le-linux-gnu/ld-2.23.so
>> 7ffff7ff0000-7ffff8000000 rw-p 00040000 fc:00 9250107
>> /lib/powerpc64le-linux-gnu/ld-2.23.so
>> 7ffffffd0000-800000000000 rw-p 00000000 00:00 0 [stack]
>>
>>
>> For the specific test, one should pass the HEAPBASE value such that it
>> can be expanded if required isn't it ?
>
> For the test, yes, it is dumb to pass an unusable HEAPBASE, but what
> happens in real life:
> * PPC32: No HEAPBASE, hugetlbfs defines a HEAPBASE at sbrk(0) +
> PAGE_SIZE = 0x10800000 ==> This is in the same slice as already
> allocated ==> the kernel does as if mmap() had been called with no hint
> address and allocates something unusable instead.
> * PPC64: No HEAPBASE, hugetlbfs seems to define a HEAPBASE at
> 100000000000, which doesn't conflict with an already allocated mapping
> ==> it works.
>
> Now, when we take the generic case, ie when slice is not activated, when
> you call mmap() without a hint address, it allocates a suitable address
> because it does bottom-up. Why do differently with slices ?
>
IIUC that is largely arch dependent, PPC64 always did topdown search.
Even for regular non hugetlb mmap it did topdown search. If you set
legacy mmap we selected bottom up approach. You can check
arch_pick_mmap_layout() for more details. Now x86 is slightly different.
For the default search if we can't find a mapping address it will try a
bottomup search. Having said that if you think libhugetlbfs made
assumptions with respect to 8xx and you don't want to break it make
8xx unmapped area search bottomup.
-aneesh
^ permalink raw reply
* Re: [PATCH v3 5/5] powerpc/mm: Fix growth direction for hugepages mmaps with slice
From: Christophe LEROY @ 2018-01-24 10:03 UTC (permalink / raw)
To: Aneesh Kumar K.V, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Scott Wood
Cc: linux-kernel, linuxppc-dev
In-Reply-To: <47bbd8be-7b2e-245b-08d9-24958eec2ed2@linux.vnet.ibm.com>
Le 24/01/2018 à 10:51, Aneesh Kumar K.V a écrit :
>
>
> On 01/24/2018 03:09 PM, Christophe LEROY wrote:
>>
>>
>> Le 24/01/2018 à 10:35, Aneesh Kumar K.V a écrit :
>>>
>
>>>> Did you try with HUGETLB_MORECORE_HEAPBASE=0x11000000 on PPC64 as I
>>>> suggested in my last email on this subject (22/01/2018 9:22) ?
>>>
>>>
>>> yes. The test ran fine for me
>>
>> You tried with 0x30000000, it works as well on PPC32.
>>
>> I'd really like you to try with 0x11000000 which is in the same slice
>> as the 10020000-10030000 range.
>>
>>
>
> Now that explains is better. But then the requested HEAPBASE was not
> free and hence topdown search got an address in the below range.
>
> 7efffd000000-7f0000000000 rw-p 00000000 00:0d 1082770 /anon_hugepage
> (deleted)
>
>
> The new range allocated is such that there is no scope for expansion of
> heap if we do a topdown search. But why should that require us to change
> from topdown/bottomup search?
>
>
> 10000000-10010000 r-xp 00000000 fc:00 9044312 /home/kvaneesh/a.out
> 10010000-10020000 r--p 00000000 fc:00 9044312 /home/kvaneesh/a.out
> 10020000-10030000 rw-p 00010000 fc:00 9044312 /home/kvaneesh/a.out
> 7efffd000000-7f0000000000 rw-p 00000000 00:0d 1082770 /anon_hugepage
> (deleted)
> 7ffff2d40000-7ffff7d60000 rw-p 00000000 00:00 0
> 7ffff7d60000-7ffff7f10000 r-xp 00000000 fc:00 9250090
> /lib/powerpc64le-linux-gnu/libc-2.23.so
> 7ffff7f10000-7ffff7f20000 r--p 001a0000 fc:00 9250090
> /lib/powerpc64le-linux-gnu/libc-2.23.so
> 7ffff7f20000-7ffff7f30000 rw-p 001b0000 fc:00 9250090
> /lib/powerpc64le-linux-gnu/libc-2.23.so
> 7ffff7f40000-7ffff7f60000 r-xp 00000000 fc:00 10754812
> /usr/lib/libhugetlbfs.so.0
> 7ffff7f60000-7ffff7f70000 r--p 00010000 fc:00 10754812
> /usr/lib/libhugetlbfs.so.0
> 7ffff7f70000-7ffff7f80000 rw-p 00020000 fc:00 10754812
> /usr/lib/libhugetlbfs.so.0
> 7ffff7f80000-7ffff7fa0000 r-xp 00000000 00:00 0 [vdso]
> 7ffff7fa0000-7ffff7fe0000 r-xp 00000000 fc:00 9250107
> /lib/powerpc64le-linux-gnu/ld-2.23.so
> 7ffff7fe0000-7ffff7ff0000 r--p 00030000 fc:00 9250107
> /lib/powerpc64le-linux-gnu/ld-2.23.so
> 7ffff7ff0000-7ffff8000000 rw-p 00040000 fc:00 9250107
> /lib/powerpc64le-linux-gnu/ld-2.23.so
> 7ffffffd0000-800000000000 rw-p 00000000 00:00 0 [stack]
>
>
> For the specific test, one should pass the HEAPBASE value such that it
> can be expanded if required isn't it ?
For the test, yes, it is dumb to pass an unusable HEAPBASE, but what
happens in real life:
* PPC32: No HEAPBASE, hugetlbfs defines a HEAPBASE at sbrk(0) +
PAGE_SIZE = 0x10800000 ==> This is in the same slice as already
allocated ==> the kernel does as if mmap() had been called with no hint
address and allocates something unusable instead.
* PPC64: No HEAPBASE, hugetlbfs seems to define a HEAPBASE at
100000000000, which doesn't conflict with an already allocated mapping
==> it works.
Now, when we take the generic case, ie when slice is not activated, when
you call mmap() without a hint address, it allocates a suitable address
because it does bottom-up. Why do differently with slices ?
Christophe
^ permalink raw reply
* Re: [PATCH v3 5/5] powerpc/mm: Fix growth direction for hugepages mmaps with slice
From: Aneesh Kumar K.V @ 2018-01-24 9:51 UTC (permalink / raw)
To: Christophe LEROY, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Scott Wood
Cc: linux-kernel, linuxppc-dev
In-Reply-To: <c6ca42a1-eae3-8f07-79e5-a71b3cdfea81@c-s.fr>
On 01/24/2018 03:09 PM, Christophe LEROY wrote:
>
>
> Le 24/01/2018 à 10:35, Aneesh Kumar K.V a écrit :
>>
>>> Did you try with HUGETLB_MORECORE_HEAPBASE=0x11000000 on PPC64 as I
>>> suggested in my last email on this subject (22/01/2018 9:22) ?
>>
>>
>> yes. The test ran fine for me
>
> You tried with 0x30000000, it works as well on PPC32.
>
> I'd really like you to try with 0x11000000 which is in the same slice as
> the 10020000-10030000 range.
>
>
Now that explains is better. But then the requested HEAPBASE was not
free and hence topdown search got an address in the below range.
7efffd000000-7f0000000000 rw-p 00000000 00:0d 1082770
/anon_hugepage (deleted)
The new range allocated is such that there is no scope for expansion of
heap if we do a topdown search. But why should that require us to change
from topdown/bottomup search?
10000000-10010000 r-xp 00000000 fc:00 9044312
/home/kvaneesh/a.out
10010000-10020000 r--p 00000000 fc:00 9044312
/home/kvaneesh/a.out
10020000-10030000 rw-p 00010000 fc:00 9044312
/home/kvaneesh/a.out
7efffd000000-7f0000000000 rw-p 00000000 00:0d 1082770
/anon_hugepage (deleted)
7ffff2d40000-7ffff7d60000 rw-p 00000000 00:00 0
7ffff7d60000-7ffff7f10000 r-xp 00000000 fc:00 9250090
/lib/powerpc64le-linux-gnu/libc-2.23.so
7ffff7f10000-7ffff7f20000 r--p 001a0000 fc:00 9250090
/lib/powerpc64le-linux-gnu/libc-2.23.so
7ffff7f20000-7ffff7f30000 rw-p 001b0000 fc:00 9250090
/lib/powerpc64le-linux-gnu/libc-2.23.so
7ffff7f40000-7ffff7f60000 r-xp 00000000 fc:00 10754812
/usr/lib/libhugetlbfs.so.0
7ffff7f60000-7ffff7f70000 r--p 00010000 fc:00 10754812
/usr/lib/libhugetlbfs.so.0
7ffff7f70000-7ffff7f80000 rw-p 00020000 fc:00 10754812
/usr/lib/libhugetlbfs.so.0
7ffff7f80000-7ffff7fa0000 r-xp 00000000 00:00 0
[vdso]
7ffff7fa0000-7ffff7fe0000 r-xp 00000000 fc:00 9250107
/lib/powerpc64le-linux-gnu/ld-2.23.so
7ffff7fe0000-7ffff7ff0000 r--p 00030000 fc:00 9250107
/lib/powerpc64le-linux-gnu/ld-2.23.so
7ffff7ff0000-7ffff8000000 rw-p 00040000 fc:00 9250107
/lib/powerpc64le-linux-gnu/ld-2.23.so
7ffffffd0000-800000000000 rw-p 00000000 00:00 0
[stack]
For the specific test, one should pass the HEAPBASE value such that it
can be expanded if required isn't it ?
-aneesh
^ permalink raw reply
* Re: [PATCH 25/26] KVM: PPC: Book3S PR: Support TAR handling for PR KVM HTM.
From: Paul Mackerras @ 2018-01-24 4:02 UTC (permalink / raw)
To: wei.guo.simon; +Cc: linuxppc-dev, kvm, kvm-ppc
In-Reply-To: <1515665499-31710-26-git-send-email-wei.guo.simon@gmail.com>
On Thu, Jan 11, 2018 at 06:11:38PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> Currently guest kernel doesn't handle TAR fac unavailable and it always
> runs with TAR bit on. PR KVM will lazily enable TAR. TAR is not a
> frequent-use reg and it is not included in SVCPU struct.
>
> To make it work for transaction memory at PR KVM:
> 1). Flush/giveup TAR at kvmppc_save_tm_pr().
> 2) If we are receiving a TAR fac unavail exception inside a transaction,
> the checkpointed TAR might be a TAR value from another process. So we need
> treclaim the transaction, then load the desired TAR value into reg, and
> perform trecheckpoint.
> 3) Load TAR facility at kvmppc_restore_tm_pr() when TM active.
> The reason we always loads TAR when restoring TM is that:
> If we don't do this way, when there is a TAR fac unavailable exception
> during TM active:
> case 1: it is the 1st TAR fac unavail exception after tbegin.
> vcpu->arch.tar should be reloaded as checkpoint tar val.
> case 2: it is the 2nd or later TAR fac unavail exception after tbegin.
> vcpu->arch.tar_tm should be reloaded as checkpoint tar val.
> There will be unnecessary difficulty to handle the above 2 cases.
>
> at the end of emulating treclaim., the correct TAR val need to be loaded
> into reg if FSCR_TAR bit is on.
> at the beginning of emulating trechkpt., TAR needs to be flushed so that
> the right tar val can be copy into tar_tm.
Would it be simpler always to load up TAR when guest_MSR[TM] is 1?
Paul.
^ permalink raw reply
* Re: [PATCH v3 5/5] powerpc/mm: Fix growth direction for hugepages mmaps with slice
From: Christophe LEROY @ 2018-01-24 9:39 UTC (permalink / raw)
To: Aneesh Kumar K.V, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Scott Wood
Cc: linux-kernel, linuxppc-dev
In-Reply-To: <9aea8b6f-0d1a-b700-efe4-8efcebc5d16d@linux.vnet.ibm.com>
Le 24/01/2018 à 10:35, Aneesh Kumar K.V a écrit :
>
>
> On 01/24/2018 02:57 PM, Christophe LEROY wrote:
>>
>>
>> Le 24/01/2018 à 10:15, Aneesh Kumar K.V a écrit :
>>>
>>>
>>> On 01/24/2018 02:32 PM, Christophe Leroy wrote:
>>>> An application running with libhugetlbfs fails to allocate
>>>> additional pages to HEAP due to the hugemap being done
>>>> inconditionally as topdown mapping:
>>>>
>>>> mmap(0x10080000, 1572864, PROT_READ|PROT_WRITE,
>>>> MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0) = 0x73e80000
>>>> [...]
>>>> mmap(0x74000000, 1048576, PROT_READ|PROT_WRITE,
>>>> MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0x180000) = 0x73d80000
>>>> munmap(0x73d80000, 1048576) = 0
>>>> [...]
>>>> mmap(0x74000000, 1572864, PROT_READ|PROT_WRITE,
>>>> MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0x180000) = 0x73d00000
>>>> munmap(0x73d00000, 1572864) = 0
>>>> [...]
>>>> mmap(0x74000000, 1572864, PROT_READ|PROT_WRITE,
>>>> MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0x180000) = 0x73d00000
>>>> munmap(0x73d00000, 1572864) = 0
>>>> [...]
>>>>
>>>> As one can see from the above strace log, mmap() allocates further
>>>> pages below the initial one because no space is available on top of it.
>>>>
>>>> This patch fixes it by requesting bottomup mapping as the non
>>>> generic hugetlb_get_unmapped_area() does
>>>>
>>>> Fixes: d0f13e3c20b6f ("[POWERPC] Introduce address space "slices" ")
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>> ---
>>>> v3: Was a standalone patch before, but conflicts with this serie.
>>>>
>>>> arch/powerpc/mm/hugetlbpage.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/powerpc/mm/hugetlbpage.c
>>>> b/arch/powerpc/mm/hugetlbpage.c
>>>> index 79e1378ee303..368ea6b248ad 100644
>>>> --- a/arch/powerpc/mm/hugetlbpage.c
>>>> +++ b/arch/powerpc/mm/hugetlbpage.c
>>>> @@ -558,7 +558,7 @@ unsigned long hugetlb_get_unmapped_area(struct
>>>> file *file, unsigned long addr,
>>>> return radix__hugetlb_get_unmapped_area(file, addr, len,
>>>> pgoff, flags);
>>>> #endif
>>>> - return slice_get_unmapped_area(addr, len, flags, mmu_psize, 1);
>>>> + return slice_get_unmapped_area(addr, len, flags, mmu_psize, 0);
>>>> }
>>>> #endif
>>>
>>> Why make this change also for PPC64? Can you do this #ifdef 8xx?.You
>>> can ideally move hugetlb_get_unmapped_area to slice.h and then make
>>> this much simpler for 8xxx?
>>>
>>
>> Did you try with HUGETLB_MORECORE_HEAPBASE=0x11000000 on PPC64 as I
>> suggested in my last email on this subject (22/01/2018 9:22) ?
>
>
> yes. The test ran fine for me
You tried with 0x30000000, it works as well on PPC32.
I'd really like you to try with 0x11000000 which is in the same slice as
the 10020000-10030000 range.
Christophe
>
> kvaneesh@ltctulc6a-p1:[~]$ HUGETLB_MORECORE=yes
> HUGETLB_MORECORE_HEAPBASE=0x30000000 ./a.out
> 10000000-10010000 r-xp 00000000 fc:00 9044312 /home/kvaneesh/a.out
> 10010000-10020000 r--p 00000000 fc:00 9044312 /home/kvaneesh/a.out
> 10020000-10030000 rw-p 00010000 fc:00 9044312 /home/kvaneesh/a.out
> 30000000-33000000 rw-p 00000000 00:0d 1062697 /anon_hugepage (deleted)
> 33000000-35000000 rw-p 03000000 00:0d 1062698 /anon_hugepage (deleted)
> 35000000-37000000 rw-p 05000000 00:0d 1062699 /anon_hugepage (deleted)
> 7ffff7d60000-7ffff7f10000 r-xp 00000000 fc:00 9250090
> /lib/powerpc64le-linux-gnu/libc-2.23.so
> 7ffff7f10000-7ffff7f20000 r--p 001a0000 fc:00 9250090
> /lib/powerpc64le-linux-gnu/libc-2.23.so
> 7ffff7f20000-7ffff7f30000 rw-p 001b0000 fc:00 9250090
> /lib/powerpc64le-linux-gnu/libc-2.23.so
> 7ffff7f40000-7ffff7f60000 r-xp 00000000 fc:00 10754812
> /usr/lib/libhugetlbfs.so.0
> 7ffff7f60000-7ffff7f70000 r--p 00010000 fc:00 10754812
> /usr/lib/libhugetlbfs.so.0
> 7ffff7f70000-7ffff7f80000 rw-p 00020000 fc:00 10754812
> /usr/lib/libhugetlbfs.so.0
> 7ffff7f80000-7ffff7fa0000 r-xp 00000000 00:00 0 [vdso]
> 7ffff7fa0000-7ffff7fe0000 r-xp 00000000 fc:00 9250107
> /lib/powerpc64le-linux-gnu/ld-2.23.so
> 7ffff7fe0000-7ffff7ff0000 r--p 00030000 fc:00 9250107
> /lib/powerpc64le-linux-gnu/ld-2.23.so
> 7ffff7ff0000-7ffff8000000 rw-p 00040000 fc:00 9250107
> /lib/powerpc64le-linux-gnu/ld-2.23.so
> 7ffffffd0000-800000000000 rw-p 00000000 00:00 0 [stack]
>
>
>>
>> Before doing anything specific to the PPC32/8xx, I'd like to be sure
>> the issue is definitly only on PPC32.
>>
>
> I am not sure I understand the problem correctly. If there is a free
> space in the required range, both topdown/bottomup search should be able
> to find it. Unless topdown found another free area suitable for hugetlb
> allocation above. My take is we should not change the topdown to
> bottomup without really understanding the failure scenarios.
>
> -aneesh
^ permalink raw reply
* Re: [PATCH v3 5/5] powerpc/mm: Fix growth direction for hugepages mmaps with slice
From: Aneesh Kumar K.V @ 2018-01-24 9:35 UTC (permalink / raw)
To: Christophe LEROY, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Scott Wood
Cc: linux-kernel, linuxppc-dev
In-Reply-To: <c532efa7-33a4-1f4f-e83a-5eaeba65b799@c-s.fr>
On 01/24/2018 02:57 PM, Christophe LEROY wrote:
>
>
> Le 24/01/2018 à 10:15, Aneesh Kumar K.V a écrit :
>>
>>
>> On 01/24/2018 02:32 PM, Christophe Leroy wrote:
>>> An application running with libhugetlbfs fails to allocate
>>> additional pages to HEAP due to the hugemap being done
>>> inconditionally as topdown mapping:
>>>
>>> mmap(0x10080000, 1572864, PROT_READ|PROT_WRITE,
>>> MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0) = 0x73e80000
>>> [...]
>>> mmap(0x74000000, 1048576, PROT_READ|PROT_WRITE,
>>> MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0x180000) = 0x73d80000
>>> munmap(0x73d80000, 1048576) = 0
>>> [...]
>>> mmap(0x74000000, 1572864, PROT_READ|PROT_WRITE,
>>> MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0x180000) = 0x73d00000
>>> munmap(0x73d00000, 1572864) = 0
>>> [...]
>>> mmap(0x74000000, 1572864, PROT_READ|PROT_WRITE,
>>> MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0x180000) = 0x73d00000
>>> munmap(0x73d00000, 1572864) = 0
>>> [...]
>>>
>>> As one can see from the above strace log, mmap() allocates further
>>> pages below the initial one because no space is available on top of it.
>>>
>>> This patch fixes it by requesting bottomup mapping as the non
>>> generic hugetlb_get_unmapped_area() does
>>>
>>> Fixes: d0f13e3c20b6f ("[POWERPC] Introduce address space "slices" ")
>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>> ---
>>> v3: Was a standalone patch before, but conflicts with this serie.
>>>
>>> arch/powerpc/mm/hugetlbpage.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/mm/hugetlbpage.c
>>> b/arch/powerpc/mm/hugetlbpage.c
>>> index 79e1378ee303..368ea6b248ad 100644
>>> --- a/arch/powerpc/mm/hugetlbpage.c
>>> +++ b/arch/powerpc/mm/hugetlbpage.c
>>> @@ -558,7 +558,7 @@ unsigned long hugetlb_get_unmapped_area(struct
>>> file *file, unsigned long addr,
>>> return radix__hugetlb_get_unmapped_area(file, addr, len,
>>> pgoff, flags);
>>> #endif
>>> - return slice_get_unmapped_area(addr, len, flags, mmu_psize, 1);
>>> + return slice_get_unmapped_area(addr, len, flags, mmu_psize, 0);
>>> }
>>> #endif
>>
>> Why make this change also for PPC64? Can you do this #ifdef 8xx?.You
>> can ideally move hugetlb_get_unmapped_area to slice.h and then make
>> this much simpler for 8xxx?
>>
>
> Did you try with HUGETLB_MORECORE_HEAPBASE=0x11000000 on PPC64 as I
> suggested in my last email on this subject (22/01/2018 9:22) ?
yes. The test ran fine for me
kvaneesh@ltctulc6a-p1:[~]$ HUGETLB_MORECORE=yes
HUGETLB_MORECORE_HEAPBASE=0x30000000 ./a.out
10000000-10010000 r-xp 00000000 fc:00 9044312
/home/kvaneesh/a.out
10010000-10020000 r--p 00000000 fc:00 9044312
/home/kvaneesh/a.out
10020000-10030000 rw-p 00010000 fc:00 9044312
/home/kvaneesh/a.out
30000000-33000000 rw-p 00000000 00:0d 1062697
/anon_hugepage (deleted)
33000000-35000000 rw-p 03000000 00:0d 1062698
/anon_hugepage (deleted)
35000000-37000000 rw-p 05000000 00:0d 1062699
/anon_hugepage (deleted)
7ffff7d60000-7ffff7f10000 r-xp 00000000 fc:00 9250090
/lib/powerpc64le-linux-gnu/libc-2.23.so
7ffff7f10000-7ffff7f20000 r--p 001a0000 fc:00 9250090
/lib/powerpc64le-linux-gnu/libc-2.23.so
7ffff7f20000-7ffff7f30000 rw-p 001b0000 fc:00 9250090
/lib/powerpc64le-linux-gnu/libc-2.23.so
7ffff7f40000-7ffff7f60000 r-xp 00000000 fc:00 10754812
/usr/lib/libhugetlbfs.so.0
7ffff7f60000-7ffff7f70000 r--p 00010000 fc:00 10754812
/usr/lib/libhugetlbfs.so.0
7ffff7f70000-7ffff7f80000 rw-p 00020000 fc:00 10754812
/usr/lib/libhugetlbfs.so.0
7ffff7f80000-7ffff7fa0000 r-xp 00000000 00:00 0
[vdso]
7ffff7fa0000-7ffff7fe0000 r-xp 00000000 fc:00 9250107
/lib/powerpc64le-linux-gnu/ld-2.23.so
7ffff7fe0000-7ffff7ff0000 r--p 00030000 fc:00 9250107
/lib/powerpc64le-linux-gnu/ld-2.23.so
7ffff7ff0000-7ffff8000000 rw-p 00040000 fc:00 9250107
/lib/powerpc64le-linux-gnu/ld-2.23.so
7ffffffd0000-800000000000 rw-p 00000000 00:00 0
[stack]
>
> Before doing anything specific to the PPC32/8xx, I'd like to be sure the
> issue is definitly only on PPC32.
>
I am not sure I understand the problem correctly. If there is a free
space in the required range, both topdown/bottomup search should be able
to find it. Unless topdown found another free area suitable for hugetlb
allocation above. My take is we should not change the topdown to
bottomup without really understanding the failure scenarios.
-aneesh
^ permalink raw reply
* Re: [PATCH v3 5/5] powerpc/mm: Fix growth direction for hugepages mmaps with slice
From: Christophe LEROY @ 2018-01-24 9:27 UTC (permalink / raw)
To: Aneesh Kumar K.V, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Scott Wood
Cc: linux-kernel, linuxppc-dev
In-Reply-To: <c6eff627-9d86-dab1-e91e-9ea5a98c4c55@linux.vnet.ibm.com>
Le 24/01/2018 à 10:15, Aneesh Kumar K.V a écrit :
>
>
> On 01/24/2018 02:32 PM, Christophe Leroy wrote:
>> An application running with libhugetlbfs fails to allocate
>> additional pages to HEAP due to the hugemap being done
>> inconditionally as topdown mapping:
>>
>> mmap(0x10080000, 1572864, PROT_READ|PROT_WRITE,
>> MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0) = 0x73e80000
>> [...]
>> mmap(0x74000000, 1048576, PROT_READ|PROT_WRITE,
>> MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0x180000) = 0x73d80000
>> munmap(0x73d80000, 1048576) = 0
>> [...]
>> mmap(0x74000000, 1572864, PROT_READ|PROT_WRITE,
>> MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0x180000) = 0x73d00000
>> munmap(0x73d00000, 1572864) = 0
>> [...]
>> mmap(0x74000000, 1572864, PROT_READ|PROT_WRITE,
>> MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0x180000) = 0x73d00000
>> munmap(0x73d00000, 1572864) = 0
>> [...]
>>
>> As one can see from the above strace log, mmap() allocates further
>> pages below the initial one because no space is available on top of it.
>>
>> This patch fixes it by requesting bottomup mapping as the non
>> generic hugetlb_get_unmapped_area() does
>>
>> Fixes: d0f13e3c20b6f ("[POWERPC] Introduce address space "slices" ")
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> v3: Was a standalone patch before, but conflicts with this serie.
>>
>> arch/powerpc/mm/hugetlbpage.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/mm/hugetlbpage.c
>> b/arch/powerpc/mm/hugetlbpage.c
>> index 79e1378ee303..368ea6b248ad 100644
>> --- a/arch/powerpc/mm/hugetlbpage.c
>> +++ b/arch/powerpc/mm/hugetlbpage.c
>> @@ -558,7 +558,7 @@ unsigned long hugetlb_get_unmapped_area(struct
>> file *file, unsigned long addr,
>> return radix__hugetlb_get_unmapped_area(file, addr, len,
>> pgoff, flags);
>> #endif
>> - return slice_get_unmapped_area(addr, len, flags, mmu_psize, 1);
>> + return slice_get_unmapped_area(addr, len, flags, mmu_psize, 0);
>> }
>> #endif
>
> Why make this change also for PPC64? Can you do this #ifdef 8xx?.You can
> ideally move hugetlb_get_unmapped_area to slice.h and then make this
> much simpler for 8xxx?
>
Did you try with HUGETLB_MORECORE_HEAPBASE=0x11000000 on PPC64 as I
suggested in my last email on this subject (22/01/2018 9:22) ?
Before doing anything specific to the PPC32/8xx, I'd like to be sure the
issue is definitly only on PPC32.
Thanks,
Christophe
^ permalink raw reply
* Re: [PATCH v3 5/5] powerpc/mm: Fix growth direction for hugepages mmaps with slice
From: Aneesh Kumar K.V @ 2018-01-24 9:15 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Scott Wood
Cc: linux-kernel, linuxppc-dev
In-Reply-To: <362a93307a09b521878c47a8999a39a228184293.1516783089.git.christophe.leroy@c-s.fr>
On 01/24/2018 02:32 PM, Christophe Leroy wrote:
> An application running with libhugetlbfs fails to allocate
> additional pages to HEAP due to the hugemap being done
> inconditionally as topdown mapping:
>
> mmap(0x10080000, 1572864, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0) = 0x73e80000
> [...]
> mmap(0x74000000, 1048576, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0x180000) = 0x73d80000
> munmap(0x73d80000, 1048576) = 0
> [...]
> mmap(0x74000000, 1572864, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0x180000) = 0x73d00000
> munmap(0x73d00000, 1572864) = 0
> [...]
> mmap(0x74000000, 1572864, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0x180000) = 0x73d00000
> munmap(0x73d00000, 1572864) = 0
> [...]
>
> As one can see from the above strace log, mmap() allocates further
> pages below the initial one because no space is available on top of it.
>
> This patch fixes it by requesting bottomup mapping as the non
> generic hugetlb_get_unmapped_area() does
>
> Fixes: d0f13e3c20b6f ("[POWERPC] Introduce address space "slices" ")
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> v3: Was a standalone patch before, but conflicts with this serie.
>
> arch/powerpc/mm/hugetlbpage.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
> index 79e1378ee303..368ea6b248ad 100644
> --- a/arch/powerpc/mm/hugetlbpage.c
> +++ b/arch/powerpc/mm/hugetlbpage.c
> @@ -558,7 +558,7 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
> return radix__hugetlb_get_unmapped_area(file, addr, len,
> pgoff, flags);
> #endif
> - return slice_get_unmapped_area(addr, len, flags, mmu_psize, 1);
> + return slice_get_unmapped_area(addr, len, flags, mmu_psize, 0);
> }
> #endif
Why make this change also for PPC64? Can you do this #ifdef 8xx?.You can
ideally move hugetlb_get_unmapped_area to slice.h and then make this
much simpler for 8xxx?
-aneesh
-aneesh
^ permalink raw reply
* [PATCH v3 5/5] powerpc/mm: Fix growth direction for hugepages mmaps with slice
From: Christophe Leroy @ 2018-01-24 9:02 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Scott Wood, aneesh.kumar
Cc: linux-kernel, linuxppc-dev
In-Reply-To: <6920f6efe2dcdabf59350b2d31ee6bd4bdef57f4.1516783089.git.christophe.leroy@c-s.fr>
An application running with libhugetlbfs fails to allocate
additional pages to HEAP due to the hugemap being done
inconditionally as topdown mapping:
mmap(0x10080000, 1572864, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0) = 0x73e80000
[...]
mmap(0x74000000, 1048576, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0x180000) = 0x73d80000
munmap(0x73d80000, 1048576) = 0
[...]
mmap(0x74000000, 1572864, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0x180000) = 0x73d00000
munmap(0x73d00000, 1572864) = 0
[...]
mmap(0x74000000, 1572864, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0x180000) = 0x73d00000
munmap(0x73d00000, 1572864) = 0
[...]
As one can see from the above strace log, mmap() allocates further
pages below the initial one because no space is available on top of it.
This patch fixes it by requesting bottomup mapping as the non
generic hugetlb_get_unmapped_area() does
Fixes: d0f13e3c20b6f ("[POWERPC] Introduce address space "slices" ")
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v3: Was a standalone patch before, but conflicts with this serie.
arch/powerpc/mm/hugetlbpage.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index 79e1378ee303..368ea6b248ad 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c
@@ -558,7 +558,7 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
return radix__hugetlb_get_unmapped_area(file, addr, len,
pgoff, flags);
#endif
- return slice_get_unmapped_area(addr, len, flags, mmu_psize, 1);
+ return slice_get_unmapped_area(addr, len, flags, mmu_psize, 0);
}
#endif
--
2.13.3
^ permalink raw reply related
* [PATCH v3 4/5] powerpc/mm: Allow up to 64 low slices
From: Christophe Leroy @ 2018-01-24 9:02 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Scott Wood, aneesh.kumar
Cc: linux-kernel, linuxppc-dev
In-Reply-To: <6920f6efe2dcdabf59350b2d31ee6bd4bdef57f4.1516783089.git.christophe.leroy@c-s.fr>
While the implementation of the "slices" address space allows
a significant amount of high slices, it limits the number of
low slices to 16 due to the use of a single u64 low_slices_psize
element in struct mm_context_t
On the 8xx, the minimum slice size is the size of the area
covered by a single PMD entry, ie 4M in 4K pages mode and 64M in
16K pages mode. This means we could have at least 64 slices.
In order to override this limitation, this patch switches the
handling of low_slices_psize to char array as done already for
high_slices_psize. This allows to increase the number of low
slices to 64 on the 8xx.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v2: Usign slice_bitmap_xxx() macros instead of bitmap_xxx() functions.
v3: keep low_slices as a u64, this allows 64 slices which is enough.
arch/powerpc/include/asm/book3s/64/mmu.h | 3 +-
arch/powerpc/include/asm/mmu-8xx.h | 7 +++-
arch/powerpc/include/asm/paca.h | 2 +-
arch/powerpc/include/asm/slice.h | 1 -
arch/powerpc/include/asm/slice_32.h | 2 ++
arch/powerpc/include/asm/slice_64.h | 2 ++
arch/powerpc/kernel/paca.c | 3 +-
arch/powerpc/mm/hash_utils_64.c | 13 ++++----
arch/powerpc/mm/slb_low.S | 8 +++--
arch/powerpc/mm/slice.c | 57 +++++++++++++++++---------------
10 files changed, 56 insertions(+), 42 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h b/arch/powerpc/include/asm/book3s/64/mmu.h
index c9448e19847a..b076a2d74c69 100644
--- a/arch/powerpc/include/asm/book3s/64/mmu.h
+++ b/arch/powerpc/include/asm/book3s/64/mmu.h
@@ -91,7 +91,8 @@ typedef struct {
struct npu_context *npu_context;
#ifdef CONFIG_PPC_MM_SLICES
- u64 low_slices_psize; /* SLB page size encodings */
+ /* SLB page size encodings*/
+ unsigned char low_slices_psize[BITS_PER_LONG / BITS_PER_BYTE];
unsigned char high_slices_psize[SLICE_ARRAY_SIZE];
unsigned long slb_addr_limit;
#else
diff --git a/arch/powerpc/include/asm/mmu-8xx.h b/arch/powerpc/include/asm/mmu-8xx.h
index 5f89b6010453..5f37ba06b56c 100644
--- a/arch/powerpc/include/asm/mmu-8xx.h
+++ b/arch/powerpc/include/asm/mmu-8xx.h
@@ -164,6 +164,11 @@
*/
#define SPRN_M_TW 799
+#ifdef CONFIG_PPC_MM_SLICES
+#include <asm/slice_32.h>
+#define SLICE_ARRAY_SIZE (1 << (32 - SLICE_LOW_SHIFT - 1))
+#endif
+
#ifndef __ASSEMBLY__
typedef struct {
unsigned int id;
@@ -171,7 +176,7 @@ typedef struct {
unsigned long vdso_base;
#ifdef CONFIG_PPC_MM_SLICES
u16 user_psize; /* page size index */
- u64 low_slices_psize; /* page size encodings */
+ unsigned char low_slices_psize[SLICE_ARRAY_SIZE];
unsigned char high_slices_psize[0];
unsigned long slb_addr_limit;
#endif
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index 23ac7fc0af23..a3e531fe9ac7 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -141,7 +141,7 @@ struct paca_struct {
#ifdef CONFIG_PPC_BOOK3S
mm_context_id_t mm_ctx_id;
#ifdef CONFIG_PPC_MM_SLICES
- u64 mm_ctx_low_slices_psize;
+ unsigned char mm_ctx_low_slices_psize[BITS_PER_LONG / BITS_PER_BYTE];
unsigned char mm_ctx_high_slices_psize[SLICE_ARRAY_SIZE];
unsigned long mm_ctx_slb_addr_limit;
#else
diff --git a/arch/powerpc/include/asm/slice.h b/arch/powerpc/include/asm/slice.h
index 2b4b70de7e71..b67ba8faa507 100644
--- a/arch/powerpc/include/asm/slice.h
+++ b/arch/powerpc/include/asm/slice.h
@@ -16,7 +16,6 @@
#define HAVE_ARCH_UNMAPPED_AREA
#define HAVE_ARCH_UNMAPPED_AREA_TOPDOWN
-#define SLICE_LOW_SHIFT 28
#define SLICE_LOW_TOP (0x100000000ull)
#define SLICE_NUM_LOW (SLICE_LOW_TOP >> SLICE_LOW_SHIFT)
#define GET_LOW_SLICE_INDEX(addr) ((addr) >> SLICE_LOW_SHIFT)
diff --git a/arch/powerpc/include/asm/slice_32.h b/arch/powerpc/include/asm/slice_32.h
index 7e27c0dfb913..349187c20100 100644
--- a/arch/powerpc/include/asm/slice_32.h
+++ b/arch/powerpc/include/asm/slice_32.h
@@ -2,6 +2,8 @@
#ifndef _ASM_POWERPC_SLICE_32_H
#define _ASM_POWERPC_SLICE_32_H
+#define SLICE_LOW_SHIFT 26 /* 64 slices */
+
#define SLICE_HIGH_SHIFT 0
#define SLICE_NUM_HIGH 0ul
#define GET_HIGH_SLICE_INDEX(addr) (addr & 0)
diff --git a/arch/powerpc/include/asm/slice_64.h b/arch/powerpc/include/asm/slice_64.h
index 9d1c97b83010..0959475239c6 100644
--- a/arch/powerpc/include/asm/slice_64.h
+++ b/arch/powerpc/include/asm/slice_64.h
@@ -2,6 +2,8 @@
#ifndef _ASM_POWERPC_SLICE_64_H
#define _ASM_POWERPC_SLICE_64_H
+#define SLICE_LOW_SHIFT 28
+
#define SLICE_HIGH_SHIFT 40
#define SLICE_NUM_HIGH (H_PGTABLE_RANGE >> SLICE_HIGH_SHIFT)
#define GET_HIGH_SLICE_INDEX(addr) ((addr) >> SLICE_HIGH_SHIFT)
diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
index d6597038931d..8e1566bf82b8 100644
--- a/arch/powerpc/kernel/paca.c
+++ b/arch/powerpc/kernel/paca.c
@@ -264,7 +264,8 @@ void copy_mm_to_paca(struct mm_struct *mm)
#ifdef CONFIG_PPC_MM_SLICES
VM_BUG_ON(!mm->context.slb_addr_limit);
get_paca()->mm_ctx_slb_addr_limit = mm->context.slb_addr_limit;
- get_paca()->mm_ctx_low_slices_psize = context->low_slices_psize;
+ memcpy(&get_paca()->mm_ctx_low_slices_psize,
+ &context->low_slices_psize, sizeof(context->low_slices_psize));
memcpy(&get_paca()->mm_ctx_high_slices_psize,
&context->high_slices_psize, TASK_SLICE_ARRAY_SZ(mm));
#else /* CONFIG_PPC_MM_SLICES */
diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
index 655a5a9a183d..da696565b969 100644
--- a/arch/powerpc/mm/hash_utils_64.c
+++ b/arch/powerpc/mm/hash_utils_64.c
@@ -1097,19 +1097,18 @@ unsigned int hash_page_do_lazy_icache(unsigned int pp, pte_t pte, int trap)
#ifdef CONFIG_PPC_MM_SLICES
static unsigned int get_paca_psize(unsigned long addr)
{
- u64 lpsizes;
- unsigned char *hpsizes;
+ unsigned char *psizes;
unsigned long index, mask_index;
if (addr < SLICE_LOW_TOP) {
- lpsizes = get_paca()->mm_ctx_low_slices_psize;
+ psizes = get_paca()->mm_ctx_low_slices_psize;
index = GET_LOW_SLICE_INDEX(addr);
- return (lpsizes >> (index * 4)) & 0xF;
+ } else {
+ psizes = get_paca()->mm_ctx_high_slices_psize;
+ index = GET_HIGH_SLICE_INDEX(addr);
}
- hpsizes = get_paca()->mm_ctx_high_slices_psize;
- index = GET_HIGH_SLICE_INDEX(addr);
mask_index = index & 0x1;
- return (hpsizes[index >> 1] >> (mask_index * 4)) & 0xF;
+ return (psizes[index >> 1] >> (mask_index * 4)) & 0xF;
}
#else
diff --git a/arch/powerpc/mm/slb_low.S b/arch/powerpc/mm/slb_low.S
index 2cf5ef3fc50d..2c7c717fd2ea 100644
--- a/arch/powerpc/mm/slb_low.S
+++ b/arch/powerpc/mm/slb_low.S
@@ -200,10 +200,12 @@ END_MMU_FTR_SECTION_IFCLR(MMU_FTR_1T_SEGMENT)
5:
/*
* Handle lpsizes
- * r9 is get_paca()->context.low_slices_psize, r11 is index
+ * r9 is get_paca()->context.low_slices_psize[index], r11 is mask_index
*/
- ld r9,PACALOWSLICESPSIZE(r13)
- mr r11,r10
+ srdi r11,r10,1 /* index */
+ addi r9,r11,PACALOWSLICESPSIZE
+ lbzx r9,r13,r9 /* r9 is lpsizes[r11] */
+ rldicl r11,r10,0,63 /* r11 = r10 & 0x1 */
6:
sldi r11,r11,2 /* index * 4 */
/* Extract the psize and multiply to get an array offset */
diff --git a/arch/powerpc/mm/slice.c b/arch/powerpc/mm/slice.c
index 549704dfa777..3d573a038d42 100644
--- a/arch/powerpc/mm/slice.c
+++ b/arch/powerpc/mm/slice.c
@@ -148,18 +148,20 @@ static void slice_mask_for_free(struct mm_struct *mm, struct slice_mask *ret,
static void slice_mask_for_size(struct mm_struct *mm, int psize, struct slice_mask *ret,
unsigned long high_limit)
{
- unsigned char *hpsizes;
+ unsigned char *hpsizes, *lpsizes;
int index, mask_index;
unsigned long i;
- u64 lpsizes;
ret->low_slices = 0;
slice_bitmap_zero(ret->high_slices, SLICE_NUM_HIGH);
lpsizes = mm->context.low_slices_psize;
- for (i = 0; i < SLICE_NUM_LOW; i++)
- if (((lpsizes >> (i * 4)) & 0xf) == psize)
+ for (i = 0; i < SLICE_NUM_LOW; i++) {
+ mask_index = i & 0x1;
+ index = i >> 1;
+ if (((lpsizes[index] >> (mask_index * 4)) & 0xf) == psize)
ret->low_slices |= 1u << i;
+ }
if (high_limit <= SLICE_LOW_TOP)
return;
@@ -211,8 +213,7 @@ static void slice_convert(struct mm_struct *mm, struct slice_mask mask, int psiz
{
int index, mask_index;
/* Write the new slice psize bits */
- unsigned char *hpsizes;
- u64 lpsizes;
+ unsigned char *hpsizes, *lpsizes;
unsigned long i, flags;
slice_dbg("slice_convert(mm=%p, psize=%d)\n", mm, psize);
@@ -225,12 +226,13 @@ static void slice_convert(struct mm_struct *mm, struct slice_mask mask, int psiz
lpsizes = mm->context.low_slices_psize;
for (i = 0; i < SLICE_NUM_LOW; i++)
- if (mask.low_slices & (1u << i))
- lpsizes = (lpsizes & ~(0xful << (i * 4))) |
- (((unsigned long)psize) << (i * 4));
-
- /* Assign the value back */
- mm->context.low_slices_psize = lpsizes;
+ if (mask.low_slices & (1u << i)) {
+ mask_index = i & 0x1;
+ index = i >> 1;
+ lpsizes[index] = (lpsizes[index] &
+ ~(0xf << (mask_index * 4))) |
+ (((unsigned long)psize) << (mask_index * 4));
+ }
hpsizes = mm->context.high_slices_psize;
for (i = 0; i < GET_HIGH_SLICE_INDEX(mm->context.slb_addr_limit); i++) {
@@ -629,7 +631,7 @@ unsigned long arch_get_unmapped_area_topdown(struct file *filp,
unsigned int get_slice_psize(struct mm_struct *mm, unsigned long addr)
{
- unsigned char *hpsizes;
+ unsigned char *psizes;
int index, mask_index;
/*
@@ -643,15 +645,14 @@ unsigned int get_slice_psize(struct mm_struct *mm, unsigned long addr)
#endif
}
if (addr < SLICE_LOW_TOP) {
- u64 lpsizes;
- lpsizes = mm->context.low_slices_psize;
+ psizes = mm->context.low_slices_psize;
index = GET_LOW_SLICE_INDEX(addr);
- return (lpsizes >> (index * 4)) & 0xf;
+ } else {
+ psizes = mm->context.high_slices_psize;
+ index = GET_HIGH_SLICE_INDEX(addr);
}
- hpsizes = mm->context.high_slices_psize;
- index = GET_HIGH_SLICE_INDEX(addr);
mask_index = index & 0x1;
- return (hpsizes[index >> 1] >> (mask_index * 4)) & 0xf;
+ return (psizes[index >> 1] >> (mask_index * 4)) & 0xf;
}
EXPORT_SYMBOL_GPL(get_slice_psize);
@@ -672,8 +673,8 @@ EXPORT_SYMBOL_GPL(get_slice_psize);
void slice_set_user_psize(struct mm_struct *mm, unsigned int psize)
{
int index, mask_index;
- unsigned char *hpsizes;
- unsigned long flags, lpsizes;
+ unsigned char *hpsizes, *lpsizes;
+ unsigned long flags;
unsigned int old_psize;
int i;
@@ -691,12 +692,14 @@ void slice_set_user_psize(struct mm_struct *mm, unsigned int psize)
wmb();
lpsizes = mm->context.low_slices_psize;
- for (i = 0; i < SLICE_NUM_LOW; i++)
- if (((lpsizes >> (i * 4)) & 0xf) == old_psize)
- lpsizes = (lpsizes & ~(0xful << (i * 4))) |
- (((unsigned long)psize) << (i * 4));
- /* Assign the value back */
- mm->context.low_slices_psize = lpsizes;
+ for (i = 0; i < SLICE_NUM_LOW; i++) {
+ mask_index = i & 0x1;
+ index = i >> 1;
+ if (((lpsizes[index] >> (mask_index * 4)) & 0xf) == old_psize)
+ lpsizes[index] = (lpsizes[index] &
+ ~(0xf << (mask_index * 4))) |
+ (((unsigned long)psize) << (mask_index * 4));
+ }
hpsizes = mm->context.high_slices_psize;
for (i = 0; i < SLICE_NUM_HIGH; i++) {
--
2.13.3
^ permalink raw reply related
* [PATCH v3 3/5] powerpc/32: Fix hugepage allocation on 8xx at hint address
From: Christophe Leroy @ 2018-01-24 9:02 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Scott Wood, aneesh.kumar
Cc: linux-kernel, linuxppc-dev
In-Reply-To: <6920f6efe2dcdabf59350b2d31ee6bd4bdef57f4.1516783089.git.christophe.leroy@c-s.fr>
On the 8xx, the page size is set in the PMD entry and applies to
all pages of the page table pointed by the said PMD entry.
When an app has some regular pages allocated (e.g. see below) and tries
to mmap() a huge page at a hint address covered by the same PMD entry,
the kernel accepts the hint allthough the 8xx cannot handle different
page sizes in the same PMD entry.
10000000-10001000 r-xp 00000000 00:0f 2597 /root/malloc
10010000-10011000 rwxp 00000000 00:0f 2597 /root/malloc
mmap(0x10080000, 524288, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_ANONYMOUS|0x40000, -1, 0) = 0x10080000
This results the app remaining forever in do_page_fault()/hugetlb_fault()
and when interrupting that app, we get the following warning:
[162980.035629] WARNING: CPU: 0 PID: 2777 at arch/powerpc/mm/hugetlbpage.c:354 hugetlb_free_pgd_range+0xc8/0x1e4
[162980.035699] CPU: 0 PID: 2777 Comm: malloc Tainted: G W 4.14.6 #85
[162980.035744] task: c67e2c00 task.stack: c668e000
[162980.035783] NIP: c000fe18 LR: c00e1eec CTR: c00f90c0
[162980.035830] REGS: c668fc20 TRAP: 0700 Tainted: G W (4.14.6)
[162980.035854] MSR: 00029032 <EE,ME,IR,DR,RI> CR: 24044224 XER: 20000000
[162980.036003]
[162980.036003] GPR00: c00e1eec c668fcd0 c67e2c00 00000010 c6869410 10080000 00000000 77fb4000
[162980.036003] GPR08: ffff0001 0683c001 00000000 ffffff80 44028228 10018a34 00004008 418004fc
[162980.036003] GPR16: c668e000 00040100 c668e000 c06c0000 c668fe78 c668e000 c6835ba0 c668fd48
[162980.036003] GPR24: 00000000 73ffffff 74000000 00000001 77fb4000 100fffff 10100000 10100000
[162980.036743] NIP [c000fe18] hugetlb_free_pgd_range+0xc8/0x1e4
[162980.036839] LR [c00e1eec] free_pgtables+0x12c/0x150
[162980.036861] Call Trace:
[162980.036939] [c668fcd0] [c00f0774] unlink_anon_vmas+0x1c4/0x214 (unreliable)
[162980.037040] [c668fd10] [c00e1eec] free_pgtables+0x12c/0x150
[162980.037118] [c668fd40] [c00eabac] exit_mmap+0xe8/0x1b4
[162980.037210] [c668fda0] [c0019710] mmput.part.9+0x20/0xd8
[162980.037301] [c668fdb0] [c001ecb0] do_exit+0x1f0/0x93c
[162980.037386] [c668fe00] [c001f478] do_group_exit+0x40/0xcc
[162980.037479] [c668fe10] [c002a76c] get_signal+0x47c/0x614
[162980.037570] [c668fe70] [c0007840] do_signal+0x54/0x244
[162980.037654] [c668ff30] [c0007ae8] do_notify_resume+0x34/0x88
[162980.037744] [c668ff40] [c000dae8] do_user_signal+0x74/0xc4
[162980.037781] Instruction dump:
[162980.037821] 7fdff378 81370000 54a3463a 80890020 7d24182e 7c841a14 712a0004 4082ff94
[162980.038014] 2f890000 419e0010 712a0ff0 408200e0 <0fe00000> 54a9000a 7f984840 419d0094
[162980.038216] ---[ end trace c0ceeca8e7a5800a ]---
[162980.038754] BUG: non-zero nr_ptes on freeing mm: 1
[162985.363322] BUG: non-zero nr_ptes on freeing mm: -1
In order to fix this, this patch uses the address space "slices"
implemented for BOOK3S/64 and enhanced to support PPC32 by the
preceding patch.
This patch modifies the context.id on the 8xx to be in the range
[1:16] instead of [0:15] in order to identify context.id == 0 as
not initialised contexts as done on BOOK3S
This patch activates CONFIG_PPC_MM_SLICES when CONFIG_HUGETLB_PAGE is
selected for the 8xx
Alltough we could in theory have as many slices as PMD entries, the
current slices implementation limits the number of low slices to 16.
This limitation is not preventing us to fix the initial issue allthough
it is suboptimal. It will be cured in a subsequent patch.
Fixes: 4b91428699477 ("powerpc/8xx: Implement support of hugepages")
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v2: First patch of v1 serie split in two parts
v3: No changes
arch/powerpc/include/asm/mmu-8xx.h | 6 ++++++
arch/powerpc/kernel/setup-common.c | 2 ++
arch/powerpc/mm/8xx_mmu.c | 2 +-
arch/powerpc/mm/hugetlbpage.c | 2 ++
arch/powerpc/mm/mmu_context_nohash.c | 18 ++++++++++++++++--
arch/powerpc/platforms/Kconfig.cputype | 1 +
6 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/mmu-8xx.h b/arch/powerpc/include/asm/mmu-8xx.h
index 5bb3dbede41a..5f89b6010453 100644
--- a/arch/powerpc/include/asm/mmu-8xx.h
+++ b/arch/powerpc/include/asm/mmu-8xx.h
@@ -169,6 +169,12 @@ typedef struct {
unsigned int id;
unsigned int active;
unsigned long vdso_base;
+#ifdef CONFIG_PPC_MM_SLICES
+ u16 user_psize; /* page size index */
+ u64 low_slices_psize; /* page size encodings */
+ unsigned char high_slices_psize[0];
+ unsigned long slb_addr_limit;
+#endif
} mm_context_t;
#define PHYS_IMMR_BASE (mfspr(SPRN_IMMR) & 0xfff80000)
diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 8fd3a70047f1..edf98ea92035 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -916,6 +916,8 @@ void __init setup_arch(char **cmdline_p)
#ifdef CONFIG_PPC64
if (!radix_enabled())
init_mm.context.slb_addr_limit = DEFAULT_MAP_WINDOW_USER64;
+#elif defined(CONFIG_PPC_8xx)
+ init_mm.context.slb_addr_limit = DEFAULT_MAP_WINDOW;
#else
#error "context.addr_limit not initialized."
#endif
diff --git a/arch/powerpc/mm/8xx_mmu.c b/arch/powerpc/mm/8xx_mmu.c
index f29212e40f40..0be77709446c 100644
--- a/arch/powerpc/mm/8xx_mmu.c
+++ b/arch/powerpc/mm/8xx_mmu.c
@@ -192,7 +192,7 @@ void set_context(unsigned long id, pgd_t *pgd)
mtspr(SPRN_M_TW, __pa(pgd) - offset);
/* Update context */
- mtspr(SPRN_M_CASID, id);
+ mtspr(SPRN_M_CASID, id - 1);
/* sync */
mb();
}
diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index a9b9083c5e49..79e1378ee303 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c
@@ -553,9 +553,11 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
struct hstate *hstate = hstate_file(file);
int mmu_psize = shift_to_mmu_psize(huge_page_shift(hstate));
+#ifdef CONFIG_PPC_RADIX_MMU
if (radix_enabled())
return radix__hugetlb_get_unmapped_area(file, addr, len,
pgoff, flags);
+#endif
return slice_get_unmapped_area(addr, len, flags, mmu_psize, 1);
}
#endif
diff --git a/arch/powerpc/mm/mmu_context_nohash.c b/arch/powerpc/mm/mmu_context_nohash.c
index 4554d6527682..d98f7e5c141b 100644
--- a/arch/powerpc/mm/mmu_context_nohash.c
+++ b/arch/powerpc/mm/mmu_context_nohash.c
@@ -331,6 +331,20 @@ int init_new_context(struct task_struct *t, struct mm_struct *mm)
{
pr_hard("initing context for mm @%p\n", mm);
+#ifdef CONFIG_PPC_MM_SLICES
+ if (!mm->context.slb_addr_limit)
+ mm->context.slb_addr_limit = DEFAULT_MAP_WINDOW;
+
+ /*
+ * We have MMU_NO_CONTEXT set to be ~0. Hence check
+ * explicitly against context.id == 0. This ensures that we properly
+ * initialize context slice details for newly allocated mm's (which will
+ * have id == 0) and don't alter context slice inherited via fork (which
+ * will have id != 0).
+ */
+ if (mm->context.id == 0)
+ slice_set_user_psize(mm, mmu_virtual_psize);
+#endif
mm->context.id = MMU_NO_CONTEXT;
mm->context.active = 0;
return 0;
@@ -428,8 +442,8 @@ void __init mmu_context_init(void)
* -- BenH
*/
if (mmu_has_feature(MMU_FTR_TYPE_8xx)) {
- first_context = 0;
- last_context = 15;
+ first_context = 1;
+ last_context = 16;
no_selective_tlbil = true;
} else if (mmu_has_feature(MMU_FTR_TYPE_47x)) {
first_context = 1;
diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
index ae07470fde3c..73a7ea333e9e 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -334,6 +334,7 @@ config PPC_BOOK3E_MMU
config PPC_MM_SLICES
bool
default y if PPC_BOOK3S_64
+ default y if PPC_8xx && HUGETLB_PAGE
default n
config PPC_HAVE_PMU_SUPPORT
--
2.13.3
^ permalink raw reply related
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