diff mbox series

[v4,1/5] sched/topology: Add check to backup comment about hotplug lock

Message ID 20180613121711.5018-2-juri.lelli@redhat.com
State New
Headers show
Series [v4,1/5] sched/topology: Add check to backup comment about hotplug lock | expand

Commit Message

Juri Lelli June 13, 2018, 12:17 p.m. UTC 
From: Mathieu Poirier <mathieu.poirier@linaro.org>


The comment above function partition_sched_domains() clearly state that
the cpu_hotplug_lock should be held but doesn't mandate one to abide to
it.

Add an explicit check backing that comment, so to make it impossible
for anyone to miss the requirement.

Suggested-by: Juri Lelli <juri.lelli@redhat.com>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

[modified changelog]
Signed-off-by: Juri Lelli <juri.lelli@redhat.com>

---
 kernel/sched/topology.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.14.3

Comments

Steven Rostedt June 14, 2018, 1:33 p.m. UTC | #1 
On Wed, 13 Jun 2018 14:17:07 +0200
Juri Lelli <juri.lelli@redhat.com> wrote:

> From: Mathieu Poirier <mathieu.poirier@linaro.org>

> 

> The comment above function partition_sched_domains() clearly state that

> the cpu_hotplug_lock should be held but doesn't mandate one to abide to

> it.

> 

> Add an explicit check backing that comment, so to make it impossible

> for anyone to miss the requirement.

> 

> Suggested-by: Juri Lelli <juri.lelli@redhat.com>

> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> [modified changelog]

> Signed-off-by: Juri Lelli <juri.lelli@redhat.com>


Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>


-- Steve

> ---

>  kernel/sched/topology.c | 1 +

>  1 file changed, 1 insertion(+)

> 

> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c

> index 61a1125c1ae4..96eee22fafe8 100644

> --- a/kernel/sched/topology.c

> +++ b/kernel/sched/topology.c

> @@ -1858,6 +1858,7 @@ void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],

>  	int i, j, n;

>  	int new_topology;

>  

> +	lockdep_assert_cpus_held();

>  	mutex_lock(&sched_domains_mutex);

>  

>  	/* Always unregister in case we don't destroy any domains: */
Juri Lelli June 14, 2018, 1:42 p.m. UTC | #2 
On 14/06/18 09:33, Steven Rostedt wrote:
> On Wed, 13 Jun 2018 14:17:07 +0200

> Juri Lelli <juri.lelli@redhat.com> wrote:

> 

> > From: Mathieu Poirier <mathieu.poirier@linaro.org>

> > 

> > The comment above function partition_sched_domains() clearly state that

> > the cpu_hotplug_lock should be held but doesn't mandate one to abide to

> > it.

> > 

> > Add an explicit check backing that comment, so to make it impossible

> > for anyone to miss the requirement.

> > 

> > Suggested-by: Juri Lelli <juri.lelli@redhat.com>

> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> > [modified changelog]

> > Signed-off-by: Juri Lelli <juri.lelli@redhat.com>

> 

> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

> 

> -- Steve

> 

> > ---

> >  kernel/sched/topology.c | 1 +

> >  1 file changed, 1 insertion(+)

> > 

> > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c

> > index 61a1125c1ae4..96eee22fafe8 100644

> > --- a/kernel/sched/topology.c

> > +++ b/kernel/sched/topology.c

> > @@ -1858,6 +1858,7 @@ void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],

> >  	int i, j, n;

> >  	int new_topology;

> >  

> > +	lockdep_assert_cpus_held();

> >  	mutex_lock(&sched_domains_mutex);

> >  

> >  	/* Always unregister in case we don't destroy any domains: */


Thanks for reviewing Steve. I thought this should be fine (also looking
at the comment before the function), but then got this from 0day

