Re: [PATCH] sched_yield() flawed since 2.2.13?

From: George Anzinger (george@pioneer.net)
Date: Sat May 20 2000 - 14:08:53 EST


Dear Giuseppe,

The indicated patch as well as some other fixes is include in the real
time scheduler patch at www.mvista.com

This patch not only gives an option to use the new rtsched.c but patches
the 2.2.14 sched.c to fix most of the outstanding problems. Below is the
part of the patch related to sched.c:)

diff -urN linux-2.2.14/kernel/sched.c linux/kernel/sched.c
--- linux-2.2.14/kernel/sched.c Tue Apr 11 04:30:52 2000
+++ linux/kernel/sched.c Thu Apr 27 05:56:33 2000
@@ -115,12 +115,12 @@
 #ifdef __SMP__
 
 #define idle_task(cpu) (task[cpu_number_map[(cpu)]])
-#define can_schedule(p) (!(p)->has_cpu)
+#define can_schedule(p) (!(p)->has_cpu || (p) == prev)
 
 #else
 
 #define idle_task(cpu) (&init_task)
-#define can_schedule(p) (1)
+#define can_schedule(p) (1)
 
 #endif
 
@@ -322,7 +322,7 @@
         int this_cpu = smp_processor_id();
         struct task_struct *tsk;
 
- tsk = current;
+ tsk = cpu_curr(this_cpu);
         if (preemption_goodness(tsk, p, this_cpu) > 0)
                 tsk->need_resched = 1;
 #endif
@@ -687,8 +687,9 @@
 {
         struct schedule_data * sched_data;
         struct task_struct *prev, *next, *p;
- int this_cpu, c;
+ int this_cpu, c, oldcounter, beenhere;
 
+try_try_again:
         if (tq_scheduler)
                 goto handle_tq_scheduler;
 tq_scheduler_back:
@@ -729,7 +730,7 @@
                         del_from_runqueue(prev);
                 case TASK_RUNNING:
         }
- prev->need_resched = 0;
+ beenhere = prev->need_resched = 0;
 
 repeat_schedule:
 
@@ -741,25 +742,11 @@
         /* Default process to select.. */
         next = idle_task(this_cpu);
         c = -1000;
+ oldcounter = 0;
         if (prev->state == TASK_RUNNING)
                 goto still_running;
 still_running_back:
 
- /*
- * This is subtle.
- * Note how we can enable interrupts here, even
- * though interrupts can add processes to the run-
- * queue. This is because any new processes will
- * be added to the front of the queue, so "p" above
- * is a safe starting point.
- * run-queue deletion and re-ordering is protected by
- * the scheduler lock
- */
-/*
- * Note! there may appear new tasks on the run-queue during this, as
- * interrupts are enabled. However, they will be put on front of the
- * list, so our list starting at "p" is essentially fixed.
- */
         while (p != &init_task) {
                 if (can_schedule(p)) {
                         int weight = goodness(prev, p, this_cpu);
@@ -768,9 +755,10 @@
                 }
                 p = p->next_run;
         }
+ prev->counter += oldcounter;
 
         /* Do we need to re-calculate counters? */
- if (!c)
+ if (!c && !beenhere)
                 goto recalculate;
         /*
          * from this point on nothing can prevent us from
@@ -825,7 +813,10 @@
 
 same_process:
   
+ prev->policy &= ~SCHED_YIELD;
         reacquire_kernel_lock(current);
+ if( current->need_resched)
+ goto try_try_again;
         return;
 
 recalculate:
@@ -837,13 +828,17 @@
                         p->counter = (p->counter >> 1) + p->priority;
                 read_unlock(&tasklist_lock);
                 spin_lock_irq(&runqueue_lock);
+ beenhere++;
                 goto repeat_schedule;
         }
 
 still_running:
- c = prev_goodness(prev, prev, this_cpu);
- next = prev;
- goto still_running_back;
+ if (!(prev->policy & SCHED_YIELD))
+ goto still_running_back;
+
+ oldcounter = prev->counter;
+ prev->counter=0;
+ goto still_running_back;
 
 handle_bh:
         do_bottom_half();
----------------- cut--------- end of patch-------------------
Giuseppe Ciaccio wrote:
>
> On Fri, 19 May 2000, George Anzinger wrote:
>
> > There have been numerous proposals around this (and several related)
> > problem(s). I suggest you look up the discussion in the digest.
> >
> > The principle problem with your patch is that it introduces more code in
> > the inner schedule loop. A different approach might be to:
> > a.) Set the counter to zero for a sched yield process,
> > b.) Do the sched loop
> > c.) Reset the counter to its prior value
>
> Step a) could be easily performed in sys_sched_yield(); this routine
> also set current->need_resched, so as to trigger a call to schedule()
> (step b) upon return. However I don't see how to reset the counter to its
> original value (step c) without adding more code to schedule(), given that
> sys_sched_yield() has already returned when schedule() is called.
>
> My apologies for not having searched the (huge) linux-kernel archive before
> posting my patch. However, IMO it is bad thing that the sched_yield() flaw
> have remained in the kernel despite of the "numerous proposals around this".
> I understand the need for keeping the schedule() code as much straightforward
> as possible, but IMO this cannot justify a wrong semantics for sched_yield().
>
> Giuseppe
>
> Giuseppe Ciaccio http://www.disi.unige.it/person/CiaccioG/
> DISI - Universita' di Genova via Dodecaneso 35 16146 Genova, Italy
> phone +39 10 353 6638 fax +39 010 3536699 ciaccio@disi.unige.it
> ------------------------------------------------------------------------

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Tue May 23 2000 - 21:00:19 EST