From dca77fa06aa53311ab17b2e65bf2567a04d032c3 Mon Sep 17 00:00:00 2001 From: Gregory Nutt Date: Fri, 24 Feb 2017 10:07:23 -0600 Subject: [PATCH] sigtimedwait: When timer expires, up_unblock_task() is called. This is okay in the single CPU case because interrupts are disable in the timer interrupt handler. But it is insufficient in the SMP case. enter_ and leave_critical_section() must be called in order to manage spinlocks correctly. --- sched/mqueue/mq_sndinternal.c | 2 +- sched/sched/sched_processtimer.c | 11 ++++++++++- sched/sched/sched_timerexpiration.c | 11 ++++++++++- sched/sched/sched_unlock.c | 1 - sched/signal/sig_timedwait.c | 29 +++++++++++++++++++++++++++-- sched/task/task_exit.c | 2 +- 6 files changed, 49 insertions(+), 7 deletions(-) diff --git a/sched/mqueue/mq_sndinternal.c b/sched/mqueue/mq_sndinternal.c index 946c69275ce..12fb9948415 100644 --- a/sched/mqueue/mq_sndinternal.c +++ b/sched/mqueue/mq_sndinternal.c @@ -224,7 +224,7 @@ FAR struct mqueue_msg_s *mq_msgalloc(void) * * Assumptions/restrictions: * - The caller has verified the input parameters using mq_verifysend(). - * - Interrupts are disabled. + * - Executes within a critical section established by the caller. * ****************************************************************************/ diff --git a/sched/sched/sched_processtimer.c b/sched/sched/sched_processtimer.c index d8bcc3e1110..d928d40649c 100644 --- a/sched/sched/sched_processtimer.c +++ b/sched/sched/sched_processtimer.c @@ -124,9 +124,18 @@ static inline void sched_process_scheduler(void) irqstate_t flags; int i; - /* Perform scheduler operations on all CPUs */ + /* If we are running on a single CPU architecture, then we know interrupts + * a disabled an there is no need to explicitly call + * enter_critical_section(). However, in the SMP case, + * enter_critical_section() does much more than just disable interrupts on + * the local CPU; it also manages spinlocks to assure the stability of the + * TCB that we are manipulating. + */ flags = enter_critical_section(); + + /* Perform scheduler operations on all CPUs */ + for (i = 0; i < CONFIG_SMP_NCPUS; i++) { sched_cpu_scheduler(i); diff --git a/sched/sched/sched_timerexpiration.c b/sched/sched/sched_timerexpiration.c index 47095306cf0..174ff1f7a70 100644 --- a/sched/sched/sched_timerexpiration.c +++ b/sched/sched/sched_timerexpiration.c @@ -290,9 +290,18 @@ static uint32_t sched_process_scheduler(uint32_t ticks, bool noswitches) irqstate_t flags; int i; - /* Perform scheduler operations on all CPUs */ + /* If we are running on a single CPU architecture, then we know interrupts + * a disabled an there is no need to explicitly call + * enter_critical_section(). However, in the SMP case, + * enter_critical_section() does much more than just disable interrupts on + * the local CPU; it also manages spinlocks to assure the stability of the + * TCB that we are manipulating. + */ flags = enter_critical_section(); + + /* Perform scheduler operations on all CPUs */ + for (i = 0; i < CONFIG_SMP_NCPUS; i++) { timeslice = sched_cpu_scheduler(i, ticks, noswitches); diff --git a/sched/sched/sched_unlock.c b/sched/sched/sched_unlock.c index d22a4b48bd6..091ad7b1cb0 100644 --- a/sched/sched/sched_unlock.c +++ b/sched/sched/sched_unlock.c @@ -145,7 +145,6 @@ int sched_unlock(void) * we should go ahead and release the pending tasks. See the logic * leave_critical_section(): It will call up_release_pending() * BEFORE it clears IRQ lock. - * BEFORE it clears IRQ lock. */ if (!spin_islocked(&g_cpu_schedlock) && !irq_cpu_locked(cpu) && diff --git a/sched/signal/sig_timedwait.c b/sched/signal/sig_timedwait.c index bdb5376d8bd..dcbc892fd97 100644 --- a/sched/signal/sig_timedwait.c +++ b/sched/signal/sig_timedwait.c @@ -1,7 +1,7 @@ /**************************************************************************** * sched/signal/sig_timedwait.c * - * Copyright (C) 2007-2009, 2012-2016 Gregory Nutt. All rights reserved. + * Copyright (C) 2007-2009, 2012-2017 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -76,12 +76,20 @@ * Name: sig_timeout * * Description: - * A timeout elapsed while waiting for signals to be queued. + * A timeout elapsed while waiting for signals to be queued. + * + * Assumptions: + * This function executes in the context of the timer interrupt handler. + * Local interrupts are assumed to be disabled on entry. * ****************************************************************************/ static void sig_timeout(int argc, wdparm_t itcb) { +#ifdef CONFIG_SMP + irqstate_t flags; +#endif + /* On many small machines, pointers are encoded and cannot be simply cast * from uint32_t to struct tcb_s *. The following union works around this * (see wdogparm_t). This odd logic could be conditioned on @@ -97,6 +105,19 @@ static void sig_timeout(int argc, wdparm_t itcb) u.itcb = itcb; ASSERT(u.wtcb); +#ifdef CONFIG_SMP + /* We must be in a critical section in order to call up_unblock_task() + * below. If we are running on a single CPU architecture, then we know + * interrupts a disabled an there is no need to explicitly call + * enter_critical_section(). However, in the SMP case, + * enter_critical_section() does much more than just disable interrupts on + * the local CPU; it also manages spinlocks to assure the stability of the + * TCB that we are manipulating. + */ + + flags = enter_critical_section(); +#endif + /* There may be a race condition -- make sure the task is * still waiting for a signal */ @@ -113,6 +134,10 @@ static void sig_timeout(int argc, wdparm_t itcb) #endif up_unblock_task(u.wtcb); } + +#ifdef CONFIG_SMP + leave_critical_section(flags); +#endif } /**************************************************************************** diff --git a/sched/task/task_exit.c b/sched/task/task_exit.c index 26d30ba3dbc..e8f3b9cd595 100644 --- a/sched/task/task_exit.c +++ b/sched/task/task_exit.c @@ -76,7 +76,7 @@ * OK on success; or ERROR on failure * * Assumeptions: - * Interrupts are disabled. + * Executing within a critical section established by the caller. * ****************************************************************************/