[    2.206129] .................................... done.
[    2.206662] Using IPI No-Shortcut mode
[    2.207084] sched_clock: Marking stable (2200014216, 0)->(2435550722, -235536506)
[    2.208792] registered taskstats version 1
[    2.209251] page_owner is disabled
[    2.290164] WARNING: CPU: 0 PID: 13 at kernel/cpu.c:311 lockdep_assert_cpus_held+0x22/0x26
[    2.291253] Modules linked in:
[    2.291589] CPU: 0 PID: 13 Comm: cpuhp/0 Not tainted 4.17.0-rc6-00217-gebf44b4 #2
[    2.292367] EIP: lockdep_assert_cpus_held+0x22/0x26
[    2.292882] EFLAGS: 00210246 CPU: 0
[    2.293255] EAX: 00000000 EBX: 00000000 ECX: c1b1fd98 EDX: 00200286
[    2.293915] ESI: 00000000 EDI: c1b1fcb4 EBP: dd8a3e60 ESP: dd8a3e60
[    2.294578]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
[    2.295128] CR0: 80050033 CR2: 00000000 CR3: 01c9d000 CR4: 000006d0
[    2.295770] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[    2.296409] DR6: fffe0ff0 DR7: 00000400
[    2.296802] Call Trace:
[    2.297066]  partition_sched_domains+0x1b/0x29f
[    2.297557]  ? dl_cpu_busy+0x1b5/0x1c6
[    2.297955]  sched_cpu_deactivate+0x8b/0xb6
[    2.298403]  ? call_rcu_bh+0x17/0x17
[    2.298786]  ? call_rcu_bh+0x17/0x17
[    2.299170]  ? rcu_read_lock_bh_held+0x55/0x55
[    2.299648]  ? sched_cpu_activate+0xcc/0xcc
[    2.300076]  cpuhp_invoke_callback+0x19d/0x978
[    2.300537]  cpuhp_thread_fun+0x125/0x16b
[    2.300949]  smpboot_thread_fn+0x192/0x1a6
[    2.301377]  kthread+0xfc/0x101
[    2.301703]  ? sort_range+0x1d/0x1d
[    2.302062]  ? kthread_flush_work_fn+0x12/0x12
[    2.302524]  ret_from_fork+0x2e/0x40
[    2.302891] Code: ff 83 c4 10 5b 5e 5f 5d c3 55 89 e5 e8 ca dc fe ff 83 3d 20 bc b4 c1 00 74 13 83 ca ff b8 98 fd b1 c1 e8 e2 4b 04 00 85 c0 75 02 <0f> 0b 5d c3 55 89 e5 56 53 e8 a2 dc fe ff 89 c6 3b 05 8c 40 bd 
[    2.304975] irq event stamp: 308
[    2.305335] hardirqs last  enabled at (307): [<c179d83c>] _raw_spin_unlock_irqrestore+0x4a/0x68
[    2.306195] hardirqs last disabled at (308): [<c179ed38>] common_exception+0x46/0x6e
[    2.306969] softirqs last  enabled at (0): [<c10448e6>] copy_process+0x582/0x16c5
[    2.307720] softirqs last disabled at (0): [<00000000>]   (null)
[    2.308328] ---[ end trace 7197acb79221f1b9 ]---

Guess early boot is "special"? :-/

Best,

- Juri
Steven Rostedt June 14, 2018, 1:47 p.m. UTC | #3 
On Thu, 14 Jun 2018 15:42:34 +0200
Juri Lelli <juri.lelli@redhat.com> wrote:

> On 14/06/18 09:33, Steven Rostedt wrote:

> > On Wed, 13 Jun 2018 14:17:07 +0200

> > Juri Lelli <juri.lelli@redhat.com> wrote:

> >   

> > > From: Mathieu Poirier <mathieu.poirier@linaro.org>

> > > 

> > > The comment above function partition_sched_domains() clearly state that

> > > the cpu_hotplug_lock should be held but doesn't mandate one to abide to

> > > it.

> > > 

> > > Add an explicit check backing that comment, so to make it impossible

> > > for anyone to miss the requirement.

> > > 

> > > Suggested-by: Juri Lelli <juri.lelli@redhat.com>

> > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> > > [modified changelog]

> > > Signed-off-by: Juri Lelli <juri.lelli@redhat.com>  

> > 

> > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

> > 

> > -- Steve

> >   

> > > ---

> > >  kernel/sched/topology.c | 1 +

> > >  1 file changed, 1 insertion(+)

> > > 

> > > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c

> > > index 61a1125c1ae4..96eee22fafe8 100644

> > > --- a/kernel/sched/topology.c

> > > +++ b/kernel/sched/topology.c

> > > @@ -1858,6 +1858,7 @@ void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],

> > >  	int i, j, n;

> > >  	int new_topology;

> > >  

> > > +	lockdep_assert_cpus_held();

> > >  	mutex_lock(&sched_domains_mutex);

> > >  

> > >  	/* Always unregister in case we don't destroy any domains: */  

> 

