From 1ee5cc9c2b9e3547e785d415cd521363b3b11a8f Mon Sep 17 00:00:00 2001 From: Bahadir Balban Date: Wed, 22 Oct 2008 13:00:28 +0300 Subject: [PATCH] Fixed a task suspend/resume scheduler issue. - Scheduler was increasing total priorities only when resuming tasks had 0 ticks. This caused forked tasks that have parent's share of ticks to finish their jobs, if these tasks exited quick enough, they would cause the total priorities to deduce without increasing it in the first place. This is now fixed. - Also strengthened rq locking, now both queues are locked before touching any. - Also removed task suspends in irq, this would cause a race condition on ticks and runqueues, since neither is protected against irqs. --- include/l4/arch/arm/exception.h | 5 ++ include/l4/generic/tcb.h | 2 - src/generic/irq.c | 10 ++- src/generic/scheduler.c | 115 +++++++++++++++++--------------- src/generic/tcb.c | 35 ---------- 5 files changed, 77 insertions(+), 90 deletions(-) diff --git a/include/l4/arch/arm/exception.h b/include/l4/arch/arm/exception.h index 55dfed1..28fd94e 100644 --- a/include/l4/arch/arm/exception.h +++ b/include/l4/arch/arm/exception.h @@ -75,6 +75,11 @@ static inline void irq_local_disable() /* This is filled on entry to irq handler, only if a process was interrupted.*/ extern unsigned int preempted_psr; +/* + * FIXME: TASK_IN_KERNEL works for non-current tasks, in_kernel() works for current task? + * in_kernel() is for irq, since normally in process context you know if you are in kernel or not :-) + */ + /* Implementing these as functions cause circular include dependency for tcb.h */ #define TASK_IN_KERNEL(tcb) (((tcb)->context.spsr & ARM_MODE_MASK) == ARM_MODE_SVC) #define TASK_IN_USER(tcb) (!TASK_IN_KERNEL(tcb)) diff --git a/include/l4/generic/tcb.h b/include/l4/generic/tcb.h index b33a25d..87d4d80 100644 --- a/include/l4/generic/tcb.h +++ b/include/l4/generic/tcb.h @@ -148,7 +148,5 @@ extern struct id_pool *thread_id_pool; extern struct id_pool *space_id_pool; extern struct id_pool *tgroup_id_pool; -void task_process_pending_flags(void); - #endif /* __TCB_H__ */ diff --git a/src/generic/irq.c b/src/generic/irq.c index ab56200..ac0c20e 100644 --- a/src/generic/irq.c +++ b/src/generic/irq.c @@ -75,8 +75,16 @@ void do_irq(void) } irq_enable(irq_index); +#if 0 /* Process any pending flags for currently runnable task */ - task_process_pending_flags(); + if (!in_nested_irq_context()) { + /* This is buggy, races on prio_total */ + if (in_user()) { + if (current->flags & TASK_SUSPENDING) + sched_suspend_async(); + } + } +#endif } diff --git a/src/generic/scheduler.c b/src/generic/scheduler.c index ebc0a15..b512f32 100644 --- a/src/generic/scheduler.c +++ b/src/generic/scheduler.c @@ -41,6 +41,18 @@ extern unsigned int current_irq_nest_count; /* This ensures no scheduling occurs after voluntary preempt_disable() */ static int voluntary_preempt = 0; +void sched_lock_runqueues(void) +{ + spin_lock(&sched_rq[0].lock); + spin_lock(&sched_rq[1].lock); +} + +void sched_unlock_runqueues(void) +{ + spin_unlock(&sched_rq[0].lock); + spin_unlock(&sched_rq[1].lock); +} + int preemptive() { return current_irq_nest_count == 0; @@ -112,14 +124,6 @@ static void sched_rq_swap_runqueues(void) rq_expired = temp; } -/* FIXME: - * Sleepers should not affect runqueue priority. - * Suspended tasks should affect runqueue priority. - * - * Also make sure that if sleepers get suspended, - * they do affect runqueue priority. - */ - /* Set policy on where to add tasks in the runqueue */ #define RQ_ADD_BEHIND 0 #define RQ_ADD_FRONT 1 @@ -129,30 +133,35 @@ static void sched_rq_add_task(struct ktcb *task, struct runqueue *rq, int front) { BUG_ON(!list_empty(&task->rq_list)); - spin_lock(&rq->lock); + sched_lock_runqueues(); if (front) list_add(&task->rq_list, &rq->task_list); else list_add_tail(&task->rq_list, &rq->task_list); rq->total++; task->rq = rq; - spin_unlock(&rq->lock); + sched_unlock_runqueues(); } -/* NOTE: Do we need an rq_lock on tcb? */ - /* Helper for removing a task from its runqueue. */ static inline void sched_rq_remove_task(struct ktcb *task) { - struct runqueue *rq = task->rq; + struct runqueue *rq; - spin_lock(&rq->lock); + sched_lock_runqueues(); + + /* + * We must lock both, otherwise rqs may swap and + * we may get the wrong rq. + */ + rq = task->rq; + BUG_ON(list_empty(&task->rq_list)); list_del_init(&task->rq_list); task->rq = 0; rq->total--; BUG_ON(rq->total < 0); - spin_unlock(&rq->lock); + sched_unlock_runqueues(); } @@ -201,33 +210,6 @@ void sched_resume_async(struct ktcb *task) sched_rq_add_task(task, rq_runnable, RQ_ADD_FRONT); } -extern void arch_switch(struct ktcb *cur, struct ktcb *next); - -static inline void context_switch(struct ktcb *next) -{ - struct ktcb *cur = current; - - // printk("(%d) to (%d)\n", cur->tid, next->tid); - - /* Flush caches and everything */ - arch_hardware_flush(next->pgd); - - /* Switch context */ - arch_switch(cur, next); - - // printk("Returning from yield. Tid: (%d)\n", cur->tid); -} - -/* - * Priority calculation is so simple it is inlined. The task gets - * the ratio of its priority to total priority of all runnable tasks. - */ -static inline int sched_recalc_ticks(struct ktcb *task, int prio_total) -{ - return task->ticks_assigned = - SCHED_TICKS * task->priority / prio_total; -} - /* * NOTE: Could do these as sched_prepare_suspend() * + schedule() or need_resched = 1 @@ -265,6 +247,35 @@ void sched_suspend_async(void) } +extern void arch_switch(struct ktcb *cur, struct ktcb *next); + +static inline void context_switch(struct ktcb *next) +{ + struct ktcb *cur = current; + + // printk("(%d) to (%d)\n", cur->tid, next->tid); + + /* Flush caches and everything */ + arch_hardware_flush(next->pgd); + + /* Switch context */ + arch_switch(cur, next); + + // printk("Returning from yield. Tid: (%d)\n", cur->tid); +} + +/* + * Priority calculation is so simple it is inlined. The task gets + * the ratio of its priority to total priority of all runnable tasks. + */ +static inline int sched_recalc_ticks(struct ktcb *task, int prio_total) +{ + BUG_ON(prio_total < task->priority); + BUG_ON(prio_total == 0); + return task->ticks_assigned = + SCHED_TICKS * task->priority / prio_total; +} + /* * Tasks come here, either by setting need_resched (via next irq), @@ -299,8 +310,9 @@ void schedule() { struct ktcb *next; - /* Should not schedule with preemption disabled */ + /* Should not schedule with preemption disabled or in nested irq */ BUG_ON(voluntary_preempt); + BUG_ON(in_nested_irq_context()); /* Should not have more ticks than SCHED_TICKS */ BUG_ON(current->ticks_left > SCHED_TICKS); @@ -343,19 +355,18 @@ void schedule() } } + /* New tasks affect runqueue total priority. */ + if (next->flags & TASK_RESUMING) { + prio_total += next->priority; + next->flags &= ~TASK_RESUMING; + } + /* Zero ticks indicates task hasn't ran since last rq swap */ if (next->ticks_left == 0) { - - /* New tasks affect runqueue total priority. */ - if (next->flags & TASK_RESUMING) { - prio_total += next->priority; - next->flags &= ~TASK_RESUMING; - } - /* * Redistribute timeslice. We do this as each task - * becomes runnable rather than all at once. It's also - * done only upon a runqueue swap. + * becomes runnable rather than all at once. It is done + * every runqueue swap */ sched_recalc_ticks(next, prio_total); next->ticks_left = next->ticks_assigned; diff --git a/src/generic/tcb.c b/src/generic/tcb.c index d94fa96..bd40b75 100644 --- a/src/generic/tcb.c +++ b/src/generic/tcb.c @@ -22,38 +22,3 @@ struct list_head global_task_list; unsigned int need_resched_offset = offsetof(struct ktcb, ts_need_resched); unsigned int syscall_regs_offset = offsetof(struct ktcb, syscall_regs); - -/* - * When there is an asynchronous pending event to be handled by - * the task (e.g. task is suspending), normally it is processed - * when the task is returning to user mode from the kernel. If - * the event is raised when the task is in userspace, this call - * in irq context makes sure it is handled. - */ -void task_process_pending_flags(void) -{ - if (TASK_IN_USER(current)) { - if (current->flags & TASK_SUSPENDING) { - if (in_irq_context()) - sched_suspend_async(); - else - sched_suspend_sync(); - } - } -} - -#if 0 -int task_suspend(struct ktcb *task) -{ - task->flags |= SCHED_FLAG_SUSPEND; - - return 0; -} - -int task_resume(struct ktcb *task) -{ - task->flags &= ~SCHED_FLAG_SUSPEND; - return sched_enqueue_task(task); -} -#endif -