Null pointer crash at find_idlest_group on db845c w/ linus/master

Null pointer crash at find_idlest_group on db845c w/ linus/master

[John Stultz]

With today's linus/master on db845c running android, I'm able to
fairly easily reproduce the following crash. I've not had a chance to
bisect it yet, but I'm suspecting its connected to Vincent's recent
rework.

If you have any suggestions, or need me to test anything, please let me know.

thanks
-john

[  136.157069] Unable to handle kernel NULL pointer dereference at
virtual address 0000000000000010
[  136.165937] Mem abort info:
[  136.168765]   ESR = 0x96000005
[  136.171862]   EC = 0x25: DABT (current EL), IL = 32 bits
[  136.177229]   SET = 0, FnV = 0
[  136.180320]   EA = 0, S1PTW = 0
[  136.183502] Data abort info:
[  136.186426]   ISV = 0, ISS = 0x00000005
[  136.190302]   CM = 0, WnR = 0
[  136.193316] user pgtable: 4k pages, 39-bit VAs, pgdp=0000000175f7e000
[  136.199814] [0000000000000010] pgd=0000000000000000, pud=0000000000000000
[  136.206666] Internal error: Oops: 96000005 [#1] PREEMPT SMP
[  136.212295] Modules linked in:
[  136.215397] CPU: 7 PID: 50 Comm: kauditd Tainted: G        W
 5.4.0-mainline-11225-g9c5add21ff63 #1252
[  136.225390] Hardware name: Thundercomm Dragonboard 845c (DT)
[  136.231111] pstate: 60c00085 (nZCv daIf +PAN +UAO)
[  136.235971] pc : find_idlest_group.isra.95+0x368/0x690
[  136.241159] lr : find_idlest_group.isra.95+0x210/0x690
[  136.246347] sp : ffffffc01036b890
[  136.249708] x29: ffffffc01036b890 x28: ffffffe7d7450480
[  136.255077] x27: ffffff80f81f0000 x26: 0000000000000000
[  136.260445] x25: 0000000000000000 x24: ffffffc01036b998
[  136.265810] x23: ffffff80f8e40a00 x22: ffffffe7d7719e30
[  136.271175] x21: ffffff80f8f16520 x20: ffffff80f8f16180
[  136.276539] x19: ffffffe7d771a610 x18: ffffffe7d71d1ef0
[  136.281908] x17: 0000000000000000 x16: 0000000000000000
[  136.287274] x15: 0000000000000001 x14: 6f3a753d74786574
[  136.292644] x13: 6e6f637420383637 x12: 632c323135633a30
[  136.298007] x11: 0000000000000070 x10: 0000000000000002
[  136.303371] x9 : 0000000000000000 x8 : 0000000000000075
[  136.308737] x7 : ffffff80f8f16000 x6 : ffffffe7d7450000
[  136.314099] x5 : 0000000000000004 x4 : 0000000000000000
[  136.319465] x3 : 000000000000025c x2 : ffffff80f8f16780
[  136.324836] x1 : 0000000000000000 x0 : 0000000000000002
[  136.330198] Call trace:
[  136.332680]  find_idlest_group.isra.95+0x368/0x690
[  136.337528]  select_task_rq_fair+0x4e4/0xd88
[  136.341848]  try_to_wake_up+0x21c/0x7f8
[  136.345735]  default_wake_function+0x34/0x48
[  136.350053]  pollwake+0x98/0xc8
[  136.353233]  __wake_up_common+0x90/0x158
[  136.357202]  __wake_up_common_lock+0x88/0xd0
[  136.361519]  __wake_up_sync_key+0x40/0x50
[  136.365584]  sock_def_readable+0x44/0x78
[  136.369560]  __netlink_sendskb+0x44/0x58
[  136.373525]  netlink_unicast+0x220/0x258
[  136.377496]  kauditd_send_queue+0xa4/0x158
[  136.381643]  kauditd_thread+0xf4/0x248
[  136.385438]  kthread+0x130/0x138
[  136.388706]  ret_from_fork+0x10/0x1c
[  136.392332] Code: 54001340 7100081f 540016e1 a9478ba1 (f9400821)
[  136.398493] ---[ end trace 47d254973801b2c4 ]---
[  136.403162] Kernel panic - not syncing: Fatal exception
[  136.408445] SMP: stopping secondary CPUs
[  136.412440] Kernel Offset: 0x27c6200000 from 0xffffffc010000000
[  136.418417] PHYS_OFFSET: 0xffffffe140000000
[  136.422655] CPU features: 0x00002,2a80a218

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Valentin Schneider]

Hi John,

On 03/12/2019 19:15, John Stultz wrote:
> With today's linus/master on db845c running android, I'm able to
> fairly easily reproduce the following crash. I've not had a chance to
> bisect it yet, but I'm suspecting its connected to Vincent's recent
> rework.
> 

Thanks for reporting this.

> If you have any suggestions, or need me to test anything, please let me know.
> 
> thanks
> -john
> 
> [  136.157069] Unable to handle kernel NULL pointer dereference at
> virtual address 0000000000000010
> [  136.165937] Mem abort info:
> [  136.168765]   ESR = 0x96000005
> [  136.171862]   EC = 0x25: DABT (current EL), IL = 32 bits
> [  136.177229]   SET = 0, FnV = 0
> [  136.180320]   EA = 0, S1PTW = 0
> [  136.183502] Data abort info:
> [  136.186426]   ISV = 0, ISS = 0x00000005
> [  136.190302]   CM = 0, WnR = 0
> [  136.193316] user pgtable: 4k pages, 39-bit VAs, pgdp=0000000175f7e000
> [  136.199814] [0000000000000010] pgd=0000000000000000, pud=0000000000000000
> [  136.206666] Internal error: Oops: 96000005 [#1] PREEMPT SMP
> [  136.212295] Modules linked in:
> [  136.215397] CPU: 7 PID: 50 Comm: kauditd Tainted: G        W
>  5.4.0-mainline-11225-g9c5add21ff63 #1252
> [  136.225390] Hardware name: Thundercomm Dragonboard 845c (DT)
> [  136.231111] pstate: 60c00085 (nZCv daIf +PAN +UAO)
> [  136.235971] pc : find_idlest_group.isra.95+0x368/0x690
> [  136.241159] lr : find_idlest_group.isra.95+0x210/0x690
> [  136.246347] sp : ffffffc01036b890
> [  136.249708] x29: ffffffc01036b890 x28: ffffffe7d7450480
> [  136.255077] x27: ffffff80f81f0000 x26: 0000000000000000
> [  136.260445] x25: 0000000000000000 x24: ffffffc01036b998
> [  136.265810] x23: ffffff80f8e40a00 x22: ffffffe7d7719e30
> [  136.271175] x21: ffffff80f8f16520 x20: ffffff80f8f16180
> [  136.276539] x19: ffffffe7d771a610 x18: ffffffe7d71d1ef0
> [  136.281908] x17: 0000000000000000 x16: 0000000000000000
> [  136.287274] x15: 0000000000000001 x14: 6f3a753d74786574
> [  136.292644] x13: 6e6f637420383637 x12: 632c323135633a30
> [  136.298007] x11: 0000000000000070 x10: 0000000000000002
> [  136.303371] x9 : 0000000000000000 x8 : 0000000000000075
> [  136.308737] x7 : ffffff80f8f16000 x6 : ffffffe7d7450000
> [  136.314099] x5 : 0000000000000004 x4 : 0000000000000000
> [  136.319465] x3 : 000000000000025c x2 : ffffff80f8f16780
> [  136.324836] x1 : 0000000000000000 x0 : 0000000000000002
> [  136.330198] Call trace:
> [  136.332680]  find_idlest_group.isra.95+0x368/0x690
> [  136.337528]  select_task_rq_fair+0x4e4/0xd88
> [  136.341848]  try_to_wake_up+0x21c/0x7f8
> [  136.345735]  default_wake_function+0x34/0x48
> [  136.350053]  pollwake+0x98/0xc8
> [  136.353233]  __wake_up_common+0x90/0x158
> [  136.357202]  __wake_up_common_lock+0x88/0xd0
> [  136.361519]  __wake_up_sync_key+0x40/0x50
> [  136.365584]  sock_def_readable+0x44/0x78
> [  136.369560]  __netlink_sendskb+0x44/0x58
> [  136.373525]  netlink_unicast+0x220/0x258
> [  136.377496]  kauditd_send_queue+0xa4/0x158
> [  136.381643]  kauditd_thread+0xf4/0x248
> [  136.385438]  kthread+0x130/0x138
> [  136.388706]  ret_from_fork+0x10/0x1c
> [  136.392332] Code: 54001340 7100081f 540016e1 a9478ba1 (f9400821)
> [  136.398493] ---[ end trace 47d254973801b2c4 ]---
> [  136.403162] Kernel panic - not syncing: Fatal exception
> [  136.408445] SMP: stopping secondary CPUs
> [  136.412440] Kernel Offset: 0x27c6200000 from 0xffffffc010000000
> [  136.418417] PHYS_OFFSET: 0xffffffe140000000
> [  136.422655] CPU features: 0x00002,2a80a218
> 

I fed this to decode_stacktrace.sh using your branch, I get:

[  136.157069] Unable to handle kernel NULL pointer dereference at
virtual address 0000000000000010
[  136.165937] Mem abort info:
[  136.168765]   ESR = 0x96000005
[  136.171862]   EC = 0x25: DABT (current EL), IL = 32 bits
[  136.177229]   SET = 0, FnV = 0
[  136.180320]   EA = 0, S1PTW = 0
[  136.183502] Data abort info:
[  136.186426]   ISV = 0, ISS = 0x00000005
[  136.190302]   CM = 0, WnR = 0
[  136.193316] user pgtable: 4k pages, 39-bit VAs, pgdp=0000000175f7e000
[  136.199814] [0000000000000010] pgd=0000000000000000, pud=0000000000000000
[  136.206666] Internal error: Oops: 96000005 [#1] PREEMPT SMP
[  136.212295] Modules linked in:
[  136.215397] CPU: 7 PID: 50 Comm: kauditd Tainted: G        W
5.4.0-mainline-11225-g9c5add21ff63 #1252
[  136.225390] Hardware name: Thundercomm Dragonboard 845c (DT)
[  136.231111] pstate: 60c00085 (nZCv daIf +PAN +UAO)
[  136.235971] pc : find_idlest_group.isra.95+0x368/0x690
[  136.241159] lr : find_idlest_group.isra.95+0x210/0x690
[  136.246347] sp : ffffffc01036b890
[  136.249708] x29: ffffffc01036b890 x28: ffffffe7d7450480
[  136.255077] x27: ffffff80f81f0000 x26: 0000000000000000
[  136.260445] x25: 0000000000000000 x24: ffffffc01036b998
[  136.265810] x23: ffffff80f8e40a00 x22: ffffffe7d7719e30
[  136.271175] x21: ffffff80f8f16520 x20: ffffff80f8f16180
[  136.276539] x19: ffffffe7d771a610 x18: ffffffe7d71d1ef0
[  136.281908] x17: 0000000000000000 x16: 0000000000000000
[  136.287274] x15: 0000000000000001 x14: 6f3a753d74786574
[  136.292644] x13: 6e6f637420383637 x12: 632c323135633a30
[  136.298007] x11: 0000000000000070 x10: 0000000000000002
[  136.303371] x9 : 0000000000000000 x8 : 0000000000000075
[  136.308737] x7 : ffffff80f8f16000 x6 : ffffffe7d7450000
[  136.314099] x5 : 0000000000000004 x4 : 0000000000000000
[  136.319465] x3 : 000000000000025c x2 : ffffff80f8f16780
[  136.324836] x1 : 0000000000000000 x0 : 0000000000000002
[  136.330198] Call trace:
[  136.332680] find_idlest_group.isra.95+0x368/0x690
[  136.337528] select_task_rq_fair (kernel/sched/fair.c:5663 kernel/sched/fair.c:6387)
[  136.341848] try_to_wake_up (kernel/sched/core.c:2397 kernel/sched/core.c:2644)
[  136.345735] default_wake_function (kernel/sched/core.c:4350)
[  136.350053] pollwake (fs/select.c:218)
[  136.353233] __wake_up_common (kernel/sched/wait.c:93)
[  136.357202] __wake_up_common_lock (./include/linux/spinlock.h:393 (discriminator 1) kernel/sched/wait.c:125 (discriminator 1))
[  136.361519] __wake_up_sync_key (kernel/sched/wait.c:191)
[  136.365584] sock_def_readable (./include/asm-generic/bitops/non-atomic.h:106 ./include/net/sock.h:837 ./include/net/sock.h:2208 net/core/sock.c:2798)
[  136.369560] __netlink_sendskb (net/netlink/af_netlink.c:1251)
[  136.373525] netlink_unicast (./include/linux/refcount.h:255 ./include/linux/refcount.h:281 ./include/net/sock.h:1728 net/netlink/af_netlink.c:1257 net/netlink/af_netlink.c:1343)
[  136.377496] kauditd_send_queue (kernel/audit.c:734)
[  136.381643] kauditd_thread (kernel/audit.c:858 (discriminator 4))
[  136.385438] kthread (kernel/kthread.c:684)
[  136.388706] ret_from_fork (arch/arm64/kernel/entry.S:909)
[ 136.392332] Code: 54001340 7100081f 540016e1 a9478ba1 (f9400821)
All code
========
   0:	54001340	b.eq	0x268
   4:	7100081f	cmp	w0, #0x2
   8:	540016e1	b.ne	0x2e4
   c:	a9478ba1	ldp	x1, x2, [x29,#120]
  10:*	f9400821	ldr	x1, [x1,#16]		<-- trapping instruction

Code starting with the faulting instruction
===========================================
   0:	f9400821	ldr	x1, [x1,#16]
[  136.398493] ---[ end trace 47d254973801b2c4 ]---
[  136.403162] Kernel panic - not syncing: Fatal exception
[  136.408445] SMP: stopping secondary CPUs
[  136.412440] Kernel Offset: 0x27c6200000 from 0xffffffc010000000
[  136.418417] PHYS_OFFSET: 0xffffffe140000000

The ISRA makes this a bit annoying to figure out the faulty line.

On a sidenote, I never really looked at what fipa-sra does, so I scrounged
this from GCC:

"""
IPA-SRA is an interprocedural pass that removes unused function return
values (turning functions returning a value which is never used into void
functions), removes unused function parameters.  It can also replace an
aggregate parameter by a set of other parameters representing part of the
original, turning those passed by reference into new ones which pass the
value directly.
"""

I suspect this is GCC getting rid of the 'sd_flag' parameter of
find_idlest_group() which is now unused. 


Back to the thing, the interesting bit is 

   0:	54001340	b.eq	0x268
   4:	7100081f	cmp	w0, #0x2
   8:	540016e1	b.ne	0x2e4
   c:	a9478ba1	ldp	x1, x2, [x29,#120]
  10:*	f9400821	ldr	x1, [x1,#16]		<-- trapping instruction

The misfit task cases seem to match the pattern of the last two lines. Here's
one:

        case group_misfit_task:                                                                                                                                                                             
                /* Select group with the highest max capacity */                                                                                                                                            
                if (local->sgc->max_capacity >= idlest->sgc->max_capacity)                                                                                                                                  
    1754:       a94783a2        ldp     x2, x0, [x29,#120]                                                                                                                                                  
    1758:       f9400801        ldr     x1, [x0,#16]  


Looking at the code, I think I got it. In find_idlest_group() we do
initialize 'idlest_sgs' (just like busiest_stat in LB) but 'idlest' is just
NULL. The latter is dereferenced in update_pick_idlest() just for the misfit
case, which goes boom. And I reviewed the damn thing... Bleh.

Fixup looks easy enough, lemme write one up.

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Valentin Schneider]

On 03/12/2019 23:20, Valentin Schneider wrote:
> Looking at the code, I think I got it. In find_idlest_group() we do
> initialize 'idlest_sgs' (just like busiest_stat in LB) but 'idlest' is just
> NULL. The latter is dereferenced in update_pick_idlest() just for the misfit
> case, which goes boom. And I reviewed the damn thing... Bleh.
> 
> Fixup looks easy enough, lemme write one up.
> 

Wait no, that can't be right. We can only get in there if both 'group' and
'idlest' have the same group_type, which can't be true on the first pass.
So if we go through the misfit stuff, idlest *has* to be set to something.
Bah.

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Valentin Schneider]

On 03/12/2019 23:49, Valentin Schneider wrote:
> On 03/12/2019 23:20, Valentin Schneider wrote:
>> Looking at the code, I think I got it. In find_idlest_group() we do
>> initialize 'idlest_sgs' (just like busiest_stat in LB) but 'idlest' is just
>> NULL. The latter is dereferenced in update_pick_idlest() just for the misfit
>> case, which goes boom. And I reviewed the damn thing... Bleh.
>>
>> Fixup looks easy enough, lemme write one up.
>>
> 
> Wait no, that can't be right. We can only get in there if both 'group' and
> 'idlest' have the same group_type, which can't be true on the first pass.
> So if we go through the misfit stuff, idlest *has* to be set to something.
> Bah.
> 

So I think the thing really is dying on a sched_group->sgc deref (pahole says
sgc is at offset #16), which means we have a NULL sched_group somewhere, but
I don't see how. That can either be 'local' (can't be, first group we visit
and doesn't go through update_pick_idlest()) or 'idlest' (see previous email).

Now, it's bedtime for me, if you get the chance in the meantime can you give
this a shot? I was about to send it out but realized it didn't really make
sense, but you never know...

Also, if it is indeed misfit related, I'm surprised we (Arm folks) haven't
hit it sooner. We've had our scheduler tests running on the LB rework for at
least a month, so we should've hit it.

---
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 08a233e97a01..e19ab7bff0f3 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8348,7 +8348,14 @@ static bool update_pick_idlest(struct sched_group *idlest,
 		return false;
 
 	case group_misfit_task:
-		/* Select group with the highest max capacity */
+		/*
+		 * Select group with the highest max capacity. First group we
+		 * visit gets picked as idlest to allow later capacity
+		 * comparisons.
+		 */
+		if (!idlest)
+			return true;
+
 		if (idlest->sgc->max_capacity >= group->sgc->max_capacity)
 			return false;
 		break;

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[John Stultz]

On Tue, Dec 3, 2019 at 4:13 PM Valentin Schneider
<valentin.schneider@arm.com> wrote:
>
> On 03/12/2019 23:49, Valentin Schneider wrote:
> > On 03/12/2019 23:20, Valentin Schneider wrote:
> >> Looking at the code, I think I got it. In find_idlest_group() we do
> >> initialize 'idlest_sgs' (just like busiest_stat in LB) but 'idlest' is just
> >> NULL. The latter is dereferenced in update_pick_idlest() just for the misfit
> >> case, which goes boom. And I reviewed the damn thing... Bleh.
> >>
> >> Fixup looks easy enough, lemme write one up.
> >>
> >
> > Wait no, that can't be right. We can only get in there if both 'group' and
> > 'idlest' have the same group_type, which can't be true on the first pass.
> > So if we go through the misfit stuff, idlest *has* to be set to something.
> > Bah.
> >
>
> So I think the thing really is dying on a sched_group->sgc deref (pahole says
> sgc is at offset #16), which means we have a NULL sched_group somewhere, but
> I don't see how. That can either be 'local' (can't be, first group we visit
> and doesn't go through update_pick_idlest()) or 'idlest' (see previous email).
>
> Now, it's bedtime for me, if you get the chance in the meantime can you give
> this a shot? I was about to send it out but realized it didn't really make
> sense, but you never know...
>
> Also, if it is indeed misfit related, I'm surprised we (Arm folks) haven't
> hit it sooner. We've had our scheduler tests running on the LB rework for at
> least a month, so we should've hit it.
>
> ---
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 08a233e97a01..e19ab7bff0f3 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8348,7 +8348,14 @@ static bool update_pick_idlest(struct sched_group *idlest,
>                 return false;
>
>         case group_misfit_task:
> -               /* Select group with the highest max capacity */
> +               /*
> +                * Select group with the highest max capacity. First group we
> +                * visit gets picked as idlest to allow later capacity
> +                * comparisons.
> +                */
> +               if (!idlest)
> +                       return true;
> +
>                 if (idlest->sgc->max_capacity >= group->sgc->max_capacity)
>                         return false;
>                 break;

Thanks for the quick patch! Unfortunately I still managed to trip it
with this patch :(

thanks
-john

[  191.807900] Unable to handle kernel NULL pointer dereference at
virtual address 0000000000000010
[  191.814822] audit: audit_lost=284 audit_rate_limit=5 audit_backlog_limit=64
[  191.816779] Mem abort info:
[  191.816782]   ESR = 0x96000005
[  191.816786]   EC = 0x25: DABT (current EL), IL = 32 bits
[  191.816789]   SET = 0, FnV = 0
[  191.816791]   EA = 0, S1PTW = 0
[  191.816793] Data abort info:
[  191.816797]   ISV = 0, ISS = 0x00000005
[  191.816799]   CM = 0, WnR = 0
[  191.816804] user pgtable: 4k pages, 39-bit VAs, pgdp=00000001719fc000
[  191.816809] [0000000000000010] pgd=0000000000000000, pud=0000000000000000
[  191.823849] audit: rate limit exceeded
[  191.826660] Internal error: Oops: 96000005 [#1] PREEMPT SMP
[  191.873941] Modules linked in:
[  191.877043] CPU: 7 PID: 50 Comm: kauditd Tainted: G        W
 5.4.0-mainline-11226-g81d7081fb03e #1276
[  191.887039] Hardware name: Thundercomm Dragonboard 845c (DT)
[  191.892751] pstate: 60c00085 (nZCv daIf +PAN +UAO)
[  191.897612] pc : find_idlest_group.isra.95+0x368/0x690
[  191.902809] lr : find_idlest_group.isra.95+0x104/0x690
[  191.908002] sp : ffffffc01036b890
[  191.911356] x29: ffffffc01036b890 x28: ffffffe471250480
[  191.916720] x27: ffffff80f1828000 x26: 0000000000000001
[  191.922082] x25: 0000000000000000 x24: ffffffc01036b998
[  191.927447] x23: ffffff80f8e41a00 x22: ffffffe471519e30
[  191.932811] x21: ffffff80f8f16aa0 x20: ffffff80f8f16e80
[  191.938173] x19: ffffffe47151a610 x18: ffffffe470fd1ef0
[  191.943543] x17: 0000000000000000 x16: 0000000000000000
[  191.948912] x15: 0000000000000001 x14: 632c323135633a30
[  191.954275] x13: 733a656c69665f61 x12: 7461645f7070613a
[  191.959636] x11: 0000000000000730 x10: 000000000000025d
[  191.964998] x9 : 0000000000003b15 x8 : 0000000000000075
[  191.970361] x7 : ffffff80f8f16200 x6 : ffffffe471250000
[  191.975728] x5 : 0000000000000000 x4 : 0000000000000008
[  191.981090] x3 : ffffff80f8f16100 x2 : ffffff80f8f16d00
[  191.986452] x1 : 0000000000000000 x0 : 0000000000000002
[  191.991815] Call trace:
[  191.994309]  find_idlest_group.isra.95+0x368/0x690
[  191.999155]  select_task_rq_fair+0x4e4/0xd88
[  192.003476]  try_to_wake_up+0x21c/0x7f8
[  192.007353]  default_wake_function+0x34/0x48
[  192.011675]  pollwake+0x98/0xc8
[  192.014863]  __wake_up_common+0x90/0x158
[  192.018838]  __wake_up_common_lock+0x88/0xd0
[  192.023163]  __wake_up_sync_key+0x40/0x50
[  192.027228]  sock_def_readable+0x44/0x78
[  192.031197]  __netlink_sendskb+0x44/0x58
[  192.035170]  netlink_unicast+0x220/0x258
[  192.039143]  kauditd_send_queue+0xa4/0x158
[  192.043292]  kauditd_thread+0xf4/0x248
[  192.047085]  kthread+0x130/0x138
[  192.050355]  ret_from_fork+0x10/0x1c
[  192.053984] Code: 54001260 7100081f 54001601 a9478ba1 (f9400821)
[  192.060141] ---[ end trace c650b8d609e54a39 ]---
[  192.064812] Kernel panic - not syncing: Fatal exception

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Vincent Guittot]

Hi John,

On Tue, 3 Dec 2019 at 20:15, John Stultz <john.stultz@linaro.org> wrote:
>
> With today's linus/master on db845c running android, I'm able to
> fairly easily reproduce the following crash. I've not had a chance to
> bisect it yet, but I'm suspecting its connected to Vincent's recent
> rework.

Does the crash happen randomly or after a specific action ?
I have a db845 so I can try to reproduce it locally.

>
> If you have any suggestions, or need me to test anything, please let me know.
>
> thanks
> -john
>
> [  136.157069] Unable to handle kernel NULL pointer dereference at
> virtual address 0000000000000010
> [  136.165937] Mem abort info:
> [  136.168765]   ESR = 0x96000005
> [  136.171862]   EC = 0x25: DABT (current EL), IL = 32 bits
> [  136.177229]   SET = 0, FnV = 0
> [  136.180320]   EA = 0, S1PTW = 0
> [  136.183502] Data abort info:
> [  136.186426]   ISV = 0, ISS = 0x00000005
> [  136.190302]   CM = 0, WnR = 0
> [  136.193316] user pgtable: 4k pages, 39-bit VAs, pgdp=0000000175f7e000
> [  136.199814] [0000000000000010] pgd=0000000000000000, pud=0000000000000000
> [  136.206666] Internal error: Oops: 96000005 [#1] PREEMPT SMP
> [  136.212295] Modules linked in:
> [  136.215397] CPU: 7 PID: 50 Comm: kauditd Tainted: G        W
>  5.4.0-mainline-11225-g9c5add21ff63 #1252
> [  136.225390] Hardware name: Thundercomm Dragonboard 845c (DT)
> [  136.231111] pstate: 60c00085 (nZCv daIf +PAN +UAO)
> [  136.235971] pc : find_idlest_group.isra.95+0x368/0x690
> [  136.241159] lr : find_idlest_group.isra.95+0x210/0x690
> [  136.246347] sp : ffffffc01036b890
> [  136.249708] x29: ffffffc01036b890 x28: ffffffe7d7450480
> [  136.255077] x27: ffffff80f81f0000 x26: 0000000000000000
> [  136.260445] x25: 0000000000000000 x24: ffffffc01036b998
> [  136.265810] x23: ffffff80f8e40a00 x22: ffffffe7d7719e30
> [  136.271175] x21: ffffff80f8f16520 x20: ffffff80f8f16180
> [  136.276539] x19: ffffffe7d771a610 x18: ffffffe7d71d1ef0
> [  136.281908] x17: 0000000000000000 x16: 0000000000000000
> [  136.287274] x15: 0000000000000001 x14: 6f3a753d74786574
> [  136.292644] x13: 6e6f637420383637 x12: 632c323135633a30
> [  136.298007] x11: 0000000000000070 x10: 0000000000000002
> [  136.303371] x9 : 0000000000000000 x8 : 0000000000000075
> [  136.308737] x7 : ffffff80f8f16000 x6 : ffffffe7d7450000
> [  136.314099] x5 : 0000000000000004 x4 : 0000000000000000
> [  136.319465] x3 : 000000000000025c x2 : ffffff80f8f16780
> [  136.324836] x1 : 0000000000000000 x0 : 0000000000000002
> [  136.330198] Call trace:
> [  136.332680]  find_idlest_group.isra.95+0x368/0x690
> [  136.337528]  select_task_rq_fair+0x4e4/0xd88
> [  136.341848]  try_to_wake_up+0x21c/0x7f8
> [  136.345735]  default_wake_function+0x34/0x48
> [  136.350053]  pollwake+0x98/0xc8
> [  136.353233]  __wake_up_common+0x90/0x158
> [  136.357202]  __wake_up_common_lock+0x88/0xd0
> [  136.361519]  __wake_up_sync_key+0x40/0x50
> [  136.365584]  sock_def_readable+0x44/0x78
> [  136.369560]  __netlink_sendskb+0x44/0x58
> [  136.373525]  netlink_unicast+0x220/0x258
> [  136.377496]  kauditd_send_queue+0xa4/0x158
> [  136.381643]  kauditd_thread+0xf4/0x248
> [  136.385438]  kthread+0x130/0x138
> [  136.388706]  ret_from_fork+0x10/0x1c
> [  136.392332] Code: 54001340 7100081f 540016e1 a9478ba1 (f9400821)
> [  136.398493] ---[ end trace 47d254973801b2c4 ]---
> [  136.403162] Kernel panic - not syncing: Fatal exception
> [  136.408445] SMP: stopping secondary CPUs
> [  136.412440] Kernel Offset: 0x27c6200000 from 0xffffffc010000000
> [  136.418417] PHYS_OFFSET: 0xffffffe140000000
> [  136.422655] CPU features: 0x00002,2a80a218

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Vincent Guittot]

On Wed, 4 Dec 2019 at 09:06, Vincent Guittot <vincent.guittot@linaro.org> wrote:
>
> Hi John,
>
> On Tue, 3 Dec 2019 at 20:15, John Stultz <john.stultz@linaro.org> wrote:
> >
> > With today's linus/master on db845c running android, I'm able to
> > fairly easily reproduce the following crash. I've not had a chance to
> > bisect it yet, but I'm suspecting its connected to Vincent's recent
> > rework.
>
> Does the crash happen randomly or after a specific action ?
> I have a db845 so I can try to reproduce it locally.
>
> >
> > If you have any suggestions, or need me to test anything, please let me know.
> >
> > thanks
> > -john
> >
> > [  136.157069] Unable to handle kernel NULL pointer dereference at
> > virtual address 0000000000000010
> > [  136.165937] Mem abort info:
> > [  136.168765]   ESR = 0x96000005
> > [  136.171862]   EC = 0x25: DABT (current EL), IL = 32 bits
> > [  136.177229]   SET = 0, FnV = 0
> > [  136.180320]   EA = 0, S1PTW = 0
> > [  136.183502] Data abort info:
> > [  136.186426]   ISV = 0, ISS = 0x00000005
> > [  136.190302]   CM = 0, WnR = 0
> > [  136.193316] user pgtable: 4k pages, 39-bit VAs, pgdp=0000000175f7e000
> > [  136.199814] [0000000000000010] pgd=0000000000000000, pud=0000000000000000
> > [  136.206666] Internal error: Oops: 96000005 [#1] PREEMPT SMP
> > [  136.212295] Modules linked in:
> > [  136.215397] CPU: 7 PID: 50 Comm: kauditd Tainted: G        W
> >  5.4.0-mainline-11225-g9c5add21ff63 #1252

Do you have the sha1 that you use for your test ?
The one above doesn't seem to point on a valid commit in linus/master

> > [  136.225390] Hardware name: Thundercomm Dragonboard 845c (DT)
> > [  136.231111] pstate: 60c00085 (nZCv daIf +PAN +UAO)
> > [  136.235971] pc : find_idlest_group.isra.95+0x368/0x690
> > [  136.241159] lr : find_idlest_group.isra.95+0x210/0x690
> > [  136.246347] sp : ffffffc01036b890
> > [  136.249708] x29: ffffffc01036b890 x28: ffffffe7d7450480
> > [  136.255077] x27: ffffff80f81f0000 x26: 0000000000000000
> > [  136.260445] x25: 0000000000000000 x24: ffffffc01036b998
> > [  136.265810] x23: ffffff80f8e40a00 x22: ffffffe7d7719e30
> > [  136.271175] x21: ffffff80f8f16520 x20: ffffff80f8f16180
> > [  136.276539] x19: ffffffe7d771a610 x18: ffffffe7d71d1ef0
> > [  136.281908] x17: 0000000000000000 x16: 0000000000000000
> > [  136.287274] x15: 0000000000000001 x14: 6f3a753d74786574
> > [  136.292644] x13: 6e6f637420383637 x12: 632c323135633a30
> > [  136.298007] x11: 0000000000000070 x10: 0000000000000002
> > [  136.303371] x9 : 0000000000000000 x8 : 0000000000000075
> > [  136.308737] x7 : ffffff80f8f16000 x6 : ffffffe7d7450000
> > [  136.314099] x5 : 0000000000000004 x4 : 0000000000000000
> > [  136.319465] x3 : 000000000000025c x2 : ffffff80f8f16780
> > [  136.324836] x1 : 0000000000000000 x0 : 0000000000000002
> > [  136.330198] Call trace:
> > [  136.332680]  find_idlest_group.isra.95+0x368/0x690
> > [  136.337528]  select_task_rq_fair+0x4e4/0xd88
> > [  136.341848]  try_to_wake_up+0x21c/0x7f8
> > [  136.345735]  default_wake_function+0x34/0x48
> > [  136.350053]  pollwake+0x98/0xc8
> > [  136.353233]  __wake_up_common+0x90/0x158
> > [  136.357202]  __wake_up_common_lock+0x88/0xd0
> > [  136.361519]  __wake_up_sync_key+0x40/0x50
> > [  136.365584]  sock_def_readable+0x44/0x78
> > [  136.369560]  __netlink_sendskb+0x44/0x58
> > [  136.373525]  netlink_unicast+0x220/0x258
> > [  136.377496]  kauditd_send_queue+0xa4/0x158
> > [  136.381643]  kauditd_thread+0xf4/0x248
> > [  136.385438]  kthread+0x130/0x138
> > [  136.388706]  ret_from_fork+0x10/0x1c
> > [  136.392332] Code: 54001340 7100081f 540016e1 a9478ba1 (f9400821)
> > [  136.398493] ---[ end trace 47d254973801b2c4 ]---
> > [  136.403162] Kernel panic - not syncing: Fatal exception
> > [  136.408445] SMP: stopping secondary CPUs
> > [  136.412440] Kernel Offset: 0x27c6200000 from 0xffffffc010000000
> > [  136.418417] PHYS_OFFSET: 0xffffffe140000000
> > [  136.422655] CPU features: 0x00002,2a80a218

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Qais Yousef]

On 12/04/19 09:06, Vincent Guittot wrote:
> Hi John,
> 
> On Tue, 3 Dec 2019 at 20:15, John Stultz <john.stultz@linaro.org> wrote:
> >
> > With today's linus/master on db845c running android, I'm able to
> > fairly easily reproduce the following crash. I've not had a chance to
> > bisect it yet, but I'm suspecting its connected to Vincent's recent
> > rework.
> 
> Does the crash happen randomly or after a specific action ?
> I have a db845 so I can try to reproduce it locally.

Isn't there a chance we use local_sgs without initializing it in that function?
AFAICS we define local_sgs on the stack but not always could be populated with
the right values. I can't see tmp_sgs being used in the function too. Could
this cause the/a problem?

 8377         struct sg_lb_stats local_sgs, tmp_sgs;
.
.
.
 8399                 if (local_group) {
 8400                         sgs = &local_sgs;
 8401                         local = group;
 8402                 } else {
 8403                         sgs = &tmp_sgs;
 8404                 }
 8405
 8406                 update_sg_wakeup_stats(sd, group, sgs, p);

Cheers

--
Qais Youef

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Valentin Schneider]

On 04/12/2019 08:22, Vincent Guittot wrote:
>>>  5.4.0-mainline-11225-g9c5add21ff63 #1252
> 
> Do you have the sha1 that you use for your test ?
> The one above doesn't seem to point on a valid commit in linus/master
> 

That would be https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/db845c-mainline-WIP

There's a db845_defconfig there too which I expect should be used to compile
this kernel.

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Valentin Schneider]

On 04/12/2019 03:46, John Stultz wrote:
> Thanks for the quick patch! Unfortunately I still managed to trip it
> with this patch :(
> 

I think I would've been more confused if it had fixed it than if it hadn't.
Figured it was worth a shot anyway.


I have a DB845 stashed somewhere but I've never used one, so it's probably
going to take me some time to get it up and running.

In the meantime, I'd suggest running it again with the following appended
patch. It gets rid of the unused parameter, which gets rid of the isra,
which should let us get a faulty line number more easily than by rummaging
through objdump.

---
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 08a233e97a01..4c3a8f188d45 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5575,8 +5575,7 @@ static int wake_affine(struct sched_domain *sd, struct task_struct *p,
 }
 
 static struct sched_group *
-find_idlest_group(struct sched_domain *sd, struct task_struct *p,
-		  int this_cpu, int sd_flag);
+find_idlest_group(struct sched_domain *sd, struct task_struct *p, int this_cpu);
 
 /*
  * find_idlest_group_cpu - find the idlest CPU among the CPUs in the group.
@@ -5665,7 +5664,7 @@ static inline int find_idlest_cpu(struct sched_domain *sd, struct task_struct *p
 			continue;
 		}
 
-		group = find_idlest_group(sd, p, cpu, sd_flag);
+		group = find_idlest_group(sd, p, cpu);
 		if (!group) {
 			sd = sd->child;
 			continue;
@@ -8370,8 +8369,7 @@ static bool update_pick_idlest(struct sched_group *idlest,
  * Assumes p is allowed on at least one CPU in sd.
  */
 static struct sched_group *
-find_idlest_group(struct sched_domain *sd, struct task_struct *p,
-		  int this_cpu, int sd_flag)
+find_idlest_group(struct sched_domain *sd, struct task_struct *p, int this_cpu)
 {
 	struct sched_group *idlest = NULL, *local = NULL, *group = sd->groups;
 	struct sg_lb_stats local_sgs, tmp_sgs;

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Valentin Schneider]

On 04/12/2019 09:42, Qais Yousef wrote:
> On 12/04/19 09:06, Vincent Guittot wrote:
>> Hi John,
>>
>> On Tue, 3 Dec 2019 at 20:15, John Stultz <john.stultz@linaro.org> wrote:
>>>
>>> With today's linus/master on db845c running android, I'm able to
>>> fairly easily reproduce the following crash. I've not had a chance to
>>> bisect it yet, but I'm suspecting its connected to Vincent's recent
>>> rework.
>>
>> Does the crash happen randomly or after a specific action ?
>> I have a db845 so I can try to reproduce it locally.
> 
> Isn't there a chance we use local_sgs without initializing it in that function?
> AFAICS we define local_sgs on the stack but not always could be populated with
> the right values. I can't see tmp_sgs being used in the function too. Could
> this cause the/a problem?
> 
>  8377         struct sg_lb_stats local_sgs, tmp_sgs;
> .
> .
> .
>  8399                 if (local_group) {
>  8400                         sgs = &local_sgs;
>  8401                         local = group;
>  8402                 } else {
>  8403                         sgs = &tmp_sgs;
>  8404                 }
>  8405
>  8406                 update_sg_wakeup_stats(sd, group, sgs, p);
> 

local_sgs is initialized in the first update_sg_wakeup_stats() entry
(the local sched_group is always the first one in the sd->groups list),
and tmp_sgs is whatever non local group we're iterating over, see:

		if (local_group) {
			sgs = &local_sgs;
			local = group;
		} else {
			sgs = &tmp_sgs;
		}

and that sgs is populated in

		update_sg_wakeup_stats(sd, group, sgs, p);


> Cheers
> 
> --
> Qais Youef
> 

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Vincent Guittot]

Le Wednesday 04 Dec 2019 à 09:42:17 (+0000), Qais Yousef a écrit :
> On 12/04/19 09:06, Vincent Guittot wrote:
> > Hi John,
> > 
> > On Tue, 3 Dec 2019 at 20:15, John Stultz <john.stultz@linaro.org> wrote:
> > >
> > > With today's linus/master on db845c running android, I'm able to
> > > fairly easily reproduce the following crash. I've not had a chance to
> > > bisect it yet, but I'm suspecting its connected to Vincent's recent
> > > rework.
> > 
> > Does the crash happen randomly or after a specific action ?
> > I have a db845 so I can try to reproduce it locally.
> 
> Isn't there a chance we use local_sgs without initializing it in that function?

Normally not because the cpu belongs to its sched_domain

Now, we test that a group has at least one allowed CPU for the task so we
could skip the local group with the correct/wrong p->cpus_ptr

The path is used for fork/exec ibut also for wakeup path for b.L when the task doesn't fit in the CPUs

So we can probably imagine a scenario where we change task affinity while
sleeping. If the wakeup happens on a CPU that belongs to the group that is not
allowed, we can imagine that we skip the local_group

John,

Could you try the fix below ?

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 08a233e..bcd216d 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8417,6 +8417,10 @@ find_idlest_group(struct sched_domain *sd, struct task_struct *p,
        if (!idlest)
                return NULL;
 
+       /* The local group has been skipped because of cpu affinity */
+       if (!local)
+               return idlest;
+
        /*
         * If the local group is idler than the selected idlest group
         * don't try and push the task.


>
> AFAICS we define local_sgs on the stack but not always could be populated with
> the right values. I can't see tmp_sgs being used in the function too. Could
> this cause the/a problem?
> 
>  8377         struct sg_lb_stats local_sgs, tmp_sgs;
> .
> .
> .
>  8399                 if (local_group) {
>  8400                         sgs = &local_sgs;
>  8401                         local = group;
>  8402                 } else {
>  8403                         sgs = &tmp_sgs;
>  8404                 }
>  8405
>  8406                 update_sg_wakeup_stats(sd, group, sgs, p);
> 
> Cheers
> 
> --
> Qais Youef

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Valentin Schneider]

On 04/12/2019 10:09, Vincent Guittot wrote:
> Now, we test that a group has at least one allowed CPU for the task so we
> could skip the local group with the correct/wrong p->cpus_ptr
> 
> The path is used for fork/exec ibut also for wakeup path for b.L when the task doesn't fit in the CPUs
> 
> So we can probably imagine a scenario where we change task affinity while
> sleeping. If the wakeup happens on a CPU that belongs to the group that is not
> allowed, we can imagine that we skip the local_group
> 

Shoot, I think you're right. If it is the local group that is NULL, then
we most likely splat on:

		if (local->sgc->max_capacity >= idlest->sgc->max_capacity)
			return NULL;

We don't splat before because we just use local_sgs, which is uninitialized
but on the stack.

Also; does it really have to involve an affinity "race"? AFAIU affinity
could have been changed a while back, but the waking CPU isn't allowed
so we skip the local_group (in simpler cases where each CPU is a group).

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Vincent Guittot]

On Wed, 4 Dec 2019 at 11:41, Valentin Schneider
<valentin.schneider@arm.com> wrote:
>
> On 04/12/2019 10:09, Vincent Guittot wrote:
> > Now, we test that a group has at least one allowed CPU for the task so we
> > could skip the local group with the correct/wrong p->cpus_ptr
> >
> > The path is used for fork/exec ibut also for wakeup path for b.L when the task doesn't fit in the CPUs
> >
> > So we can probably imagine a scenario where we change task affinity while
> > sleeping. If the wakeup happens on a CPU that belongs to the group that is not
> > allowed, we can imagine that we skip the local_group
> >
>
> Shoot, I think you're right. If it is the local group that is NULL, then
> we most likely splat on:
>
>                 if (local->sgc->max_capacity >= idlest->sgc->max_capacity)
>                         return NULL;
>
> We don't splat before because we just use local_sgs, which is uninitialized
> but on the stack.
>
> Also; does it really have to involve an affinity "race"? AFAIU affinity
> could have been changed a while back, but the waking CPU isn't allowed
> so we skip the local_group (in simpler cases where each CPU is a group).

In fact, this will depend of the uninitialized values of local_sgs. I
have been able to reproduce the situation where we skip local group
but not to trigger the crash because the values already in the stack
don't trigger the misfit comparison.

I  wait for John feedback to confirm that this fix his problem and
will send a clean version of the patch
>
>

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Qais Yousef]

On 12/04/19 13:08, Vincent Guittot wrote:
> On Wed, 4 Dec 2019 at 11:41, Valentin Schneider
> <valentin.schneider@arm.com> wrote:
> >
> > On 04/12/2019 10:09, Vincent Guittot wrote:
> > > Now, we test that a group has at least one allowed CPU for the task so we
> > > could skip the local group with the correct/wrong p->cpus_ptr
> > >
> > > The path is used for fork/exec ibut also for wakeup path for b.L when the task doesn't fit in the CPUs
> > >
> > > So we can probably imagine a scenario where we change task affinity while
> > > sleeping. If the wakeup happens on a CPU that belongs to the group that is not
> > > allowed, we can imagine that we skip the local_group
> > >
> >
> > Shoot, I think you're right. If it is the local group that is NULL, then
> > we most likely splat on:
> >
> >                 if (local->sgc->max_capacity >= idlest->sgc->max_capacity)
> >                         return NULL;
> >
> > We don't splat before because we just use local_sgs, which is uninitialized
> > but on the stack.
> >
> > Also; does it really have to involve an affinity "race"? AFAIU affinity
> > could have been changed a while back, but the waking CPU isn't allowed
> > so we skip the local_group (in simpler cases where each CPU is a group).
> 
> In fact, this will depend of the uninitialized values of local_sgs. I
> have been able to reproduce the situation where we skip local group
> but not to trigger the crash because the values already in the stack
> don't trigger the misfit comparison.

Will it be expensive to initialize local_sgs = {0}?

--
Qais Yousef

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Vincent Guittot]

On Wed, 4 Dec 2019 at 14:32, Qais Yousef <qais.yousef@arm.com> wrote:
>
> On 12/04/19 13:08, Vincent Guittot wrote:
> > On Wed, 4 Dec 2019 at 11:41, Valentin Schneider
> > <valentin.schneider@arm.com> wrote:
> > >
> > > On 04/12/2019 10:09, Vincent Guittot wrote:
> > > > Now, we test that a group has at least one allowed CPU for the task so we
> > > > could skip the local group with the correct/wrong p->cpus_ptr
> > > >
> > > > The path is used for fork/exec ibut also for wakeup path for b.L when the task doesn't fit in the CPUs
> > > >
> > > > So we can probably imagine a scenario where we change task affinity while
> > > > sleeping. If the wakeup happens on a CPU that belongs to the group that is not
> > > > allowed, we can imagine that we skip the local_group
> > > >
> > >
> > > Shoot, I think you're right. If it is the local group that is NULL, then
> > > we most likely splat on:
> > >
> > >                 if (local->sgc->max_capacity >= idlest->sgc->max_capacity)
> > >                         return NULL;
> > >
> > > We don't splat before because we just use local_sgs, which is uninitialized
> > > but on the stack.
> > >
> > > Also; does it really have to involve an affinity "race"? AFAIU affinity
> > > could have been changed a while back, but the waking CPU isn't allowed
> > > so we skip the local_group (in simpler cases where each CPU is a group).
> >
> > In fact, this will depend of the uninitialized values of local_sgs. I
> > have been able to reproduce the situation where we skip local group
> > but not to trigger the crash because the values already in the stack
> > don't trigger the misfit comparison.
>
> Will it be expensive to initialize local_sgs = {0}?

if we want to initialize local_sgs, it should be something like
local_sgs =  {
.avg_load = UINT_MAX,
.group_type = group_overloaded,
};

to make sure that we will not select local. This doesn't reflect any
kind of reality whereas local=NULL is more meaningful and more robust
IMO


>
> --
> Qais Yousef

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Qais Yousef]

On 12/04/19 14:48, Vincent Guittot wrote:
> if we want to initialize local_sgs, it should be something like
> local_sgs =  {
> .avg_load = UINT_MAX,
> .group_type = group_overloaded,
> };

+1

> 
> to make sure that we will not select local. This doesn't reflect any
> kind of reality whereas local=NULL is more meaningful and more robust
> IMO

It's just defensive programming from my side :) I don't feel strongly about it
though.

Thanks

--
Qais Yousef

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Valentin Schneider]

On 04/12/2019 12:08, Vincent Guittot wrote:
>> Also; does it really have to involve an affinity "race"? AFAIU affinity
>> could have been changed a while back, but the waking CPU isn't allowed
>> so we skip the local_group (in simpler cases where each CPU is a group).
> 
> In fact, this will depend of the uninitialized values of local_sgs. I
> have been able to reproduce the situation where we skip local group
> but not to trigger the crash because the values already in the stack
> don't trigger the misfit comparison.
> 

One more thing, DB845 has a single DynamIQ cluster that is represented as a
flat hierarchy (unlike regular big.LITTLE, see
arch/arm64/boot/dts/qcom/sdm845.dtsi) so we'll just have a single MC level
with groups being individual CPUs, making the bug easier to reproduce
(than on regular big.LITTLE, that is). 

> I  wait for John feedback to confirm that this fix his problem and
> will send a clean version of the patch
>>
>>

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[John Stultz]

On Wed, Dec 4, 2019 at 2:09 AM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> Le Wednesday 04 Dec 2019 à 09:42:17 (+0000), Qais Yousef a écrit :
> > On 12/04/19 09:06, Vincent Guittot wrote:
> > > Hi John,
> > >
> > > On Tue, 3 Dec 2019 at 20:15, John Stultz <john.stultz@linaro.org> wrote:
> > > >
> > > > With today's linus/master on db845c running android, I'm able to
> > > > fairly easily reproduce the following crash. I've not had a chance to
> > > > bisect it yet, but I'm suspecting its connected to Vincent's recent
> > > > rework.
> > >
> > > Does the crash happen randomly or after a specific action ?
> > > I have a db845 so I can try to reproduce it locally.
> >
> > Isn't there a chance we use local_sgs without initializing it in that function?
>
> Normally not because the cpu belongs to its sched_domain
>
> Now, we test that a group has at least one allowed CPU for the task so we
> could skip the local group with the correct/wrong p->cpus_ptr
>
> The path is used for fork/exec ibut also for wakeup path for b.L when the task doesn't fit in the CPUs
>
> So we can probably imagine a scenario where we change task affinity while
> sleeping. If the wakeup happens on a CPU that belongs to the group that is not
> allowed, we can imagine that we skip the local_group
>
> John,
>
> Could you try the fix below ?
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 08a233e..bcd216d 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8417,6 +8417,10 @@ find_idlest_group(struct sched_domain *sd, struct task_struct *p,
>         if (!idlest)
>                 return NULL;
>
> +       /* The local group has been skipped because of cpu affinity */
> +       if (!local)
> +               return idlest;
> +
>         /*
>          * If the local group is idler than the selected idlest group
>          * don't try and push the task.

This patch does seem to solve the issue for me! Thanks so much!

Tested-by: John Stultz <john.stultz@linaro.org>
-john

Re: Null pointer crash at find_idlest_group on db845c w/ linus/master

[Vincent Guittot]

On Wed, 4 Dec 2019 at 19:16, John Stultz <john.stultz@linaro.org> wrote:
>
> On Wed, Dec 4, 2019 at 2:09 AM Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > Le Wednesday 04 Dec 2019 à 09:42:17 (+0000), Qais Yousef a écrit :
> > > On 12/04/19 09:06, Vincent Guittot wrote:
> > > > Hi John,
> > > >
> > > > On Tue, 3 Dec 2019 at 20:15, John Stultz <john.stultz@linaro.org> wrote:
> > > > >
> > > > > With today's linus/master on db845c running android, I'm able to
> > > > > fairly easily reproduce the following crash. I've not had a chance to
> > > > > bisect it yet, but I'm suspecting its connected to Vincent's recent
> > > > > rework.
> > > >
> > > > Does the crash happen randomly or after a specific action ?
> > > > I have a db845 so I can try to reproduce it locally.
> > >
> > > Isn't there a chance we use local_sgs without initializing it in that function?
> >
> > Normally not because the cpu belongs to its sched_domain
> >
> > Now, we test that a group has at least one allowed CPU for the task so we
> > could skip the local group with the correct/wrong p->cpus_ptr
> >
> > The path is used for fork/exec ibut also for wakeup path for b.L when the task doesn't fit in the CPUs
> >
> > So we can probably imagine a scenario where we change task affinity while
> > sleeping. If the wakeup happens on a CPU that belongs to the group that is not
> > allowed, we can imagine that we skip the local_group
> >
> > John,
> >
> > Could you try the fix below ?
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 08a233e..bcd216d 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -8417,6 +8417,10 @@ find_idlest_group(struct sched_domain *sd, struct task_struct *p,
> >         if (!idlest)
> >                 return NULL;
> >
> > +       /* The local group has been skipped because of cpu affinity */
> > +       if (!local)
> > +               return idlest;
> > +
> >         /*
> >          * If the local group is idler than the selected idlest group
> >          * don't try and push the task.
>
> This patch does seem to solve the issue for me! Thanks so much!

Thanks for testing

>
> Tested-by: John Stultz <john.stultz@linaro.org>
> -john