Skip to content

Commit

Permalink
sched: Fix nx_waitid(), nx_waitpid(), nxtask_exithook() for SMP
Browse files Browse the repository at this point in the history
Summary:
- I noticed waitpid_test stops with lc823450-xgevk:rndis
- The condition was CONFIG_DEBUG_ASSERTION=y
- Actually, the child task sent SIGCHILD but the parent couldn't catch the signal
- Then, I found that nx_waitid(), nx_waitpid() use sched_lock()
- However, a parent task and a child task are running on different CPUs
- So, sched_lock() is not enough and need to use a critical section
- Also, signal handling in nxtask_exithook() must be done in a critical section

Impact:
- SMP only

Testing:
- Tested with ostest with the following configurations
- lc823450-xgevk:rndis (CONFIG_DEBUG_ASSERTION=y and n)
- spresense:smp
- spresense:wifi_smp (NCPUS=2 and 4)
- sabre-6quad:smp (QEMU)
- esp32-core:smp (QEMU)
- maix-bit:smp (QEMU)
- sim:smp

Signed-off-by: Masayuki Ishikawa <[email protected]>
  • Loading branch information
masayuki2009 authored and xiaoxiang781216 committed Nov 15, 2020
1 parent 0cf6614 commit 4cc38ca
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 4 deletions.
13 changes: 13 additions & 0 deletions sched/sched/sched_waitid.c
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,14 @@ int nx_waitid(int idtype, id_t id, FAR siginfo_t *info, int options)
sigemptyset(&set);
nxsig_addset(&set, SIGCHLD);

/* NOTE: sched_lock() is not enough for SMP
* because the child task is running on another CPU
*/

#ifdef CONFIG_SMP
irqstate_t flags = enter_critical_section();
#endif

/* Disable pre-emption so that nothing changes while the loop executes */

sched_lock();
Expand Down Expand Up @@ -379,6 +387,11 @@ int nx_waitid(int idtype, id_t id, FAR siginfo_t *info, int options)

errout:
sched_unlock();

#ifdef CONFIG_SMP
leave_critical_section(flags);
#endif

return ret;
}

Expand Down
34 changes: 30 additions & 4 deletions sched/sched/sched_waitpid.c
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,14 @@ pid_t nx_waitpid(pid_t pid, int *stat_loc, int options)

DEBUGASSERT(stat_loc);

/* NOTE: sched_lock() is not enough for SMP
* because the child task is running on another CPU
*/

#ifdef CONFIG_SMP
irqstate_t flags = enter_critical_section();
#endif

/* Disable pre-emption so that nothing changes in the following tests */

sched_lock();
Expand Down Expand Up @@ -158,11 +166,15 @@ pid_t nx_waitpid(pid_t pid, int *stat_loc, int options)

/* On success, return the PID */

sched_unlock();
return pid;
ret = pid;

errout:
sched_unlock();

#ifdef CONFIG_SMP
leave_critical_section(flags);
#endif

return ret;
}

Expand Down Expand Up @@ -199,6 +211,14 @@ pid_t nx_waitpid(pid_t pid, int *stat_loc, int options)
sigemptyset(&set);
nxsig_addset(&set, SIGCHLD);

/* NOTE: sched_lock() is not enough for SMP
* because the child task is running on another CPU
*/

#ifdef CONFIG_SMP
irqstate_t flags = enter_critical_section();
#endif

/* Disable pre-emption so that nothing changes while the loop executes */

sched_lock();
Expand Down Expand Up @@ -438,11 +458,17 @@ pid_t nx_waitpid(pid_t pid, int *stat_loc, int options)
}
}

sched_unlock();
return pid;
/* On success, return the PID */

ret = pid;

errout:
sched_unlock();

#ifdef CONFIG_SMP
leave_critical_section(flags);
#endif

return ret;
}
#endif /* CONFIG_SCHED_HAVE_PARENT */
Expand Down
10 changes: 10 additions & 0 deletions sched/task/task_exithook.c
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,12 @@ void nxtask_exithook(FAR struct tcb_s *tcb, int status, bool nonblocking)

nxtask_recover(tcb);

/* NOTE: signal handling needs to be done in a criticl section */

#ifdef CONFIG_SMP
irqstate_t flags = enter_critical_section();
#endif

/* Send the SIGCHLD signal to the parent task group */

nxtask_signalparent(tcb, status);
Expand Down Expand Up @@ -668,6 +674,10 @@ void nxtask_exithook(FAR struct tcb_s *tcb, int status, bool nonblocking)

nxsig_cleanup(tcb); /* Deallocate Signal lists */

#ifdef CONFIG_SMP
leave_critical_section(flags);
#endif

/* This function can be re-entered in certain cases. Set a flag
* bit in the TCB to not that we have already completed this exit
* processing.
Expand Down

0 comments on commit 4cc38ca

Please sign in to comment.