> Thanks for reviewing Steve. I thought this should be fine (also looking

> at the comment before the function), but then got this from 0day

> 

> [    2.206129] .................................... done.

> [    2.206662] Using IPI No-Shortcut mode

> [    2.207084] sched_clock: Marking stable (2200014216, 0)->(2435550722, -235536506)

> [    2.208792] registered taskstats version 1

> [    2.209251] page_owner is disabled

> [    2.290164] WARNING: CPU: 0 PID: 13 at kernel/cpu.c:311 lockdep_assert_cpus_held+0x22/0x26

> [    2.291253] Modules linked in:

> [    2.291589] CPU: 0 PID: 13 Comm: cpuhp/0 Not tainted 4.17.0-rc6-00217-gebf44b4 #2

> [    2.292367] EIP: lockdep_assert_cpus_held+0x22/0x26

> [    2.292882] EFLAGS: 00210246 CPU: 0

> [    2.293255] EAX: 00000000 EBX: 00000000 ECX: c1b1fd98 EDX: 00200286

> [    2.293915] ESI: 00000000 EDI: c1b1fcb4 EBP: dd8a3e60 ESP: dd8a3e60

> [    2.294578]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068

> [    2.295128] CR0: 80050033 CR2: 00000000 CR3: 01c9d000 CR4: 000006d0

> [    2.295770] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000

> [    2.296409] DR6: fffe0ff0 DR7: 00000400

> [    2.296802] Call Trace:

> [    2.297066]  partition_sched_domains+0x1b/0x29f

> [    2.297557]  ? dl_cpu_busy+0x1b5/0x1c6

> [    2.297955]  sched_cpu_deactivate+0x8b/0xb6

> [    2.298403]  ? call_rcu_bh+0x17/0x17

> [    2.298786]  ? call_rcu_bh+0x17/0x17

> [    2.299170]  ? rcu_read_lock_bh_held+0x55/0x55

> [    2.299648]  ? sched_cpu_activate+0xcc/0xcc

> [    2.300076]  cpuhp_invoke_callback+0x19d/0x978

> [    2.300537]  cpuhp_thread_fun+0x125/0x16b

> [    2.300949]  smpboot_thread_fn+0x192/0x1a6

> [    2.301377]  kthread+0xfc/0x101

> [    2.301703]  ? sort_range+0x1d/0x1d

> [    2.302062]  ? kthread_flush_work_fn+0x12/0x12

> [    2.302524]  ret_from_fork+0x2e/0x40

> [    2.302891] Code: ff 83 c4 10 5b 5e 5f 5d c3 55 89 e5 e8 ca dc fe ff 83 3d 20 bc b4 c1 00 74 13 83 ca ff b8 98 fd b1 c1 e8 e2 4b 04 00 85 c0 75 02 <0f> 0b 5d c3 55 89 e5 56 53 e8 a2 dc fe ff 89 c6 3b 05 8c 40 bd 

> [    2.304975] irq event stamp: 308

> [    2.305335] hardirqs last  enabled at (307): [<c179d83c>] _raw_spin_unlock_irqrestore+0x4a/0x68

> [    2.306195] hardirqs last disabled at (308): [<c179ed38>] common_exception+0x46/0x6e

> [    2.306969] softirqs last  enabled at (0): [<c10448e6>] copy_process+0x582/0x16c5

> [    2.307720] softirqs last disabled at (0): [<00000000>]   (null)

> [    2.308328] ---[ end trace 7197acb79221f1b9 ]---

> 

> Guess early boot is "special"? :-/


I'm guessing the comment is wrong. I just added the first three patches
and was going to try to trigger the first "bug" I had before. Let me
check if I have lockdep enabled and see if I see the same thing.

-- Steve
Juri Lelli June 14, 2018, 1:50 p.m. UTC | #4 
On 14/06/18 09:47, Steven Rostedt wrote:
> On Thu, 14 Jun 2018 15:42:34 +0200

> Juri Lelli <juri.lelli@redhat.com> wrote:

> 

> > On 14/06/18 09:33, Steven Rostedt wrote:

> > > On Wed, 13 Jun 2018 14:17:07 +0200

> > > Juri Lelli <juri.lelli@redhat.com> wrote:

> > >   

> > > > From: Mathieu Poirier <mathieu.poirier@linaro.org>

> > > > 

> > > > The comment above function partition_sched_domains() clearly state that

> > > > the cpu_hotplug_lock should be held but doesn't mandate one to abide to

