* Re: [PATCH 1/3] sched: Fix nohz_kick_needed to consider the nr_busy of the parent domain's group
From: Peter Zijlstra @ 2013-10-22 22:11 UTC (permalink / raw)
To: Vaidyanathan Srinivasan
Cc: Michael Neuling, Mike Galbraith, linuxppc-dev, linux-kernel,
Anton Blanchard, Preeti U Murthy, Paul Turner, Ingo Molnar
In-Reply-To: <20131021114442.13291.99344.stgit@drishya>
On Mon, Oct 21, 2013 at 05:14:42PM +0530, Vaidyanathan Srinivasan wrote:
> kernel/sched/fair.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 7c70201..12f0eab 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5807,12 +5807,19 @@ static inline int nohz_kick_needed(struct rq *rq, int cpu)
>
> rcu_read_lock();
> for_each_domain(cpu, sd) {
> + struct sched_domain *sd_parent = sd->parent;
> + struct sched_group *sg;
> + struct sched_group_power *sgp;
> + int nr_busy;
> +
> + if (sd_parent) {
> + sg = sd_parent->groups;
> + sgp = sg->sgp;
> + nr_busy = atomic_read(&sgp->nr_busy_cpus);
> +
> + if (sd->flags & SD_SHARE_PKG_RESOURCES && nr_busy > 1)
> + goto need_kick_unlock;
> + }
>
> if (sd->flags & SD_ASYM_PACKING && nr_busy != sg->group_weight
> && (cpumask_first_and(nohz.idle_cpus_mask,
>
Almost I'd say; what happens on !sd_parent && SD_ASYM_PACKING ?
Also, this made me look at the nr_busy stuff again, and somehow that
entire thing makes me a little sad.
Can't we do something like the below and cut that nr_busy sd iteration
short?
This nohz stuff really needs to be re-thought and made more scalable --
its a royal pain :/
kernel/sched/core.c | 4 ++++
kernel/sched/fair.c | 21 +++++++++++++++------
kernel/sched/sched.h | 5 ++---
3 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index c06b8d3..89db8dc 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5271,6 +5271,7 @@ DEFINE_PER_CPU(struct sched_domain *, sd_llc);
DEFINE_PER_CPU(int, sd_llc_size);
DEFINE_PER_CPU(int, sd_llc_id);
DEFINE_PER_CPU(struct sched_domain *, sd_numa);
+DEFINE_PER_CPU(struct sched_domain *, sd_busy);
static void update_top_cache_domain(int cpu)
{
@@ -5290,6 +5291,9 @@ static void update_top_cache_domain(int cpu)
sd = lowest_flag_domain(cpu, SD_NUMA);
rcu_assign_pointer(per_cpu(sd_numa, cpu), sd);
+
+ sd = highest_flag_domain(cpu, SD_SHARE_PKG_RESOURCES | SD_ASYM_PACKING);
+ rcu_assign_pointer(per_cpu(sd_busy, cpu), sd);
}
/*
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 813dd61..3d5141e 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6512,19 +6512,23 @@ static inline void nohz_balance_exit_idle(int cpu)
}
}
-static inline void set_cpu_sd_state_busy(void)
+static inline void set_cpu_sd_state_busy(int cpu)
{
struct sched_domain *sd;
+ struct rq *rq = cpu_rq(cpu);
rcu_read_lock();
- sd = rcu_dereference_check_sched_domain(this_rq()->sd);
+ sd = rcu_dereference_check_sched_domain(rq->sd);
if (!sd || !sd->nohz_idle)
goto unlock;
sd->nohz_idle = 0;
- for (; sd; sd = sd->parent)
+ for (; sd; sd = sd->parent) {
atomic_inc(&sd->groups->sgp->nr_busy_cpus);
+ if (sd == per_cpu(sd_busy, cpu))
+ break;
+ }
unlock:
rcu_read_unlock();
}
@@ -6532,16 +6536,21 @@ static inline void set_cpu_sd_state_busy(void)
void set_cpu_sd_state_idle(void)
{
struct sched_domain *sd;
+ int cpu = smp_processor_id();
+ struct rq *rq = cpu_rq(cpu);
rcu_read_lock();
- sd = rcu_dereference_check_sched_domain(this_rq()->sd);
+ sd = rcu_dereference_check_sched_domain(rq->sd);
if (!sd || sd->nohz_idle)
goto unlock;
sd->nohz_idle = 1;
- for (; sd; sd = sd->parent)
+ for (; sd; sd = sd->parent) {
atomic_dec(&sd->groups->sgp->nr_busy_cpus);
+ if (sd == per_cpu(sd_busy, cpu))
+ break;
+ }
unlock:
rcu_read_unlock();
}
@@ -6756,7 +6765,7 @@ static inline int nohz_kick_needed(struct rq *rq, int cpu)
* We may be recently in ticked or tickless idle mode. At the first
* busy tick after returning from idle, we will update the busy stats.
*/
- set_cpu_sd_state_busy();
+ set_cpu_sd_state_busy(cpu);
nohz_balance_exit_idle(cpu);
/*
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index ffc7087..80c5fd2 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -599,9 +599,8 @@ static inline struct sched_domain *highest_flag_domain(int cpu, int flag)
struct sched_domain *sd, *hsd = NULL;
for_each_domain(cpu, sd) {
- if (!(sd->flags & flag))
- break;
- hsd = sd;
+ if (sd->flags & flag)
+ hsd = sd;
}
return hsd;
^ permalink raw reply related
* Re: [PATCH 2/3] sched: Fix asymmetric scheduling for POWER7
From: Peter Zijlstra @ 2013-10-22 22:18 UTC (permalink / raw)
To: Vaidyanathan Srinivasan
Cc: Michael Neuling, Mike Galbraith, linuxppc-dev, linux-kernel,
Anton Blanchard, Preeti U Murthy, Paul Turner, Ingo Molnar
In-Reply-To: <20131021114452.13291.19947.stgit@drishya>
On Mon, Oct 21, 2013 at 05:14:52PM +0530, Vaidyanathan Srinivasan wrote:
> kernel/sched/fair.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 12f0eab..828ed97 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5821,8 +5821,8 @@ static inline int nohz_kick_needed(struct rq *rq, int cpu)
> goto need_kick_unlock;
> }
>
> - if (sd->flags & SD_ASYM_PACKING && nr_busy != sg->group_weight
> - && (cpumask_first_and(nohz.idle_cpus_mask,
> + if (sd->flags & SD_ASYM_PACKING &&
> + (cpumask_first_and(nohz.idle_cpus_mask,
> sched_domain_span(sd)) < cpu))
> goto need_kick_unlock;
>
>
Ahh, so here you remove the nr_busy usage.. this patch should really go
before the first one that makes this all weird and funny.
^ permalink raw reply
* Re: [PATCH 3/3] sched: Aggressive balance in domains whose groups share package resources
From: Peter Zijlstra @ 2013-10-22 22:23 UTC (permalink / raw)
To: Vaidyanathan Srinivasan
Cc: Michael Neuling, Mike Galbraith, linuxppc-dev, linux-kernel,
Anton Blanchard, Preeti U Murthy, Paul Turner, Ingo Molnar
In-Reply-To: <20131021114502.13291.60794.stgit@drishya>
On Mon, Oct 21, 2013 at 05:15:02PM +0530, Vaidyanathan Srinivasan wrote:
> kernel/sched/fair.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 828ed97..bbcd96b 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5165,6 +5165,8 @@ static int load_balance(int this_cpu, struct rq *this_rq,
> {
> int ld_moved, cur_ld_moved, active_balance = 0;
> struct sched_group *group;
> + struct sched_domain *child;
> + int share_pkg_res = 0;
> struct rq *busiest;
> unsigned long flags;
> struct cpumask *cpus = __get_cpu_var(load_balance_mask);
> @@ -5190,6 +5192,10 @@ static int load_balance(int this_cpu, struct rq *this_rq,
>
> schedstat_inc(sd, lb_count[idle]);
>
> + child = sd->child;
> + if (child && child->flags & SD_SHARE_PKG_RESOURCES)
> + share_pkg_res = 1;
> +
> redo:
> if (!should_we_balance(&env)) {
> *continue_balancing = 0;
> @@ -5202,6 +5208,7 @@ redo:
> goto out_balanced;
> }
>
> +redo_grp:
> busiest = find_busiest_queue(&env, group);
> if (!busiest) {
> schedstat_inc(sd, lb_nobusyq[idle]);
> @@ -5292,6 +5299,11 @@ more_balance:
> if (!cpumask_empty(cpus)) {
> env.loop = 0;
> env.loop_break = sched_nr_migrate_break;
> + if (share_pkg_res &&
> + cpumask_intersects(cpus,
> + to_cpumask(group->cpumask)))
sched_group_cpus()
> + goto redo_grp;
> +
> goto redo;
> }
> goto out_balanced;
> @@ -5318,9 +5330,15 @@ more_balance:
> */
> if (!cpumask_test_cpu(this_cpu,
> tsk_cpus_allowed(busiest->curr))) {
> + cpumask_clear_cpu(cpu_of(busiest), cpus);
> raw_spin_unlock_irqrestore(&busiest->lock,
> flags);
> env.flags |= LBF_ALL_PINNED;
> + if (share_pkg_res &&
> + cpumask_intersects(cpus,
> + to_cpumask(group->cpumask)))
> + goto redo_grp;
> +
> goto out_one_pinned;
> }
Man this retry logic is getting annoying.. isn't there anything saner we
can do?
^ permalink raw reply
* perf events ring buffer memory barrier on powerpc
From: Michael Neuling @ 2013-10-22 23:54 UTC (permalink / raw)
To: Frederic Weisbecker, benh, anton, linux-kernel, Linux PPC dev,
Victor Kaplansky, Mathieu Desnoyers, michael
Frederic,
In the perf ring buffer code we have this in perf_output_get_handle():
if (!local_dec_and_test(&rb->nest))
goto out;
/*
* Publish the known good head. Rely on the full barrier implied
* by atomic_dec_and_test() order the rb->head read and this
* write.
*/
rb->user_page->data_head = head;
The comment says atomic_dec_and_test() but the code is
local_dec_and_test().
On powerpc, local_dec_and_test() doesn't have a memory barrier but
atomic_dec_and_test() does. Is the comment wrong, or is
local_dec_and_test() suppose to imply a memory barrier too and we have
it wrongly implemented in powerpc?
My guess is that local_dec_and_test() is correct but we to add an
explicit memory barrier like below:
(Kudos to Victor Kaplansky for finding this)
Mikey
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index cd55144..95768c6 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -87,10 +87,10 @@ again:
goto out;
/*
- * Publish the known good head. Rely on the full barrier implied
- * by atomic_dec_and_test() order the rb->head read and this
- * write.
+ * Publish the known good head. We need a memory barrier to order the
+ * order the rb->head read and this write.
*/
+ smp_mb ();
rb->user_page->data_head = head;
/*
^ permalink raw reply related
* Re: [PATCH 1/3] sched: Fix nohz_kick_needed to consider the nr_busy of the parent domain's group
From: Preeti U Murthy @ 2013-10-23 4:00 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Michael Neuling, Mike Galbraith, linuxppc-dev, linux-kernel,
Anton Blanchard, Paul Turner, Ingo Molnar
In-Reply-To: <20131022221138.GJ2490@laptop.programming.kicks-ass.net>
Hi Peter,
On 10/23/2013 03:41 AM, Peter Zijlstra wrote:
> On Mon, Oct 21, 2013 at 05:14:42PM +0530, Vaidyanathan Srinivasan wrote:
>> kernel/sched/fair.c | 19 +++++++++++++------
>> 1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 7c70201..12f0eab 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -5807,12 +5807,19 @@ static inline int nohz_kick_needed(struct rq *rq, int cpu)
>>
>> rcu_read_lock();
>> for_each_domain(cpu, sd) {
>> + struct sched_domain *sd_parent = sd->parent;
>> + struct sched_group *sg;
>> + struct sched_group_power *sgp;
>> + int nr_busy;
>> +
>> + if (sd_parent) {
>> + sg = sd_parent->groups;
>> + sgp = sg->sgp;
>> + nr_busy = atomic_read(&sgp->nr_busy_cpus);
>> +
>> + if (sd->flags & SD_SHARE_PKG_RESOURCES && nr_busy > 1)
>> + goto need_kick_unlock;
>> + }
>>
>> if (sd->flags & SD_ASYM_PACKING && nr_busy != sg->group_weight
>> && (cpumask_first_and(nohz.idle_cpus_mask,
>>
>
> Almost I'd say; what happens on !sd_parent && SD_ASYM_PACKING ?
You are right, sorry about this. The idea was to correct the nr_busy
computation before the patch that would remove its usage in the second
patch. But that would mean the condition nr_busy != sg->group_weight
would be invalid with this patch. The second patch needs to go first to
avoid this confusion.
>
> Also, this made me look at the nr_busy stuff again, and somehow that
> entire thing makes me a little sad.
>
> Can't we do something like the below and cut that nr_busy sd iteration
> short?
We can surely cut the nr_busy sd iteration but not like what is done
with this patch. You stop the nr_busy computation at the sched domain
that has the flag SD_SHARE_PKG_RESOURCES set. But nohz_kick_needed()
would want to know the nr_busy for one level above this.
Consider a core. Assume it is the highest domain with this flag set.
The nr_busy of its groups, which are logical threads are set to 1/0
each. But nohz_kick_needed() would like to know the sum of the nr_busy
parameter of all the groups, i.e. the threads in a core before it
decides if it can kick nohz_idle balancing. The information about the
individual group's nr_busy is of no relevance here.
Thats why the above patch tries to get the
sd->parent->groups->sgp->nr_busy_cpus. This will translate rightly to
the core's busy cpus in this example. But the below patch stops before
updating this parameter at the sd->parent level, where sd is the highest
level sched domain with the SD_SHARE_PKG_RESOURCES flag set.
But we can get around all this confusion if we can move the nr_busy
parameter to be included in the sched_domain structure rather than the
sched_groups_power structure. Anyway the only place where nr_busy is
used, that is at nohz_kick_needed(), is done to know the total number of
busy cpus at a sched domain level which has the SD_SHARE_PKG_RESOURCES
set and not at a sched group level.
So why not move nr_busy to struct sched_domain and having the below
patch which just updates this parameter for the sched domain, sd_busy ?
This will avoid iterating through all the levels of sched domains and
should resolve the scalability issue. We also don't need to get to
sd->parent to get the nr_busy parameter for the sake of nohz_kick_needed().
What do you think?
Regards
Preeti U Murthy
>
> This nohz stuff really needs to be re-thought and made more scalable --
> its a royal pain :/
>
>
> kernel/sched/core.c | 4 ++++
> kernel/sched/fair.c | 21 +++++++++++++++------
> kernel/sched/sched.h | 5 ++---
> 3 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index c06b8d3..89db8dc 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5271,6 +5271,7 @@ DEFINE_PER_CPU(struct sched_domain *, sd_llc);
> DEFINE_PER_CPU(int, sd_llc_size);
> DEFINE_PER_CPU(int, sd_llc_id);
> DEFINE_PER_CPU(struct sched_domain *, sd_numa);
> +DEFINE_PER_CPU(struct sched_domain *, sd_busy);
>
> static void update_top_cache_domain(int cpu)
> {
> @@ -5290,6 +5291,9 @@ static void update_top_cache_domain(int cpu)
>
> sd = lowest_flag_domain(cpu, SD_NUMA);
> rcu_assign_pointer(per_cpu(sd_numa, cpu), sd);
> +
> + sd = highest_flag_domain(cpu, SD_SHARE_PKG_RESOURCES | SD_ASYM_PACKING);
> + rcu_assign_pointer(per_cpu(sd_busy, cpu), sd);
> }
>
> /*
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 813dd61..3d5141e 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6512,19 +6512,23 @@ static inline void nohz_balance_exit_idle(int cpu)
> }
> }
>
> -static inline void set_cpu_sd_state_busy(void)
> +static inline void set_cpu_sd_state_busy(int cpu)
> {
> struct sched_domain *sd;
> + struct rq *rq = cpu_rq(cpu);
>
> rcu_read_lock();
> - sd = rcu_dereference_check_sched_domain(this_rq()->sd);
> + sd = rcu_dereference_check_sched_domain(rq->sd);
>
> if (!sd || !sd->nohz_idle)
> goto unlock;
> sd->nohz_idle = 0;
>
> - for (; sd; sd = sd->parent)
> + for (; sd; sd = sd->parent) {
> atomic_inc(&sd->groups->sgp->nr_busy_cpus);
> + if (sd == per_cpu(sd_busy, cpu))
> + break;
> + }
> unlock:
> rcu_read_unlock();
> }
> @@ -6532,16 +6536,21 @@ static inline void set_cpu_sd_state_busy(void)
> void set_cpu_sd_state_idle(void)
> {
> struct sched_domain *sd;
> + int cpu = smp_processor_id();
> + struct rq *rq = cpu_rq(cpu);
>
> rcu_read_lock();
> - sd = rcu_dereference_check_sched_domain(this_rq()->sd);
> + sd = rcu_dereference_check_sched_domain(rq->sd);
>
> if (!sd || sd->nohz_idle)
> goto unlock;
> sd->nohz_idle = 1;
>
> - for (; sd; sd = sd->parent)
> + for (; sd; sd = sd->parent) {
> atomic_dec(&sd->groups->sgp->nr_busy_cpus);
> + if (sd == per_cpu(sd_busy, cpu))
> + break;
> + }
> unlock:
> rcu_read_unlock();
> }
> @@ -6756,7 +6765,7 @@ static inline int nohz_kick_needed(struct rq *rq, int cpu)
> * We may be recently in ticked or tickless idle mode. At the first
> * busy tick after returning from idle, we will update the busy stats.
> */
> - set_cpu_sd_state_busy();
> + set_cpu_sd_state_busy(cpu);
> nohz_balance_exit_idle(cpu);
>
> /*
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index ffc7087..80c5fd2 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -599,9 +599,8 @@ static inline struct sched_domain *highest_flag_domain(int cpu, int flag)
> struct sched_domain *sd, *hsd = NULL;
>
> for_each_domain(cpu, sd) {
> - if (!(sd->flags & flag))
> - break;
> - hsd = sd;
> + if (sd->flags & flag)
> + hsd = sd;
> }
>
> return hsd;
>
^ permalink raw reply
* Re: [PATCH 1/3] sched: Fix nohz_kick_needed to consider the nr_busy of the parent domain's group
From: Preeti U Murthy @ 2013-10-23 4:21 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Michael Neuling, Mike Galbraith, linuxppc-dev, linux-kernel,
Anton Blanchard, Paul Turner, Ingo Molnar
In-Reply-To: <526749EC.9030005@linux.vnet.ibm.com>
On 10/23/2013 09:30 AM, Preeti U Murthy wrote:
> Hi Peter,
>
> On 10/23/2013 03:41 AM, Peter Zijlstra wrote:
>> On Mon, Oct 21, 2013 at 05:14:42PM +0530, Vaidyanathan Srinivasan wrote:
>>> kernel/sched/fair.c | 19 +++++++++++++------
>>> 1 file changed, 13 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>> index 7c70201..12f0eab 100644
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>> @@ -5807,12 +5807,19 @@ static inline int nohz_kick_needed(struct rq *rq, int cpu)
>>>
>>> rcu_read_lock();
>>> for_each_domain(cpu, sd) {
>>> + struct sched_domain *sd_parent = sd->parent;
>>> + struct sched_group *sg;
>>> + struct sched_group_power *sgp;
>>> + int nr_busy;
>>> +
>>> + if (sd_parent) {
>>> + sg = sd_parent->groups;
>>> + sgp = sg->sgp;
>>> + nr_busy = atomic_read(&sgp->nr_busy_cpus);
>>> +
>>> + if (sd->flags & SD_SHARE_PKG_RESOURCES && nr_busy > 1)
>>> + goto need_kick_unlock;
>>> + }
>>>
>>> if (sd->flags & SD_ASYM_PACKING && nr_busy != sg->group_weight
>>> && (cpumask_first_and(nohz.idle_cpus_mask,
>>>
>>
>> Almost I'd say; what happens on !sd_parent && SD_ASYM_PACKING ?
>
> You are right, sorry about this. The idea was to correct the nr_busy
> computation before the patch that would remove its usage in the second
> patch. But that would mean the condition nr_busy != sg->group_weight
> would be invalid with this patch. The second patch needs to go first to
> avoid this confusion.
>
>>
>> Also, this made me look at the nr_busy stuff again, and somehow that
>> entire thing makes me a little sad.
>>
>> Can't we do something like the below and cut that nr_busy sd iteration
>> short?
>
> We can surely cut the nr_busy sd iteration but not like what is done
> with this patch. You stop the nr_busy computation at the sched domain
> that has the flag SD_SHARE_PKG_RESOURCES set. But nohz_kick_needed()
> would want to know the nr_busy for one level above this.
> Consider a core. Assume it is the highest domain with this flag set.
> The nr_busy of its groups, which are logical threads are set to 1/0
> each. But nohz_kick_needed() would like to know the sum of the nr_busy
> parameter of all the groups, i.e. the threads in a core before it
> decides if it can kick nohz_idle balancing. The information about the
> individual group's nr_busy is of no relevance here.
>
> Thats why the above patch tries to get the
> sd->parent->groups->sgp->nr_busy_cpus. This will translate rightly to
> the core's busy cpus in this example. But the below patch stops before
> updating this parameter at the sd->parent level, where sd is the highest
> level sched domain with the SD_SHARE_PKG_RESOURCES flag set.
>
> But we can get around all this confusion if we can move the nr_busy
> parameter to be included in the sched_domain structure rather than the
> sched_groups_power structure. Anyway the only place where nr_busy is
> used, that is at nohz_kick_needed(), is done to know the total number of
> busy cpus at a sched domain level which has the SD_SHARE_PKG_RESOURCES
> set and not at a sched group level.
>
> So why not move nr_busy to struct sched_domain and having the below
> patch which just updates this parameter for the sched domain, sd_busy ?
Oh this can't be done :( Domain structures are per cpu!
Regards
Preeti U Murthy
^ permalink raw reply
* Re: [PATCH] [RFC] Emulate "lwsync" to run standard user land on e500 cores
From: Kumar Gala @ 2013-10-23 5:07 UTC (permalink / raw)
To: Wolfgang Denk; +Cc: Scott Wood, linuxppc-dev
In-Reply-To: <1382081880-6666-1-git-send-email-wd@denx.de>
On Oct 18, 2013, at 2:38 AM, Wolfgang Denk wrote:
> Default Debian PowerPC doesn't work on e500 because the code contains
> "lwsync" instructions, which are unsupported on this core. As a
> result, applications using this will crash with an "unhandled signal =
4"
> "Illegal instruction" error.
>=20
> As a work around we add code to emulate this insn. This is expensive
> performance-wise, but allows to run standard user land code.
>=20
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Scott Wood <scottwood@freescale.com>
> ---
> I am aware that the clean solution to the problem is to build user
> space with compiler options that match the target architecture.
> However, sometimes this is just too much effort.
>=20
> Also, of course the performance of such an emulation sucks. But the
> the occurrence of such instructions is so rare that no significant
> slowdown can be oserved.
>=20
> I'm not sure if this should / could go into mainline. I'm posting it
> primarily so it can be found should anybody else need this.
> - wd
>=20
> arch/powerpc/kernel/traps.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>=20
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index f783c93..f330374 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -986,6 +986,13 @@ static int emulate_instruction(struct pt_regs =
*regs)
> return 0;
> }
>=20
> + /* Emulating the lwsync insn as a sync insn */
> + if (instword =3D=3D PPC_INST_LWSYNC) {
> + PPC_WARN_EMULATED(lwsync, regs);
> + asm volatile("sync" : : : "memory");
Do we really need the inline asm? Doesn't the fact of just taking an =
exception and returning from it equate to a sync.
> + return 0;
> + }
> +
> /* Emulate the mcrxr insn. */
> if ((instword & PPC_INST_MCRXR_MASK) =3D=3D PPC_INST_MCRXR) {
> int shift =3D (instword >> 21) & 0x1c;
> --=20
> 1.8.3.1
>=20
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: Missing _restvr_20 and _savevr_20 subroutines for lib/raid6/altivec8.o
From: Kumar Gala @ 2013-10-23 5:28 UTC (permalink / raw)
To: Ben Hutchings
Cc: debian-powerpc, linuxppc-dev@lists.ozlabs.org list,
Anton Blanchard, Debian kernel maintainers
In-Reply-To: <1382221463.2794.87.camel@deadeye.wl.decadent.org.uk>
On Oct 19, 2013, at 5:24 PM, Ben Hutchings wrote:
> When building lib/raid6/altivec8.o with gcc 4.8 on Debian, the =
compiler
> is generating references to two new runtime subroutines which are
> apparently not included in the kernel:
>=20
> ERROR: "_restvr_20" [lib/raid6/raid6_pq.ko] undefined!
> ERROR: "_savevr_20" [lib/raid6/raid6_pq.ko] undefined!
>=20
> The save/restore subroutines are specified in
> =
http://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.7.1.html#SAVE=
-RESTORE
> and we do have the _restgpr_* and _savegpr_* subroutines in
> arch/powerpc/boot/crtsavres.S. I'm not sure whether these subroutines
> should be added or whether this indicates the compiler is doing
> something wrong.
>=20
> A configuration that triggers this is included below.
>=20
> Ben.
Try with CONFIG_CC_OPTIMIZE_FOR_SIZE=3Dn. A feature was added to gcc =
for -Os to "outline" the save/restore routines. I'm surprised this =
hasn't shown up sooner.
Well need to add _restvr_* / _savevr_* to the version in =
lib/crtsaveres.S.
=
http://gcc.gnu.org/git/?p=3Dgcc.git;a=3Dblob_plain;f=3Dlibgcc/config/rs600=
0/crtrestvr.S;hb=3DHEAD
=
http://gcc.gnu.org/git/?p=3Dgcc.git;a=3Dblob_plain;f=3Dlibgcc/config/rs600=
0/crtsavevr.S;hb=3DHEAD
- k=
^ permalink raw reply
* Re: perf events ring buffer memory barrier on powerpc
From: Victor Kaplansky @ 2013-10-23 7:39 UTC (permalink / raw)
To: Michael Neuling
Cc: Mathieu Desnoyers, linux-kernel, Linux PPC dev, anton,
Frederic Weisbecker
In-Reply-To: <12083.1382486094@ale.ozlabs.ibm.com>
See below.
Michael Neuling <mikey@neuling.org> wrote on 10/23/2013 02:54:54 AM:
>
> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> index cd55144..95768c6 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -87,10 +87,10 @@ again:
> goto out;
>
> /*
> - * Publish the known good head. Rely on the full barrier implied
> - * by atomic_dec_and_test() order the rb->head read and this
> - * write.
> + * Publish the known good head. We need a memory barrier to order the
> + * order the rb->head read and this write.
> */
> + smp_mb ();
> rb->user_page->data_head = head;
>
> /*
1. As far as I understand, smp_mb() is superfluous in this case, smp_wmb()
should be enough.
(same for the space between the name of function and open
parenthesis :-) )
2. Again, as far as I understand from ./Documentation/atomic_ops.txt, it is
mistake in architecture independent
code to rely on memory barriers in atomic operations, all the more so in
"local" operations.
3. The solution above is sub-optimal on architectures where memory barrier
is part of "local", since we are going to execute
two consecutive barriers. So, maybe, it would be better to use
smp_mb__after_atomic_dec().
4. I'm not sure, but I think there is another, unrelated potential problem
in function perf_output_put_handle()
- the write to "data_head" -
kernel/events/ring_buffer.c:
77 /*
78 * Publish the known good head. Rely on the full barrier
implied
79 * by atomic_dec_and_test() order the rb->head read and this
80 * write.
81 */
82 rb->user_page->data_head = head;
As data_head is 64-bit wide, the update should be done by an atomic64_set
().
Regards,
-- Victor
^ permalink raw reply
* [PATCH] powerpc: Don't corrupt user registers on 32-bit
From: Paul Mackerras @ 2013-10-23 8:40 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Scott Wood, linuxppc-dev, Alexander Graf
Commit de79f7b9f6 ("powerpc: Put FP/VSX and VR state into structures")
modified load_up_fpu() and load_up_altivec() in such a way that they
now use r7 and r8. Unfortunately, the callers of these functions on
32-bit machines then return to userspace via fast_exception_return,
which doesn't restore all of the volatile GPRs, but only r1, r3 -- r6
and r9 -- r12. This was causing userspace segfaults and other
userspace misbehaviour on 32-bit machines.
This fixes the problem by changing the register usage of load_up_fpu()
and load_up_altivec() to avoid using r7 and r8 and instead use r6 and
r10. This also adds comments to those functions saying which registers
may be used.
Signed-off-by: Paul Mackerras <paulus@samba.org>
---
arch/powerpc/kernel/fpu.S | 14 ++++++++------
arch/powerpc/kernel/vector.S | 15 +++++++++------
2 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/kernel/fpu.S b/arch/powerpc/kernel/fpu.S
index 4dca05e..f7f5b8b 100644
--- a/arch/powerpc/kernel/fpu.S
+++ b/arch/powerpc/kernel/fpu.S
@@ -106,6 +106,8 @@ _GLOBAL(store_fp_state)
* and save its floating-point registers in its thread_struct.
* Load up this task's FP registers from its thread_struct,
* enable the FPU for the current task and return to the task.
+ * Note that on 32-bit this can only use registers that will be
+ * restored by fast_exception_return, i.e. r3 - r6, r10 and r11.
*/
_GLOBAL(load_up_fpu)
mfmsr r5
@@ -131,10 +133,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_VSX)
beq 1f
toreal(r4)
addi r4,r4,THREAD /* want last_task_used_math->thread */
- addi r8,r4,THREAD_FPSTATE
- SAVE_32FPVSRS(0, R5, R8)
+ addi r10,r4,THREAD_FPSTATE
+ SAVE_32FPVSRS(0, R5, R10)
mffs fr0
- stfd fr0,FPSTATE_FPSCR(r8)
+ stfd fr0,FPSTATE_FPSCR(r10)
PPC_LL r5,PT_REGS(r4)
toreal(r5)
PPC_LL r4,_MSR-STACK_FRAME_OVERHEAD(r5)
@@ -157,10 +159,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_VSX)
or r12,r12,r4
std r12,_MSR(r1)
#endif
- addi r7,r5,THREAD_FPSTATE
- lfd fr0,FPSTATE_FPSCR(r7)
+ addi r10,r5,THREAD_FPSTATE
+ lfd fr0,FPSTATE_FPSCR(r10)
MTFSF_L(fr0)
- REST_32FPVSRS(0, R4, R7)
+ REST_32FPVSRS(0, R4, R10)
#ifndef CONFIG_SMP
subi r4,r5,THREAD
fromreal(r4)
diff --git a/arch/powerpc/kernel/vector.S b/arch/powerpc/kernel/vector.S
index eacda4e..0458a9a 100644
--- a/arch/powerpc/kernel/vector.S
+++ b/arch/powerpc/kernel/vector.S
@@ -64,6 +64,9 @@ _GLOBAL(store_vr_state)
* Enables the VMX for use in the kernel on return.
* On SMP we know the VMX is free, since we give it up every
* switch (ie, no lazy save of the vector registers).
+ *
+ * Note that on 32-bit this can only use registers that will be
+ * restored by fast_exception_return, i.e. r3 - r6, r10 and r11.
*/
_GLOBAL(load_up_altivec)
mfmsr r5 /* grab the current MSR */
@@ -89,11 +92,11 @@ _GLOBAL(load_up_altivec)
/* Save VMX state to last_task_used_altivec's THREAD struct */
toreal(r4)
addi r4,r4,THREAD
- addi r7,r4,THREAD_VRSTATE
- SAVE_32VRS(0,r5,r7)
+ addi r6,r4,THREAD_VRSTATE
+ SAVE_32VRS(0,r5,r6)
mfvscr vr0
li r10,VRSTATE_VSCR
- stvx vr0,r10,r7
+ stvx vr0,r10,r6
/* Disable VMX for last_task_used_altivec */
PPC_LL r5,PT_REGS(r4)
toreal(r5)
@@ -125,13 +128,13 @@ _GLOBAL(load_up_altivec)
oris r12,r12,MSR_VEC@h
std r12,_MSR(r1)
#endif
- addi r7,r5,THREAD_VRSTATE
+ addi r6,r5,THREAD_VRSTATE
li r4,1
li r10,VRSTATE_VSCR
stw r4,THREAD_USED_VR(r5)
- lvx vr0,r10,r7
+ lvx vr0,r10,r6
mtvscr vr0
- REST_32VRS(0,r4,r7)
+ REST_32VRS(0,r4,r6)
#ifndef CONFIG_SMP
/* Update last_task_used_altivec to 'current' */
subi r4,r5,THREAD /* Back to 'current' */
--
1.8.4.rc3
^ permalink raw reply related
* Re: [v5][PATCH 1/6] powerpc/book3e: load critical/machine/debug exception stack
From: "“tiejun.chen”" @ 2013-10-23 9:26 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1382135824.7979.906.camel@snotra.buserror.net>
On 10/19/2013 06:37 AM, Scott Wood wrote:
> On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
>> We always alloc critical/machine/debug check exceptions. This is
>> different from the normal exception. So we should load these exception
>> stack properly like we did for booke.
>
> This is "booke". Do you mean like "like we did for 32-bit"?
Yes.
>
> And the code is already trying to load the special stack; it just
> happens that it's loading from a different location than the C code
> placed the stack addresses. The changelog should point out the specific
> thing that is being fixed.
Here I don't fix anything, and I just want to do the same thing as 32-bit.
>
>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
>> ---
>> arch/powerpc/kernel/exceptions-64e.S | 49 +++++++++++++++++++++++++++++++---
>> 1 file changed, 46 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
>> index 4b23119..4d8e57f 100644
>> --- a/arch/powerpc/kernel/exceptions-64e.S
>> +++ b/arch/powerpc/kernel/exceptions-64e.S
>> @@ -36,6 +36,37 @@
>> */
>> #define SPECIAL_EXC_FRAME_SIZE INT_FRAME_SIZE
>>
>> +/* only on book3e */
>> +#define DBG_STACK_BASE dbgirq_ctx
>> +#define MC_STACK_BASE mcheckirq_ctx
>> +#define CRIT_STACK_BASE critirq_ctx
>> +
>> +#ifdef CONFIG_RELOCATABLE
>> +#define LOAD_STACK_BASE(reg, level) \
>> + tovirt(r2,r2); \
>> + LOAD_REG_ADDR(reg, level##_STACK_BASE);
>> +#else
>> +#define LOAD_STACK_BASE(reg, level) \
>> + LOAD_REG_IMMEDIATE(reg, level##_STACK_BASE);
>> +#endif
>> +
>> +#ifdef CONFIG_SMP
>> +#define BOOK3E_LOAD_EXC_LEVEL_STACK(level) \
>> + mfspr r14,SPRN_PIR; \
>> + slwi r14,r14,3; \
>> + LOAD_STACK_BASE(r10, level); \
>> + add r10,r10,r14; \
>> + ld r10,0(r10); \
>> + addi r10,r10,THREAD_SIZE; \
>> + std r10,PACA_##level##_STACK(r13);
>> +#else
>> +#define BOOK3E_LOAD_EXC_LEVEL_STACK(level) \
>> + LOAD_STACK_BASE(r10, level); \
>> + ld r10,0(r10); \
>> + addi r10,r10,THREAD_SIZE; \
>> + std r10,PACA_##level##_STACK(r13);
>> +#endif
>
> It looks like you're loading the stack from *irq_ctx, storing it in
> PACA_*_stack, and then (immediately after this in the caller) loading it
> back from PACA_*_STACK. Why not just load it from *irq_ctx and get rid
> of PACA_*_STACK altogether -- or change the C code to initialize the
> addresses in the PACA instead, and get ird of *irq_ctx on 64-bit?
Okay, I'd like to move forward the c code, please see next version.
>
>> /* Exception prolog code for all exceptions */
>> #define EXCEPTION_PROLOG(n, intnum, type, addition) \
>> mtspr SPRN_SPRG_##type##_SCRATCH,r13; /* get spare registers */ \
>> @@ -68,20 +99,32 @@
>> #define SPRN_GDBELL_SRR1 SPRN_GSRR1
>>
>> #define CRIT_SET_KSTACK \
>> + andi. r10,r11,MSR_PR; \
>> + bne 1f; \
>> + BOOK3E_LOAD_EXC_LEVEL_STACK(CRIT); \
>> ld r1,PACA_CRIT_STACK(r13); \
>> - subi r1,r1,SPECIAL_EXC_FRAME_SIZE;
>> + subi r1,r1,SPECIAL_EXC_FRAME_SIZE; \
>
> The caller will already check MSR_PR and override this if coming from
> userspace; why do you need to check again here?
Looks this is redundant so this will be left out.
Thanks,
Tiejun
^ permalink raw reply
* Re: [v5][PATCH 2/6] powerpc/book3e: store critical/machine/debug exception thread info
From: "“tiejun.chen”" @ 2013-10-23 9:27 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1382136213.7979.908.camel@snotra.buserror.net>
On 10/19/2013 06:43 AM, Scott Wood wrote:
> On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
>> We need to store thread info to these exception thread info like something
>> we already did for PPC32.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
>> ---
>> arch/powerpc/kernel/exceptions-64e.S | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
>> index 4d8e57f..07cf657 100644
>> --- a/arch/powerpc/kernel/exceptions-64e.S
>> +++ b/arch/powerpc/kernel/exceptions-64e.S
>> @@ -67,6 +67,18 @@
>> std r10,PACA_##level##_STACK(r13);
>> #endif
>>
>> +/* Store something to exception thread info */
>> +#define BOOK3E_STORE_EXC_LEVEL_THEAD_INFO(type) \
>> + ld r14,PACAKSAVE(r13); \
>> + CURRENT_THREAD_INFO(r14, r14); \
>> + CURRENT_THREAD_INFO(r15, r1); \
>> + ld r10,TI_FLAGS(r14); \
>> + std r10,TI_FLAGS(r15); \
>> + ld r10,TI_PREEMPT(r14); \
>> + std r10,TI_PREEMPT(r15); \
>> + ld r10,TI_TASK(r14); \
>> + std r10,TI_TASK(r15);
>
> Where is "type" used?
>
Yes, its noting now but its worth leaving this to extend something in the future.
> BTW, no need for a BOOK3E prefix for things local to this file.
>
What about "EXC_LEVEL_EXCEPTION_PROLOG"? Please see next version.
Thanks,
Tiejun
^ permalink raw reply
* Re: [v5][PATCH 3/6] book3e/kgdb: update thread's dbcr0
From: "“tiejun.chen”" @ 2013-10-23 9:27 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1382137030.7979.914.camel@snotra.buserror.net>
On 10/19/2013 06:57 AM, Scott Wood wrote:
> On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
>> gdb always need to generate a single step properly to invoke
>> a kgdb state. But with lazy interrupt, book3e can't always
>> trigger a debug exception with a single step since the current
>> is blocked for handling those pending exception, then we miss
>> that expected dbcr configuration at last to generate a debug
>> exception.
>
> What do you mean by "the current is blocked"? Could you explain more
> clearly what lazy EE has to do with MSR_DE and DBCR0?
>
I will go another path to make sure the lazy EE doesn't affect KGDB, so please
see next version.
Thanks,
Tiejun
^ permalink raw reply
* Re: [v5][PATCH 4/6] powerpc/book3e: support kgdb for kernel space
From: "“tiejun.chen”" @ 2013-10-23 9:27 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1382137119.7979.916.camel@snotra.buserror.net>
On 10/19/2013 06:58 AM, Scott Wood wrote:
> On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
>> Currently we need to skip this for supporting KGDB.
>
> Does it need to depend on CONFIG_KGDB? Either you've fixed the "can't
> quite save properly" part, or you haven't.
I'm not 100% sure if my change is fine to other scenarios so I have to use this
guarantee other stuff are still safe. But if you think we can remove this
dependent, I'm happy to do :)
Thanks,
Tiejun
^ permalink raw reply
* Re: [v5][PATCH 6/6] book3e/kgdb: Fix a single stgep case of lazy IRQ
From: "“tiejun.chen”" @ 2013-10-23 9:28 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1382139147.7979.934.camel@snotra.buserror.net>
On 10/19/2013 07:32 AM, Scott Wood wrote:
> On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
>> When we're in kgdb_singlestep(), we have to work around to get
>> thread_info by copying from the kernel stack before calling
>> kgdb_handle_exception(), then copying it back afterwards.
>>
>> But for PPC64, we have a lazy interrupt implementation. So after
>> copying thread info frome kernle stack, if we need to replay an
>> interrupt, we shouldn't restore that previous backup thread info
>> to make sure we can replay an interrupt lately with a proper
>> thread info.
>
> Explain why copying it would be a problem.
>
This would be gone away in next version as well :)
Thanks,
Tiejun
^ permalink raw reply
* Re: [v5][PATCH 1/6] powerpc/book3e: load critical/machine/debug exception stack
From: "“tiejun.chen”" @ 2013-10-23 9:28 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1382140519.7979.936.camel@snotra.buserror.net>
On 10/19/2013 07:55 AM, Scott Wood wrote:
> On Thu, 2013-06-20 at 18:28 +0800, Tiejun Chen wrote:
>> We always alloc critical/machine/debug check exceptions. This is
>> different from the normal exception. So we should load these exception
>> stack properly like we did for booke.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
>> ---
>> arch/powerpc/kernel/exceptions-64e.S | 49 +++++++++++++++++++++++++++++++---
>> 1 file changed, 46 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
>> index 4b23119..4d8e57f 100644
>> --- a/arch/powerpc/kernel/exceptions-64e.S
>> +++ b/arch/powerpc/kernel/exceptions-64e.S
>> @@ -36,6 +36,37 @@
>> */
>> #define SPECIAL_EXC_FRAME_SIZE INT_FRAME_SIZE
>>
>> +/* only on book3e */
>> +#define DBG_STACK_BASE dbgirq_ctx
>> +#define MC_STACK_BASE mcheckirq_ctx
>> +#define CRIT_STACK_BASE critirq_ctx
>> +
>> +#ifdef CONFIG_RELOCATABLE
>> +#define LOAD_STACK_BASE(reg, level) \
>> + tovirt(r2,r2); \
>> + LOAD_REG_ADDR(reg, level##_STACK_BASE);
>
> Where does r2 come from here, where does it get used, and why do we need
> tovirt() on book3e?
>
As I remember this should be covered when we boot that capture kernel in
kexec/kdump case.
Now this is also gone away after move forward the c code.
Thanks,
Tiejun
^ permalink raw reply
* [v6][PATCH 0/5] powerpc/book3e: powerpc/book3e: make kgdb to work well
From: Tiejun Chen @ 2013-10-23 9:31 UTC (permalink / raw)
To: scottwood; +Cc: linuxppc-dev, linux-kernel
Scott,
Tested on fsl-p5040 DS.
v6:
* rebase
* change the C code to initialize the exception stack addresses in the PACA instead.
* Clear the PACA_IRQ_HARD_DIS force to exit directly from this debug exception
without replaying interrupt.
* so drop "book3e/kgdb: update thread's dbcr0".
v5:
* rebase on merge branch.
Note the original patch, [ATCH 5/7] kgdb/kgdbts: support ppc64, is already merged
by Jason.
v4:
* use DEFINE_PER_CPU to allocate kgdb's thread_info
* add patch 7 to make usre copy thread_info only !__check_irq_replay
* leave "andi. r14,r11,MSR_PR" out of "#ifndef CONFIG_KGDB"
since cr0 is still used lately.
* retest
v3:
* make work when enable CONFIG_RELOCATABLE
* fix one typo in patch,
"powerpc/book3e: store critical/machine/debug exception thread info":
ld r1,PACAKSAVE(r13);
-> ld r14,PACAKSAVE(r13);
* remove copying the thread_info since booke and book3e always copy
the thead_info now when we enter the debug exception, and so drop
the v2 patch, "book3e/kgdb: Fix a single stgep case of lazy IRQ"
v2:
* Make sure we cover CONFIG_PPC_BOOK3E_64 safely
* Use LOAD_REG_IMMEDIATE() to load properly
the value of the constant expression in load debug exception stack
* Copy thread infor form the kernel stack coming from usr
* Rebase latest powerpc git tree
v1:
* Copy thread info only when we are from !user mode since we'll get kernel stack
coming from usr directly.
* remove save/restore EX_R14/EX_R15 since DBG_EXCEPTION_PROLOG already covered
this.
* use CURRENT_THREAD_INFO() conveniently to get thread.
* fix some typos
* add a patch to make sure gdb can generate a single step properly to invoke a
kgdb state.
* add a patch to if we need to replay an interrupt, we shouldn't restore that
previous backup thread info to make sure we can replay an interrupt lately
with a proper thread info.
* rebase latest powerpc git tree
v0:
This patchset is used to support kgdb for book3e.
----------------------------------------------------------------
Tiejun Chen (5):
powerpc/book3e: initialize crit/mc/dbg kernel stack pointers
powerpc/book3e: store crit/mc/dbg exception thread info
powerpc/book3e: support kgdb for kernel space
powerpc/kgdb: use DEFINE_PER_CPU to allocate kgdb's thread_info
powerpc/book3e/kgdb: Fix a single stgep case of lazy IRQ
arch/powerpc/kernel/exceptions-64e.S | 26 ++++++++++++++++++++++----
arch/powerpc/kernel/kgdb.c | 13 ++++++++++---
arch/powerpc/kernel/setup_64.c | 18 ++++++++++++------
3 files changed, 44 insertions(+), 13 deletions(-)
Tiejun
^ permalink raw reply
* [v6][PATCH 1/5] powerpc/book3e: initialize crit/mc/dbg kernel stack pointers
From: Tiejun Chen @ 2013-10-23 9:31 UTC (permalink / raw)
To: scottwood; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1382520685-11609-1-git-send-email-tiejun.chen@windriver.com>
We already allocated critical/machine/debug check exceptions, but
we also should initialize those associated kernel stack pointers
for use by special exceptions in the PACA.
Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
arch/powerpc/kernel/setup_64.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 278ca93..5c96d92 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -526,14 +526,20 @@ static void __init exc_lvl_early_init(void)
extern unsigned int exc_debug_debug_book3e;
unsigned int i;
+ unsigned long sp;
for_each_possible_cpu(i) {
- critirq_ctx[i] = (struct thread_info *)
- __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
- dbgirq_ctx[i] = (struct thread_info *)
- __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
- mcheckirq_ctx[i] = (struct thread_info *)
- __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
+ sp = memblock_alloc(THREAD_SIZE, THREAD_SIZE);
+ critirq_ctx[i] = (struct thread_info *)__va(sp);
+ paca[i].crit_kstack = __va(sp + THREAD_SIZE);
+
+ sp = memblock_alloc(THREAD_SIZE, THREAD_SIZE);
+ dbgirq_ctx[i] = (struct thread_info *)__va(sp);
+ paca[i].dbg_kstack = __va(sp + THREAD_SIZE);
+
+ sp = memblock_alloc(THREAD_SIZE, THREAD_SIZE);
+ mcheckirq_ctx[i] = (struct thread_info *)__va(sp);
+ paca[i].mc_kstack = __va(sp + THREAD_SIZE);
}
if (cpu_has_feature(CPU_FTR_DEBUG_LVL_EXC))
--
1.7.9.5
^ permalink raw reply related
* [v6][PATCH 2/5] powerpc/book3e: store crit/mc/dbg exception thread info
From: Tiejun Chen @ 2013-10-23 9:31 UTC (permalink / raw)
To: scottwood; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1382520685-11609-1-git-send-email-tiejun.chen@windriver.com>
We need to store thread info to these exception thread info like something
we already did for PPC32.
Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
arch/powerpc/kernel/exceptions-64e.S | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index 68d74b4..a55cf62 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -36,6 +36,19 @@
*/
#define SPECIAL_EXC_FRAME_SIZE INT_FRAME_SIZE
+/* Now we only store something to exception thread info */
+#define EXC_LEVEL_EXCEPTION_PROLOG(type) \
+ ld r14,PACAKSAVE(r13); \
+ CURRENT_THREAD_INFO(r14, r14); \
+ CURRENT_THREAD_INFO(r15, r1); \
+ ld r10,TI_FLAGS(r14); \
+ std r10,TI_FLAGS(r15); \
+ ld r10,TI_PREEMPT(r14); \
+ std r10,TI_PREEMPT(r15); \
+ ld r10,TI_TASK(r14); \
+ std r10,TI_TASK(r15);
+
+
/* Exception prolog code for all exceptions */
#define EXCEPTION_PROLOG(n, intnum, type, addition) \
mtspr SPRN_SPRG_##type##_SCRATCH,r13; /* get spare registers */ \
@@ -69,19 +82,22 @@
#define CRIT_SET_KSTACK \
ld r1,PACA_CRIT_STACK(r13); \
- subi r1,r1,SPECIAL_EXC_FRAME_SIZE;
+ subi r1,r1,SPECIAL_EXC_FRAME_SIZE; \
+ EXC_LEVEL_EXCEPTION_PROLOG(CRIT);
#define SPRN_CRIT_SRR0 SPRN_CSRR0
#define SPRN_CRIT_SRR1 SPRN_CSRR1
#define DBG_SET_KSTACK \
ld r1,PACA_DBG_STACK(r13); \
- subi r1,r1,SPECIAL_EXC_FRAME_SIZE;
+ subi r1,r1,SPECIAL_EXC_FRAME_SIZE; \
+ EXC_LEVEL_EXCEPTION_PROLOG(DBG);
#define SPRN_DBG_SRR0 SPRN_DSRR0
#define SPRN_DBG_SRR1 SPRN_DSRR1
#define MC_SET_KSTACK \
ld r1,PACA_MC_STACK(r13); \
- subi r1,r1,SPECIAL_EXC_FRAME_SIZE;
+ subi r1,r1,SPECIAL_EXC_FRAME_SIZE; \
+ EXC_LEVEL_EXCEPTION_PROLOG(MC);
#define SPRN_MC_SRR0 SPRN_MCSRR0
#define SPRN_MC_SRR1 SPRN_MCSRR1
--
1.7.9.5
^ permalink raw reply related
* [v6][PATCH 3/5] powerpc/book3e: support kgdb for kernel space
From: Tiejun Chen @ 2013-10-23 9:31 UTC (permalink / raw)
To: scottwood; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1382520685-11609-1-git-send-email-tiejun.chen@windriver.com>
Currently we need to skip this for supporting KGDB.
Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
arch/powerpc/kernel/exceptions-64e.S | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index a55cf62..0b750c6 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -597,11 +597,13 @@ kernel_dbg_exc:
rfdi
/* Normal debug exception */
+1: andi. r14,r11,MSR_PR; /* check for userspace again */
+#ifndef CONFIG_KGDB
/* XXX We only handle coming from userspace for now since we can't
* quite save properly an interrupted kernel state yet
*/
-1: andi. r14,r11,MSR_PR; /* check for userspace again */
beq kernel_dbg_exc; /* if from kernel mode */
+#endif
/* Now we mash up things to make it look like we are coming on a
* normal exception
--
1.7.9.5
^ permalink raw reply related
* [v6][PATCH 4/5] powerpc/kgdb: use DEFINE_PER_CPU to allocate kgdb's thread_info
From: Tiejun Chen @ 2013-10-23 9:31 UTC (permalink / raw)
To: scottwood; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1382520685-11609-1-git-send-email-tiejun.chen@windriver.com>
Use DEFINE_PER_CPU to allocate thread_info statically instead of kmalloc().
This can avoid introducing more memory check codes.
Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
arch/powerpc/kernel/kgdb.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/kgdb.c b/arch/powerpc/kernel/kgdb.c
index c1eef24..447c14b 100644
--- a/arch/powerpc/kernel/kgdb.c
+++ b/arch/powerpc/kernel/kgdb.c
@@ -151,15 +151,15 @@ static int kgdb_handle_breakpoint(struct pt_regs *regs)
return 1;
}
+static DEFINE_PER_CPU(struct thread_info, kgdb_thread_info);
static int kgdb_singlestep(struct pt_regs *regs)
{
struct thread_info *thread_info, *exception_thread_info;
- struct thread_info *backup_current_thread_info;
+ struct thread_info *backup_current_thread_info = &__get_cpu_var(kgdb_thread_info);
if (user_mode(regs))
return 0;
- backup_current_thread_info = kmalloc(sizeof(struct thread_info), GFP_KERNEL);
/*
* On Book E and perhaps other processors, singlestep is handled on
* the critical exception stack. This causes current_thread_info()
@@ -185,7 +185,6 @@ static int kgdb_singlestep(struct pt_regs *regs)
/* Restore current_thread_info lastly. */
memcpy(exception_thread_info, backup_current_thread_info, sizeof *thread_info);
- kfree(backup_current_thread_info);
return 1;
}
--
1.7.9.5
^ permalink raw reply related
* [v6][PATCH 5/5] powerpc/book3e/kgdb: Fix a single stgep case of lazy IRQ
From: Tiejun Chen @ 2013-10-23 9:31 UTC (permalink / raw)
To: scottwood; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1382520685-11609-1-git-send-email-tiejun.chen@windriver.com>
In lazy EE magic, we may have a lazy interrupt occured while
entering kgdb, but we really don't want to replay that interrupt
for kgdb, so we have to clear the PACA_IRQ_HARD_DIS force to
make sure we can exit directly from this debug exception.
Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
arch/powerpc/kernel/kgdb.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/powerpc/kernel/kgdb.c b/arch/powerpc/kernel/kgdb.c
index 447c14b..9872f58 100644
--- a/arch/powerpc/kernel/kgdb.c
+++ b/arch/powerpc/kernel/kgdb.c
@@ -185,6 +185,14 @@ static int kgdb_singlestep(struct pt_regs *regs)
/* Restore current_thread_info lastly. */
memcpy(exception_thread_info, backup_current_thread_info, sizeof *thread_info);
+#ifdef CONFIG_PPC64
+ /*
+ * Clear the PACA_IRQ_HARD_DIS from the pending mask
+ * since we are about to exit this directly from debug
+ * exception without any replay interrupt in lazy EE case.
+ */
+ local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
+#endif
return 1;
}
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 1/3] sched: Fix nohz_kick_needed to consider the nr_busy of the parent domain's group
From: Preeti U Murthy @ 2013-10-23 9:50 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Michael Neuling, Mike Galbraith, linuxppc-dev, linux-kernel,
Anton Blanchard, Paul Turner, Ingo Molnar
In-Reply-To: <20131022221138.GJ2490@laptop.programming.kicks-ass.net>
Hi Peter
On 10/23/2013 03:41 AM, Peter Zijlstra wrote:
> This nohz stuff really needs to be re-thought and made more scalable --
> its a royal pain :/
Why not do something like the below instead? It does the following.
This patch introduces sd_busy just like your suggested patch, except that
it points to the parent of the highest level sched domain which has the
SD_SHARE_PKG_RESOURCES set and initializes it in update_top_cache_domain().
This is the sched domain that is relevant in nohz_kick_needed().
sd_set_sd_state_busy(), sd_set_sd_state_idle() and nohz_kick_needed() query
and update *only* this sched domain(sd_busy) for nr_busy_cpus. They are the
only users of this parameter. While we are at it, we might as well change
the nohz_idle parameter to be updated at the sd_busy domain level alone and
not the base domain level of a CPU. This will unify the concept of busy cpus
at just one level of sched domain.
There is no need to iterate through all levels of sched domains of a cpu to
update nr_busy_cpus since it is irrelevant at all other sched domains except
at sd_busy level.
De-couple asymmetric load balancing from the nr_busy parameter which the
PATCH 2/3 anyway does. sd_busy therefore is irrelevant for asymmetric load
balancing.
Regards
Preeti U Murthy
--------------------START_PATCH-------------------------------
sched: Fix nohz_kick_needed()
---
kernel/sched/core.c | 4 ++++
kernel/sched/fair.c | 40 ++++++++++++++++++++++------------------
kernel/sched/sched.h | 1 +
3 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index c06b8d3..c1dd11c 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5271,6 +5271,7 @@ DEFINE_PER_CPU(struct sched_domain *, sd_llc);
DEFINE_PER_CPU(int, sd_llc_size);
DEFINE_PER_CPU(int, sd_llc_id);
DEFINE_PER_CPU(struct sched_domain *, sd_numa);
+DEFINE_PER_CPU(struct sched_domain *, sd_busy);
static void update_top_cache_domain(int cpu)
{
@@ -5290,6 +5291,9 @@ static void update_top_cache_domain(int cpu)
sd = lowest_flag_domain(cpu, SD_NUMA);
rcu_assign_pointer(per_cpu(sd_numa, cpu), sd);
+
+ sd = highest_flag_domain(cpu, SD_SHARE_PKG_RESOURCES)->parent;
+ rcu_assign_pointer(per_cpu(sd_busy, cpu), sd);
}
/*
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 813dd61..71e6f14 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6515,16 +6515,16 @@ static inline void nohz_balance_exit_idle(int cpu)
static inline void set_cpu_sd_state_busy(void)
{
struct sched_domain *sd;
+ int cpu = smp_processor_id();
rcu_read_lock();
- sd = rcu_dereference_check_sched_domain(this_rq()->sd);
+ sd = per_cpu(sd_busy, cpu);
if (!sd || !sd->nohz_idle)
goto unlock;
sd->nohz_idle = 0;
- for (; sd; sd = sd->parent)
- atomic_inc(&sd->groups->sgp->nr_busy_cpus);
+ atomic_inc(&sd->groups->sgp->nr_busy_cpus);
unlock:
rcu_read_unlock();
}
@@ -6532,16 +6532,16 @@ unlock:
void set_cpu_sd_state_idle(void)
{
struct sched_domain *sd;
+ int cpu = smp_processor_id();
rcu_read_lock();
- sd = rcu_dereference_check_sched_domain(this_rq()->sd);
+ sd = per_cpu(sd_busy, cpu);
if (!sd || sd->nohz_idle)
goto unlock;
sd->nohz_idle = 1;
- for (; sd; sd = sd->parent)
- atomic_dec(&sd->groups->sgp->nr_busy_cpus);
+ atomic_dec(&sd->groups->sgp->nr_busy_cpus);
unlock:
rcu_read_unlock();
}
@@ -6748,6 +6748,9 @@ static inline int nohz_kick_needed(struct rq *rq, int cpu)
{
unsigned long now = jiffies;
struct sched_domain *sd;
+ struct sched_group *sg;
+ struct sched_group_power *sgp;
+ int nr_busy;
if (unlikely(idle_cpu(cpu)))
return 0;
@@ -6773,22 +6776,23 @@ static inline int nohz_kick_needed(struct rq *rq, int cpu)
goto need_kick;
rcu_read_lock();
- for_each_domain(cpu, sd) {
- struct sched_group *sg = sd->groups;
- struct sched_group_power *sgp = sg->sgp;
- int nr_busy = atomic_read(&sgp->nr_busy_cpus);
+ sd = per_cpu(sd_busy, cpu);
- if (sd->flags & SD_SHARE_PKG_RESOURCES && nr_busy > 1)
- goto need_kick_unlock;
+ if (sd) {
+ sg = sd->groups;
+ sgp = sg->sgp;
+ nr_busy = atomic_read(&sgp->nr_busy_cpus);
- if (sd->flags & SD_ASYM_PACKING && nr_busy != sg->group_weight
- && (cpumask_first_and(nohz.idle_cpus_mask,
- sched_domain_span(sd)) < cpu))
+ if (nr_busy > 1)
goto need_kick_unlock;
-
- if (!(sd->flags & (SD_SHARE_PKG_RESOURCES | SD_ASYM_PACKING)))
- break;
}
+
+ sd = highest_flag_domain(cpu, SD_ASYM_PACKING);
+
+ if (sd && (cpumask_first_and(nohz.idle_cpus_mask,
+ sched_domain_span(sd)) < cpu))
+ goto need_kick_unlock;
+
rcu_read_unlock();
return 0;
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index ffc7087..0f1253f 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -623,6 +623,7 @@ DECLARE_PER_CPU(struct sched_domain *, sd_llc);
DECLARE_PER_CPU(int, sd_llc_size);
DECLARE_PER_CPU(int, sd_llc_id);
DECLARE_PER_CPU(struct sched_domain *, sd_numa);
+DECLARE_PER_CPU(struct sched_domain *, sd_busy);
struct sched_group_power {
atomic_t ref;
^ permalink raw reply related
* Re: [PATCH] [RFC] Emulate "lwsync" to run standard user land on e500 cores
From: Scott Wood @ 2013-10-23 10:15 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev, Wolfgang Denk
In-Reply-To: <39CCEB38-1D9B-4918-B8F4-148D4E90FE21@kernel.crashing.org>
On Wed, 2013-10-23 at 00:07 -0500, Kumar Gala wrote:
> On Oct 18, 2013, at 2:38 AM, Wolfgang Denk wrote:
> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> > index f783c93..f330374 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -986,6 +986,13 @@ static int emulate_instruction(struct pt_regs *regs)
> > return 0;
> > }
> >
> > + /* Emulating the lwsync insn as a sync insn */
> > + if (instword == PPC_INST_LWSYNC) {
> > + PPC_WARN_EMULATED(lwsync, regs);
> > + asm volatile("sync" : : : "memory");
>
> Do we really need the inline asm? Doesn't the fact of just taking an exception and returning from it equate to a sync.
No, it doesn't equate to a sync. See the discussion here:
http://patchwork.ozlabs.org/patch/256747/
-Scott
^ permalink raw reply
* Re: powerpc: Don't corrupt user registers on 32-bit
From: Scott Wood @ 2013-10-23 10:20 UTC (permalink / raw)
To: Paul Mackerras; +Cc: Alexander Graf, linuxppc-dev
In-Reply-To: <20131023084002.GA8325@iris.ozlabs.ibm.com>
On Wed, Oct 23, 2013 at 09:40:02AM +0100, Paul Mackerras wrote:
> Commit de79f7b9f6 ("powerpc: Put FP/VSX and VR state into structures")
> modified load_up_fpu() and load_up_altivec() in such a way that they
> now use r7 and r8. Unfortunately, the callers of these functions on
> 32-bit machines then return to userspace via fast_exception_return,
> which doesn't restore all of the volatile GPRs, but only r1, r3 -- r6
> and r9 -- r12. This was causing userspace segfaults and other
> userspace misbehaviour on 32-bit machines.
>
> This fixes the problem by changing the register usage of load_up_fpu()
> and load_up_altivec() to avoid using r7 and r8 and instead use r6 and
> r10. This also adds comments to those functions saying which registers
> may be used.
>
> Signed-off-by: Paul Mackerras <paulus@samba.org>
>
> ---
> arch/powerpc/kernel/fpu.S | 14 ++++++++------
> arch/powerpc/kernel/vector.S | 15 +++++++++------
> 2 files changed, 17 insertions(+), 12 deletions(-)
Tested-by: Scott Wood <scottwood@freescale.com> (on e500mc, so no altivec)
-Scott
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox