Ingo Molnar <[EMAIL PROTECTED]> writes:
> * OGAWA Hirofumi <[EMAIL PROTECTED]> wrote:
>
>> "Fabio Comolli" <[EMAIL PROTECTED]> writes:
>>
>> > Just found this in syslog. It was during normal activity, about 6
>> > minutes after resume-from-ram. I never saw this before.
>>
>> It seems someone missed to check PREEMPT_ACTIVE in __resched_legal().
>
> but PREEMPT_ACTIVE is 0x10000000, not 0x20000000.
>
>> Could you please test the following patch?
>
> no. cond_resched() is always legal in the !PREEMPT case.
>
> i found another bug and realized that the whole __resched_legal()
> approach is fundamentally wrong! The patch below fixes this.
Hmm.. but the path seems,
-> cond_resched()
-> if (__resched_legal()) /* preempt_count == 0 */
-> __cond_resched() /* preempt_count == 0x10000000 */
-> schedule()
[...]
-> cond_resched()
-> if (__resched_legal()) /* preempt_count == 0x10000000 */
-> __cond_resched() /* preempt_count == 0x20000000 */
-> schedule() /* warning */
Where is it prevented? Or warning is just wrong?
> ------------------->
> Subject: [patch] sched: remove __resched_legal() and fix
> cond_resched_softirq()
> From: Ingo Molnar <[EMAIL PROTECTED]>
>
> remove the __resched_legal() check: it is conceptually broken. The
> biggest problem it had is that it can mask buggy cond_resched() calls. A
> cond_resched() call is only legal if we are not in an atomic context.
> But __resched_legal() hid this fact. Same goes for cond_resched_locked()
> and cond_resched_softirq().
>
> furthermore, the __legal_resched(0) call was buggy in
> cond_resched_softirq() and caused unnecessary long softirq latencies!
>
> the fix is to preserve the only valid inhibitor to voluntary preemption:
> if the system is still booting. None of the other behavior of
> __resched_legal() made any sense.
>
> the effect of this fix should be more real bugs exposed, and shorter
> softirq latencies.
>
> Signed-off-by: Ingo Molnar <[EMAIL PROTECTED]>
> ---
> kernel/sched.c | 17 +++--------------
> 1 file changed, 3 insertions(+), 14 deletions(-)
>
> Index: linux/kernel/sched.c
> ===================================================================
> --- linux.orig/kernel/sched.c
> +++ linux/kernel/sched.c
> @@ -4617,17 +4617,6 @@ asmlinkage long sys_sched_yield(void)
> return 0;
> }
>
> -static inline int __resched_legal(int expected_preempt_count)
> -{
> -#ifdef CONFIG_PREEMPT
> - if (unlikely(preempt_count() != expected_preempt_count))
> - return 0;
> -#endif
> - if (unlikely(system_state != SYSTEM_RUNNING))
> - return 0;
> - return 1;
> -}
> -
> static void __cond_resched(void)
> {
> #ifdef CONFIG_DEBUG_SPINLOCK_SLEEP
> @@ -4647,7 +4636,7 @@ static void __cond_resched(void)
>
> int __sched cond_resched(void)
> {
> - if (need_resched() && __resched_legal(0)) {
> + if (need_resched() && system_state == SYSTEM_RUNNING) {
> __cond_resched();
> return 1;
> }
> @@ -4673,7 +4662,7 @@ int cond_resched_lock(spinlock_t *lock)
> ret = 1;
> spin_lock(lock);
> }
> - if (need_resched() && __resched_legal(1)) {
> + if (need_resched() && system_state == SYSTEM_RUNNING) {
> spin_release(&lock->dep_map, 1, _THIS_IP_);
> _raw_spin_unlock(lock);
> preempt_enable_no_resched();
> @@ -4689,7 +4678,7 @@ int __sched cond_resched_softirq(void)
> {
> BUG_ON(!in_softirq());
>
> - if (need_resched() && __resched_legal(0)) {
> + if (need_resched() && system_state == SYSTEM_RUNNING) {
> raw_local_irq_disable();
> _local_bh_enable();
> raw_local_irq_enable();
--
OGAWA Hirofumi <[EMAIL PROTECTED]>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