> > > > it.

> > > > 

> > > > Add an explicit check backing that comment, so to make it impossible

> > > > for anyone to miss the requirement.

> > > > 

> > > > Suggested-by: Juri Lelli <juri.lelli@redhat.com>

> > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> > > > [modified changelog]

> > > > Signed-off-by: Juri Lelli <juri.lelli@redhat.com>  

> > > 

> > > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

> > > 

> > > -- Steve

> > >   

> > > > ---

> > > >  kernel/sched/topology.c | 1 +

> > > >  1 file changed, 1 insertion(+)

> > > > 

> > > > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c

> > > > index 61a1125c1ae4..96eee22fafe8 100644

> > > > --- a/kernel/sched/topology.c

> > > > +++ b/kernel/sched/topology.c

> > > > @@ -1858,6 +1858,7 @@ void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],

> > > >  	int i, j, n;

> > > >  	int new_topology;

> > > >  

> > > > +	lockdep_assert_cpus_held();

> > > >  	mutex_lock(&sched_domains_mutex);

> > > >  

> > > >  	/* Always unregister in case we don't destroy any domains: */  

> > 

> > Thanks for reviewing Steve. I thought this should be fine (also looking

> > at the comment before the function), but then got this from 0day

> > 

> > [    2.206129] .................................... done.

> > [    2.206662] Using IPI No-Shortcut mode

> > [    2.207084] sched_clock: Marking stable (2200014216, 0)->(2435550722, -235536506)

> > [    2.208792] registered taskstats version 1

> > [    2.209251] page_owner is disabled

> > [    2.290164] WARNING: CPU: 0 PID: 13 at kernel/cpu.c:311 lockdep_assert_cpus_held+0x22/0x26

> > [    2.291253] Modules linked in:

> > [    2.291589] CPU: 0 PID: 13 Comm: cpuhp/0 Not tainted 4.17.0-rc6-00217-gebf44b4 #2

> > [    2.292367] EIP: lockdep_assert_cpus_held+0x22/0x26

> > [    2.292882] EFLAGS: 00210246 CPU: 0

> > [    2.293255] EAX: 00000000 EBX: 00000000 ECX: c1b1fd98 EDX: 00200286

> > [    2.293915] ESI: 00000000 EDI: c1b1fcb4 EBP: dd8a3e60 ESP: dd8a3e60

> > [    2.294578]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068

> > [    2.295128] CR0: 80050033 CR2: 00000000 CR3: 01c9d000 CR4: 000006d0

> > [    2.295770] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000

> > [    2.296409] DR6: fffe0ff0 DR7: 00000400

> > [    2.296802] Call Trace:

> > [    2.297066]  partition_sched_domains+0x1b/0x29f

> > [    2.297557]  ? dl_cpu_busy+0x1b5/0x1c6

> > [    2.297955]  sched_cpu_deactivate+0x8b/0xb6

> > [    2.298403]  ? call_rcu_bh+0x17/0x17

> > [    2.298786]  ? call_rcu_bh+0x17/0x17

> > [    2.299170]  ? rcu_read_lock_bh_held+0x55/0x55

> > [    2.299648]  ? sched_cpu_activate+0xcc/0xcc

> > [    2.300076]  cpuhp_invoke_callback+0x19d/0x978

> > [    2.300537]  cpuhp_thread_fun+0x125/0x16b

> > [    2.300949]  smpboot_thread_fn+0x192/0x1a6

> > [    2.301377]  kthread+0xfc/0x101

> > [    2.301703]  ? sort_range+0x1d/0x1d

> > [    2.302062]  ? kthread_flush_work_fn+0x12/0x12

> > [    2.302524]  ret_from_fork+0x2e/0x40

> > [    2.302891] Code: ff 83 c4 10 5b 5e 5f 5d c3 55 89 e5 e8 ca dc fe ff 83 3d 20 bc b4 c1 00 74 13 83 ca ff b8 98 fd b1 c1 e8 e2 4b 04 00 85 c0 75 02 <0f> 0b 5d c3 55 89 e5 56 53 e8 a2 dc fe ff 89 c6 3b 05 8c 40 bd 

> > [    2.304975] irq event stamp: 308

> > [    2.305335] hardirqs last  enabled at (307): [<c179d83c>] _raw_spin_unlock_irqrestore+0x4a/0x68

