Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1498286 > unrolled thread
| Started by | Tejun Heo <tj@kernel.org> |
|---|---|
| First post | 2016-10-10 15:10 +0200 |
| Last post | 2016-10-11 14:30 +0200 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue available early during boot) Tejun Heo <tj@kernel.org> - 2016-10-10 15:10 +0200
Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue available early during boot) Tejun Heo <tj@kernel.org> - 2016-10-10 15:20 +0200
Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue available early during boot) Michael Ellerman <mpe@ellerman.id.au> - 2016-10-11 13:30 +0200
Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue available early during boot) Balbir Singh <bsingharora@gmail.com> - 2016-10-11 14:30 +0200
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2016-10-10 15:10 +0200 |
| Subject | Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue available early during boot) |
| Message-ID | <sqIv7-1FC-5@gated-at.bofh.it> |
Hello, Michael.
On Mon, Oct 10, 2016 at 09:22:55PM +1100, Michael Ellerman wrote:
> This patch seems to be causing one of my Power8 boxes not to boot.
>
> Specifically commit 3347fa092821 ("workqueue: make workqueue available
> early during boot") in linux-next.
>
> If I revert this on top of next-20161005 then the machine boots again.
>
> I've attached the oops below. It looks like the cfs_rq of p->se is NULL?
Hah, weird that it's arch dependent, or maybe it's just different
config options. Most likely, it's caused by workqueue_init() call
being moved too early. Can you please try the following patch and see
whether the problem goes away?
Thanks.
diff --git a/init/main.c b/init/main.c
index 5c4fd68..fe4fa47 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1013,7 +1013,7 @@ static noinline void __init kernel_init_freeable(void)
smp_prepare_cpus(setup_max_cpus);
- workqueue_init();
+ //workqueue_init();
do_pre_smp_initcalls();
lockup_detector_init();
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index ad0cd43..400f5e2 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5585,3 +5585,4 @@ int __init workqueue_init(void)
return 0;
}
+early_initcall(workqueue_init);
[toc] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2016-10-10 15:20 +0200 |
| Message-ID | <sqIEO-1J7-5@gated-at.bofh.it> |
| In reply to | #1498286 |
On Mon, Oct 10, 2016 at 09:02:53AM -0400, Tejun Heo wrote:
> Hello, Michael.
>
> On Mon, Oct 10, 2016 at 09:22:55PM +1100, Michael Ellerman wrote:
> > This patch seems to be causing one of my Power8 boxes not to boot.
> >
> > Specifically commit 3347fa092821 ("workqueue: make workqueue available
> > early during boot") in linux-next.
> >
> > If I revert this on top of next-20161005 then the machine boots again.
> >
> > I've attached the oops below. It looks like the cfs_rq of p->se is NULL?
>
> Hah, weird that it's arch dependent, or maybe it's just different
> config options. Most likely, it's caused by workqueue_init() call
> being moved too early. Can you please try the following patch and see
> whether the problem goes away?
And if it does, can you please post the boot log with kernel boot
option "initcall_debug=1" specified?
Thanks.
--
tejun
[toc] | [prev] | [next] | [standalone]
| From | Michael Ellerman <mpe@ellerman.id.au> |
|---|---|
| Date | 2016-10-11 13:30 +0200 |
| Subject | Re: Oops on Power8 (was Re: [PATCH v2 1/7] workqueue: make workqueue available early during boot) |
| Message-ID | <sr3pT-666-15@gated-at.bofh.it> |
| In reply to | #1498286 |
Tejun Heo <tj@kernel.org> writes:
> Hello, Michael.
>
> On Mon, Oct 10, 2016 at 09:22:55PM +1100, Michael Ellerman wrote:
>> This patch seems to be causing one of my Power8 boxes not to boot.
>>
>> Specifically commit 3347fa092821 ("workqueue: make workqueue available
>> early during boot") in linux-next.
>>
>> If I revert this on top of next-20161005 then the machine boots again.
>>
>> I've attached the oops below. It looks like the cfs_rq of p->se is NULL?
>
> Hah, weird that it's arch dependent, or maybe it's just different
> config options. Most likely, it's caused by workqueue_init() call
> being moved too early. Can you please try the following patch and see
> whether the problem goes away?
No that doesn't help.
What does is this:
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 94732d1ab00a..4e79549d242f 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1614,7 +1614,8 @@ int select_task_rq(struct task_struct *p, int cpu, int sd_flags, int wake_flags)
* [ this allows ->select_task() to simply return task_cpu(p) and
* not worry about this generic constraint ]
*/
- if (unlikely(!cpumask_test_cpu(cpu, tsk_cpus_allowed(p)) ||
+ if (unlikely(cpu >= nr_cpu_ids ||
+ !cpumask_test_cpu(cpu, tsk_cpus_allowed(p)) ||
!cpu_online(cpu)))
cpu = select_fallback_rq(task_cpu(p), p);
The oops happens because we're in enqueue_task_fair() and p->se->cfs_rq
is NULL.
The cfs_rq is NULL because we did set_task_rq(p, 2048), where 2048 is
NR_CPUS. That causes us to index past the end of the tg->cfs_rq array in
set_task_rq() and happen to get NULL.
We never should have done set_task_rq(p, 2048), because 2048 is >=
nr_cpu_ids, which means it's not a valid CPU number, and set_task_rq()
doesn't cope with that.
The reason we're calling set_task_rq() with CPU 2048 is because
in select_task_rq() we had tsk_nr_cpus_allowed() = 0, because
tsk_cpus_allowed(p) is an empty cpu mask.
That means we do in select_task_rq():
cpu = cpumask_any(tsk_cpus_allowed(p));
And when tsk_cpus_allowed(p) is empty cpumask_any() returns nr_cpu_ids,
causing cpu to be set to 2048 in my case.
select_task_rq() then does the check to see if it should use a fallback
rq:
if (unlikely(!cpumask_test_cpu(cpu, tsk_cpus_allowed(p)) ||
!cpu_online(cpu)))
cpu = select_fallback_rq(task_cpu(p), p);
But in both those checks we end up indexing off the end of the cpu mask,
because cpu is >= nr_cpu_ids. At least on my system they both return
true and so we return cpu == 2048.
The patch above is pretty clearly not the right fix, though maybe it's a
good safety measure.
Presumably we shouldn't be ending up with tsk_cpus_allowed() being
empty, but I haven't had time to track down why that's happening.
cheers
[toc] | [prev] | [next] | [standalone]
| From | Balbir Singh <bsingharora@gmail.com> |
|---|---|
| Date | 2016-10-11 14:30 +0200 |
| Message-ID | <sr4lX-6Fi-27@gated-at.bofh.it> |
| In reply to | #1498767 |
On 11/10/16 22:22, Michael Ellerman wrote:
> Tejun Heo <tj@kernel.org> writes:
>
>> Hello, Michael.
>>
>> On Mon, Oct 10, 2016 at 09:22:55PM +1100, Michael Ellerman wrote:
>>> This patch seems to be causing one of my Power8 boxes not to boot.
>>>
>>> Specifically commit 3347fa092821 ("workqueue: make workqueue available
>>> early during boot") in linux-next.
>>>
>>> If I revert this on top of next-20161005 then the machine boots again.
>>>
>>> I've attached the oops below. It looks like the cfs_rq of p->se is NULL?
>>
>> Hah, weird that it's arch dependent, or maybe it's just different
>> config options. Most likely, it's caused by workqueue_init() call
>> being moved too early. Can you please try the following patch and see
>> whether the problem goes away?
>
> No that doesn't help.
>
> What does is this:
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 94732d1ab00a..4e79549d242f 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1614,7 +1614,8 @@ int select_task_rq(struct task_struct *p, int cpu, int sd_flags, int wake_flags)
> * [ this allows ->select_task() to simply return task_cpu(p) and
> * not worry about this generic constraint ]
> */
> - if (unlikely(!cpumask_test_cpu(cpu, tsk_cpus_allowed(p)) ||
> + if (unlikely(cpu >= nr_cpu_ids ||
> + !cpumask_test_cpu(cpu, tsk_cpus_allowed(p)) ||
> !cpu_online(cpu)))
> cpu = select_fallback_rq(task_cpu(p), p);
>
>
> The oops happens because we're in enqueue_task_fair() and p->se->cfs_rq
> is NULL.
>
> The cfs_rq is NULL because we did set_task_rq(p, 2048), where 2048 is
> NR_CPUS. That causes us to index past the end of the tg->cfs_rq array in
> set_task_rq() and happen to get NULL.
>
> We never should have done set_task_rq(p, 2048), because 2048 is >=
> nr_cpu_ids, which means it's not a valid CPU number, and set_task_rq()
> doesn't cope with that.
>
> The reason we're calling set_task_rq() with CPU 2048 is because
> in select_task_rq() we had tsk_nr_cpus_allowed() = 0, because
> tsk_cpus_allowed(p) is an empty cpu mask.
>
> That means we do in select_task_rq():
> cpu = cpumask_any(tsk_cpus_allowed(p));
>
> And when tsk_cpus_allowed(p) is empty cpumask_any() returns nr_cpu_ids,
> causing cpu to be set to 2048 in my case.
>
> select_task_rq() then does the check to see if it should use a fallback
> rq:
>
> if (unlikely(!cpumask_test_cpu(cpu, tsk_cpus_allowed(p)) ||
> !cpu_online(cpu)))
> cpu = select_fallback_rq(task_cpu(p), p);
>
>
> But in both those checks we end up indexing off the end of the cpu mask,
> because cpu is >= nr_cpu_ids. At least on my system they both return
> true and so we return cpu == 2048.
>
> The patch above is pretty clearly not the right fix, though maybe it's a
> good safety measure.
>
> Presumably we shouldn't be ending up with tsk_cpus_allowed() being
> empty, but I haven't had time to track down why that's happening.
>
> cheers
>
+peterz
FYI: I see the samething on my cpu as well, its just that I get lucky
and cpu_online(cpu) returns false.
I think from a functional perspective we may want to get some additional
debug checks for places where the cpumask is empty early during boot.
Looks like there is a dependency between cpumasks and cpus coming online.
I wonder if we can hit similar issues during hotplug
FWIW, your patch looks correct to me, though one might argue that
cpumask_test_cpu() is a better place to fix it
Balbir Singh
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web