From: Kevin Hilman khilman@linaro.org
Commit cd5c2cc93d3d (hmp: Remove potential for task_struct access race) introduced a put_task_struct() to prevent races, but in doing so introduced potential spinlock recursion. (This change was further consolidated in commit 0baa5811bacf -- sched: hmp: unify active migration code.)
Unfortunately, the put_task_struct() is done while the runqueue spinlock is held, but put_task_struct() can also cause a reschedule causing the runqueue lock to be acquired recursively.
To fix, move the put_task_struct() outside the runqueue spinlock.
Reported-by: Victor Lixin victor.lixin@hisilicon.com Cc: Jorge Ramirez-Ortiz jorge.ramirez-ortiz@linaro.org Cc: Chris Redpath chris.redpath@arm.com Cc: Liviu Dudau Liviu.Dudau@arm.com Cc: Jon Medhurst tixy@linaro.org Signed-off-by: Kevin Hilman khilman@linaro.org --- Reported in LDTS Ticket: https://support.linaro.org/tickets/1349
Chris, Tixy, any objections to this HMP fix for LSK-v3.10?
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 41d0cbda605d..1baf6413a882 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -6489,10 +6489,10 @@ static int __do_active_load_balance_cpu_stop(void *data, bool check_sd_lb_flag) rcu_read_unlock(); double_unlock_balance(busiest_rq, target_rq); out_unlock: - if (!check_sd_lb_flag) - put_task_struct(p); busiest_rq->active_balance = 0; raw_spin_unlock_irq(&busiest_rq->lock); + if (!check_sd_lb_flag) + put_task_struct(p); return 0; }
CC to Lixin, the original reporter and patch proposer.
On 04/09/2015 05:32 AM, Kevin Hilman wrote:
From: Kevin Hilman khilman@linaro.org
Commit cd5c2cc93d3d (hmp: Remove potential for task_struct access race) introduced a put_task_struct() to prevent races, but in doing so introduced potential spinlock recursion. (This change was further consolidated in commit 0baa5811bacf -- sched: hmp: unify active migration code.)
Unfortunately, the put_task_struct() is done while the runqueue spinlock is held, but put_task_struct() can also cause a reschedule causing the runqueue lock to be acquired recursively.
To fix, move the put_task_struct() outside the runqueue spinlock.
Reported-by: Victor Lixin victor.lixin@hisilicon.com Cc: Jorge Ramirez-Ortiz jorge.ramirez-ortiz@linaro.org Cc: Chris Redpath chris.redpath@arm.com Cc: Liviu Dudau Liviu.Dudau@arm.com Cc: Jon Medhurst tixy@linaro.org Signed-off-by: Kevin Hilman khilman@linaro.org
Reported in LDTS Ticket: https://support.linaro.org/tickets/1349
Chris, Tixy, any objections to this HMP fix for LSK-v3.10?
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 41d0cbda605d..1baf6413a882 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -6489,10 +6489,10 @@ static int __do_active_load_balance_cpu_stop(void *data, bool check_sd_lb_flag) rcu_read_unlock(); double_unlock_balance(busiest_rq, target_rq); out_unlock:
- if (!check_sd_lb_flag)
put_task_struct(p);
- if (!check_sd_lb_flag)
put_task_struct(p);}
On 04/09/2015 05:32 AM, Kevin Hilman wrote:
From: Kevin Hilman khilman@linaro.org
Commit cd5c2cc93d3d (hmp: Remove potential for task_struct access race) introduced a put_task_struct() to prevent races, but in doing so introduced potential spinlock recursion. (This change was further consolidated in commit 0baa5811bacf -- sched: hmp: unify active migration code.)
Unfortunately, the put_task_struct() is done while the runqueue spinlock is held, but put_task_struct() can also cause a reschedule causing the runqueue lock to be acquired recursively.
To fix, move the put_task_struct() outside the runqueue spinlock.
Reported-by: Victor Lixin victor.lixin@hisilicon.com Cc: Jorge Ramirez-Ortiz jorge.ramirez-ortiz@linaro.org Cc: Chris Redpath chris.redpath@arm.com Cc: Liviu Dudau Liviu.Dudau@arm.com Cc: Jon Medhurst tixy@linaro.org Signed-off-by: Kevin Hilman khilman@linaro.org
Reviewed-by: Alex Shi
Reported in LDTS Ticket: https://support.linaro.org/tickets/1349
Chris, Tixy, any objections to this HMP fix for LSK-v3.10?
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 41d0cbda605d..1baf6413a882 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -6489,10 +6489,10 @@ static int __do_active_load_balance_cpu_stop(void *data, bool check_sd_lb_flag) rcu_read_unlock(); double_unlock_balance(busiest_rq, target_rq); out_unlock:
- if (!check_sd_lb_flag)
put_task_struct(p);
- if (!check_sd_lb_flag)
put_task_struct(p);}
On Wed, 2015-04-08 at 14:32 -0700, Kevin Hilman wrote:
From: Kevin Hilman khilman@linaro.org
Commit cd5c2cc93d3d (hmp: Remove potential for task_struct access race) introduced a put_task_struct() to prevent races, but in doing so introduced potential spinlock recursion. (This change was further consolidated in commit 0baa5811bacf -- sched: hmp: unify active migration code.)
Unfortunately, the put_task_struct() is done while the runqueue spinlock is held, but put_task_struct() can also cause a reschedule causing the runqueue lock to be acquired recursively.
To fix, move the put_task_struct() outside the runqueue spinlock.
Reported-by: Victor Lixin victor.lixin@hisilicon.com Cc: Jorge Ramirez-Ortiz jorge.ramirez-ortiz@linaro.org Cc: Chris Redpath chris.redpath@arm.com Cc: Liviu Dudau Liviu.Dudau@arm.com Cc: Jon Medhurst tixy@linaro.org Signed-off-by: Kevin Hilman khilman@linaro.org
Reported in LDTS Ticket: https://support.linaro.org/tickets/1349
Chris, Tixy, any objections to this HMP fix for LSK-v3.10?
Looks good to me.
Reviewed-by: Jon Medhurst tixy@linaro.org
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 41d0cbda605d..1baf6413a882 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -6489,10 +6489,10 @@ static int __do_active_load_balance_cpu_stop(void *data, bool check_sd_lb_flag) rcu_read_unlock(); double_unlock_balance(busiest_rq, target_rq); out_unlock:
- if (!check_sd_lb_flag)
put_task_struct(p);
- if (!check_sd_lb_flag)
put_task_struct(p);}
Hi Kevin, Tixy ________________________________________ From: Jon Medhurst (Tixy) [tixy@linaro.org] Sent: 10 April 2015 12:25 To: Kevin Hilman Cc: Chris Redpath; alex.shi@linaro.org; Jorge Ramirez-Ortiz; Mark Brown; linaro-kernel@lists.linaro.org Subject: Re: [PATCH LSK-v3.10] sched: hmp: fix spinlock recurision in active migration
On Wed, 2015-04-08 at 14:32 -0700, Kevin Hilman wrote:
From: Kevin Hilman khilman@linaro.org
Commit cd5c2cc93d3d (hmp: Remove potential for task_struct access race) introduced a put_task_struct() to prevent races, but in doing so introduced potential spinlock recursion. (This change was further consolidated in commit 0baa5811bacf -- sched: hmp: unify active migration code.)
Unfortunately, the put_task_struct() is done while the runqueue spinlock is held, but put_task_struct() can also cause a reschedule causing the runqueue lock to be acquired recursively.
To fix, move the put_task_struct() outside the runqueue spinlock.
Reported-by: Victor Lixin victor.lixin@hisilicon.com Cc: Jorge Ramirez-Ortiz jorge.ramirez-ortiz@linaro.org Cc: Chris Redpath chris.redpath@arm.com Cc: Liviu Dudau Liviu.Dudau@arm.com Cc: Jon Medhurst tixy@linaro.org Signed-off-by: Kevin Hilman khilman@linaro.org
Reported in LDTS Ticket: https://support.linaro.org/tickets/1349
Chris, Tixy, any objections to this HMP fix for LSK-v3.10?
Looks good to me.
Reviewed-by: Jon Medhurst tixy@linaro.org
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 41d0cbda605d..1baf6413a882 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -6489,10 +6489,10 @@ static int __do_active_load_balance_cpu_stop(void *data, bool check_sd_lb_flag) rcu_read_unlock(); double_unlock_balance(busiest_rq, target_rq); out_unlock:
if (!check_sd_lb_flag)put_task_struct(p); busiest_rq->active_balance = 0; raw_spin_unlock_irq(&busiest_rq->lock);
if (!check_sd_lb_flag)put_task_struct(p); return 0;}
Apologies for the disclaimer which will undoubtedly follow, still need to sort that out.
Looks OK to me too.
Reviewed-by: Chris Redpath chris.redpath@arm.com
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590 ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782
On Wed, 2015-04-08 at 14:32 -0700, Kevin Hilman wrote:
From: Kevin Hilman khilman@linaro.org
Commit cd5c2cc93d3d (hmp: Remove potential for task_struct access race) introduced a put_task_struct() to prevent races, but in doing so introduced potential spinlock recursion. (This change was further consolidated in commit 0baa5811bacf -- sched: hmp: unify active migration code.)
Unfortunately, the put_task_struct() is done while the runqueue spinlock is held, but put_task_struct() can also cause a reschedule
Under what circumstances can this reschedule happen? As the LDTS ticket is not accessible we are lacking the background and analysis of the reported problem.
On 04/13/2015 11:42 AM, Jon Medhurst (Tixy) wrote:
On Wed, 2015-04-08 at 14:32 -0700, Kevin Hilman wrote:
From: Kevin Hilman khilman@linaro.org
Commit cd5c2cc93d3d (hmp: Remove potential for task_struct access race) introduced a put_task_struct() to prevent races, but in doing so introduced potential spinlock recursion. (This change was further consolidated in commit 0baa5811bacf -- sched: hmp: unify active migration code.)
Unfortunately, the put_task_struct() is done while the runqueue spinlock is held, but put_task_struct() can also cause a reschedule
Under what circumstances can this reschedule happen? As the LDTS ticket is not accessible we are lacking the background and analysis of the reported problem.
I attach the sping lock recursion detection log as reported
I suppose you dont access to this link below https://support.linaro.org/tickets/1349
On Mon, 2015-04-13 at 11:58 -0400, Jorge Ramirez-Ortiz wrote:
I attach the sping lock recursion detection log as reported
Thanks
I suppose you dont access to this link below https://support.linaro.org/tickets/1349
I don't have, and I assume people at ARM who work on HMP won't either.
linaro-kernel@lists.linaro.org
-
Alex Shi -
Chris Redpath -
Jon Medhurst (Tixy) -
Jorge Ramirez-Ortiz -
Kevin Hilman