> > [    2.306195] hardirqs last disabled at (308): [<c179ed38>] common_exception+0x46/0x6e

> > [    2.306969] softirqs last  enabled at (0): [<c10448e6>] copy_process+0x582/0x16c5

> > [    2.307720] softirqs last disabled at (0): [<00000000>]   (null)

> > [    2.308328] ---[ end trace 7197acb79221f1b9 ]---

> > 

> > Guess early boot is "special"? :-/

> 

> I'm guessing the comment is wrong. I just added the first three patches

> and was going to try to trigger the first "bug" I had before. Let me

> check if I have lockdep enabled and see if I see the same thing.


Mmm, I had it enabled while testing and didn't get the splat.
Quentin Perret June 14, 2018, 1:58 p.m. UTC | #5 
On Thursday 14 Jun 2018 at 15:50:40 (+0200), Juri Lelli wrote:
> On 14/06/18 09:47, Steven Rostedt wrote:

> > On Thu, 14 Jun 2018 15:42:34 +0200

> > Juri Lelli <juri.lelli@redhat.com> wrote:

> > 

> > > On 14/06/18 09:33, Steven Rostedt wrote:

> > > > On Wed, 13 Jun 2018 14:17:07 +0200

> > > > Juri Lelli <juri.lelli@redhat.com> wrote:

> > > >   

> > > > > From: Mathieu Poirier <mathieu.poirier@linaro.org>

> > > > > 

> > > > > The comment above function partition_sched_domains() clearly state that

> > > > > the cpu_hotplug_lock should be held but doesn't mandate one to abide to

> > > > > it.

> > > > > 

> > > > > Add an explicit check backing that comment, so to make it impossible

> > > > > for anyone to miss the requirement.

> > > > > 

> > > > > Suggested-by: Juri Lelli <juri.lelli@redhat.com>

> > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> > > > > [modified changelog]

> > > > > Signed-off-by: Juri Lelli <juri.lelli@redhat.com>  

> > > > 

> > > > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

> > > > 

> > > > -- Steve

> > > >   

> > > > > ---

> > > > >  kernel/sched/topology.c | 1 +

> > > > >  1 file changed, 1 insertion(+)

> > > > > 

> > > > > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c

> > > > > index 61a1125c1ae4..96eee22fafe8 100644

> > > > > --- a/kernel/sched/topology.c

> > > > > +++ b/kernel/sched/topology.c

> > > > > @@ -1858,6 +1858,7 @@ void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],

> > > > >  	int i, j, n;

> > > > >  	int new_topology;

> > > > >  

> > > > > +	lockdep_assert_cpus_held();

> > > > >  	mutex_lock(&sched_domains_mutex);

> > > > >  

> > > > >  	/* Always unregister in case we don't destroy any domains: */  

> > > 

> > > Thanks for reviewing Steve. I thought this should be fine (also looking

> > > at the comment before the function), but then got this from 0day

> > > 

> > > [    2.206129] .................................... done.

> > > [    2.206662] Using IPI No-Shortcut mode

> > > [    2.207084] sched_clock: Marking stable (2200014216, 0)->(2435550722, -235536506)

> > > [    2.208792] registered taskstats version 1

> > > [    2.209251] page_owner is disabled

> > > [    2.290164] WARNING: CPU: 0 PID: 13 at kernel/cpu.c:311 lockdep_assert_cpus_held+0x22/0x26

> > > [    2.291253] Modules linked in:

> > > [    2.291589] CPU: 0 PID: 13 Comm: cpuhp/0 Not tainted 4.17.0-rc6-00217-gebf44b4 #2

> > > [    2.292367] EIP: lockdep_assert_cpus_held+0x22/0x26

> > > [    2.292882] EFLAGS: 00210246 CPU: 0

> > > [    2.293255] EAX: 00000000 EBX: 00000000 ECX: c1b1fd98 EDX: 00200286

> > > [    2.293915] ESI: 00000000 EDI: c1b1fcb4 EBP: dd8a3e60 ESP: dd8a3e60

> > > [    2.294578]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068

> > > [    2.295128] CR0: 80050033 CR2: 00000000 CR3: 01c9d000 CR4: 000006d0

> > > [    2.295770] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000

> > > [    2.296409] DR6: fffe0ff0 DR7: 00000400

> > > [    2.296802] Call Trace:

> > > [    2.297066]  partition_sched_domains+0x1b/0x29f

> > > [    2.297557]  ? dl_cpu_busy+0x1b5/0x1c6

> > > [    2.297955]  sched_cpu_deactivate+0x8b/0xb6

> > > [    2.298403]  ? call_rcu_bh+0x17/0x17

> > > [    2.298786]  ? call_rcu_bh+0x17/0x17

> > > [    2.299170]  ? rcu_read_lock_bh_held+0x55/0x55

> > > [    2.299648]  ? sched_cpu_activate+0xcc/0xcc

> > > [    2.300076]  cpuhp_invoke_callback+0x19d/0x978

> > > [    2.300537]  cpuhp_thread_fun+0x125/0x16b

> > > [    2.300949]  smpboot_thread_fn+0x192/0x1a6

> > > [    2.301377]  kthread+0xfc/0x101

> > > [    2.301703]  ? sort_range+0x1d/0x1d

> > > [    2.302062]  ? kthread_flush_work_fn+0x12/0x12

> > > [    2.302524]  ret_from_fork+0x2e/0x40

> > > [    2.302891] Code: ff 83 c4 10 5b 5e 5f 5d c3 55 89 e5 e8 ca dc fe ff 83 3d 20 bc b4 c1 00 74 13 83 ca ff b8 98 fd b1 c1 e8 e2 4b 04 00 85 c0 75 02 <0f> 0b 5d c3 55 89 e5 56 53 e8 a2 dc fe ff 89 c6 3b 05 8c 40 bd 

> > > [    2.304975] irq event stamp: 308

> > > [    2.305335] hardirqs last  enabled at (307): [<c179d83c>] _raw_spin_unlock_irqrestore+0x4a/0x68

> > > [    2.306195] hardirqs last disabled at (308): [<c179ed38>] common_exception+0x46/0x6e

> > > [    2.306969] softirqs last  enabled at (0): [<c10448e6>] copy_process+0x582/0x16c5

> > > [    2.307720] softirqs last disabled at (0): [<00000000>]   (null)

> > > [    2.308328] ---[ end trace 7197acb79221f1b9 ]---

> > > 

> > > Guess early boot is "special"? :-/

> > 

> > I'm guessing the comment is wrong. I just added the first three patches

> > and was going to try to trigger the first "bug" I had before. Let me

> > check if I have lockdep enabled and see if I see the same thing.

> 

> Mmm, I had it enabled while testing and didn't get the splat.


Hmm not sure if this can help but I think that rebuild_sched_domains()
does _not_ take the hotplug lock before calling partition_sched_domains()
when CONFIG_CPUSETS=n. But it does take it for CONFIG_CPUSETS=y.

Not sure if this is actually the right thing to do, but maybe worth
investigating ?

Thanks,
Quentin
Juri Lelli June 14, 2018, 2:11 p.m. UTC | #6 
On 14/06/18 14:58, Quentin Perret wrote:

[...]

> Hmm not sure if this can help but I think that rebuild_sched_domains()

> does _not_ take the hotplug lock before calling partition_sched_domains()

> when CONFIG_CPUSETS=n. But it does take it for CONFIG_CPUSETS=y.


Did you mean cpuset_mutex?

cpu_hotplug_lock seems to be the problem here..

Can't seem to follow otherwise. :-(
Quentin Perret June 14, 2018, 2:18 p.m. UTC | #7 
On Thursday 14 Jun 2018 at 16:11:18 (+0200), Juri Lelli wrote:
> On 14/06/18 14:58, Quentin Perret wrote:

> 

> [...]

> 

> > Hmm not sure if this can help but I think that rebuild_sched_domains()

> > does _not_ take the hotplug lock before calling partition_sched_domains()

> > when CONFIG_CPUSETS=n. But it does take it for CONFIG_CPUSETS=y.

> 

> Did you mean cpuset_mutex?


Nope, I really meant the cpu_hotplug_lock !

With CONFIG_CPUSETS=n, rebuild_sched_domains() calls
partition_sched_domains() directly:

https://elixir.bootlin.com/linux/latest/source/include/linux/cpuset.h#L255

But with CONFIG_CPUSETS=y, rebuild_sched_domains() calls,
rebuild_sched_domains_locked(), which calls get_online_cpus() which
calls cpus_read_lock(), which does percpu_down_read(&cpu_hotplug_lock).
And all that happens before calling partition_sched_domains().

So yeah, the point I was trying to make is that there is an inconsistency
here, maybe for a good reason ? Maybe related to the issue you're seeing ?

Thanks,
Quentin
Juri Lelli June 14, 2018, 2:30 p.m. UTC | #8 
On 14/06/18 15:18, Quentin Perret wrote:
> On Thursday 14 Jun 2018 at 16:11:18 (+0200), Juri Lelli wrote:

> > On 14/06/18 14:58, Quentin Perret wrote:

> > 

> > [...]

> > 

> > > Hmm not sure if this can help but I think that rebuild_sched_domains()

> > > does _not_ take the hotplug lock before calling partition_sched_domains()

> > > when CONFIG_CPUSETS=n. But it does take it for CONFIG_CPUSETS=y.

> > 

> > Did you mean cpuset_mutex?

> 

> Nope, I really meant the cpu_hotplug_lock !

> 

> With CONFIG_CPUSETS=n, rebuild_sched_domains() calls

> partition_sched_domains() directly:

> 

> https://elixir.bootlin.com/linux/latest/source/include/linux/cpuset.h#L255

> 

> But with CONFIG_CPUSETS=y, rebuild_sched_domains() calls,

> rebuild_sched_domains_locked(), which calls get_online_cpus() which

> calls cpus_read_lock(), which does percpu_down_read(&cpu_hotplug_lock).

> And all that happens before calling partition_sched_domains().


Ah, right!
 
> So yeah, the point I was trying to make is that there is an inconsistency

> here, maybe for a good reason ? Maybe related to the issue you're seeing ?


The config that came with the 0day splat was indeed CONFIG_CPUSETS=n.

So, in this case IIUC we hit the !doms_new branch of partition_sched_
domains, which uses cpu_active_mask (and cpu_possible_mask indirectly).
Should this be still protected by the hotplug lock then?
Quentin Perret June 15, 2018, 8:39 a.m. UTC | #9 
On Thursday 14 Jun 2018 at 16:30:37 (+0200), Juri Lelli wrote:
> On 14/06/18 15:18, Quentin Perret wrote:

> > On Thursday 14 Jun 2018 at 16:11:18 (+0200), Juri Lelli wrote:

> > > On 14/06/18 14:58, Quentin Perret wrote:

> > > 

> > > [...]

> > > 

> > > > Hmm not sure if this can help but I think that rebuild_sched_domains()

> > > > does _not_ take the hotplug lock before calling partition_sched_domains()

> > > > when CONFIG_CPUSETS=n. But it does take it for CONFIG_CPUSETS=y.

> > > 

> > > Did you mean cpuset_mutex?

> > 

> > Nope, I really meant the cpu_hotplug_lock !

> > 

> > With CONFIG_CPUSETS=n, rebuild_sched_domains() calls

> > partition_sched_domains() directly:

> > 

> > https://elixir.bootlin.com/linux/latest/source/include/linux/cpuset.h#L255

> > 

> > But with CONFIG_CPUSETS=y, rebuild_sched_domains() calls,

> > rebuild_sched_domains_locked(), which calls get_online_cpus() which

> > calls cpus_read_lock(), which does percpu_down_read(&cpu_hotplug_lock).

> > And all that happens before calling partition_sched_domains().

> 

> Ah, right!

>  

> > So yeah, the point I was trying to make is that there is an inconsistency

> > here, maybe for a good reason ? Maybe related to the issue you're seeing ?

> 

> The config that came with the 0day splat was indeed CONFIG_CPUSETS=n.

> 

> So, in this case IIUC we hit the !doms_new branch of partition_sched_

> domains, which uses cpu_active_mask (and cpu_possible_mask indirectly).

> Should this be still protected by the hotplug lock then?


Hmm I'm not sure ... But looking at your call trace, it seems that the
issue happens when sched_cpu_deactivate() is called (not sure why this
is called during boot BTW ?), which calls cpuset_update_active_cpus().

And again, for CONFIG_CPUSETS=n, that defaults to a raw call to
partition_sched_domain(), but with ndoms_new=1, and no lock taken.
I'm still not sure if this is done like that for a good reason, or if
this is actually an issue that this patch caught nicely ...

Quentin
diff mbox series

Patch

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 61a1125c1ae4..96eee22fafe8 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1858,6 +1858,7 @@  void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
 	int i, j, n;
 	int new_topology;
 
+	lockdep_assert_cpus_held();
 	mutex_lock(&sched_domains_mutex);
 
 	/* Always unregister in case we don't destroy any domains: */