Assumably a BUG in Linux Kernel (scheduler part)

Assumably a BUG in Linux Kernel (scheduler part)

[Andrey Gelman]

Hello there !
Assumably, I've discovered a bug in Linux kernel (version 2.6.16), at:
kernel\sched.c   function set_user_nice()

Problem description:
After you execute nice() system call, the dynamic priority is set to the new 
value of the static priority, instead of being adjusted by a difference 
between new and old nice values.
In other words:
What you have before you execute nice():  p->prio == static_prio - bonus
After you execute nice():  p->prio == "a new" static_prio  (no bonus)

BUG Fix:
Here I paste the whole of function set_user_nice() (thanks god, it's short) 
with the BUG highlighted and fixed:

void set_user_nice(task_t *p, long nice)
{
        unsigned long flags;
        prio_array_t *array;
        runqueue_t *rq;
        int old_prio, new_prio, delta;

        if (TASK_NICE(p) == nice || nice < -20 || nice > 19)
                return;
        /*
         * We have to be careful, if called from sys_setpriority(),
         * the task might be in the middle of scheduling on another CPU.
         */
        rq = task_rq_lock(p, &flags);
        /*
         * The RT priorities are set via sched_setscheduler(), but we still
         * allow the 'normal' nice value to be set - but as expected
         * it wont have any effect on scheduling until the task is
         * not SCHED_NORMAL/SCHED_BATCH:
         */
        if (rt_task(p)) {
                p->static_prio = NICE_TO_PRIO(nice);
                goto out_unlock;
        }
        array = p->array;
        if (array)
                dequeue_task(p, array);
//-------------------------------------------------
/*
	//BUGGED FORMULA : 5 lines
        old_prio = p->prio;
        new_prio = NICE_TO_PRIO(nice);
        delta = new_prio - old_prio;
        p->static_prio = NICE_TO_PRIO(nice);
        p->prio += delta;
*/
    //BUG FIX : 5 lines
    old_prio = p->static_prio;
    new_prio = NICE_TO_PRIO(nice);
    delta = new_prio - old_prio;
    p->static_prio = new_prio;
    p->prio += delta;
//-------------------------------------------------
        if (array) {
                enqueue_task(p, array);
                /*
                 * If the task increased its priority or is running and
                 * lowered its priority, then reschedule its CPU:
                 */
                if (delta < 0 || (delta > 0 && task_running(rq, p)))
                        resched_task(rq->curr);
        }
out_unlock:
        task_rq_unlock(rq, &flags);
}



Thank you,
Andrey Gelman,
Haifa, ISRAEL
9-Jun-2006

Re: Assumably a BUG in Linux Kernel (scheduler part)

[Ingo Molnar]

* Andrey Gelman <agelman@012.net.il> wrote:

> Hello there !
> Assumably, I've discovered a bug in Linux kernel (version 2.6.16), at:
> kernel\sched.c   function set_user_nice()
[...]

> //-------------------------------------------------
> /*
> 	//BUGGED FORMULA : 5 lines
>         old_prio = p->prio;
>         new_prio = NICE_TO_PRIO(nice);
>         delta = new_prio - old_prio;
>         p->static_prio = NICE_TO_PRIO(nice);
>         p->prio += delta;
> */
>     //BUG FIX : 5 lines
>     old_prio = p->static_prio;
>     new_prio = NICE_TO_PRIO(nice);
>     delta = new_prio - old_prio;
>     p->static_prio = new_prio;
>     p->prio += delta;
> //-------------------------------------------------

you are right, this is a bug in the scheduler - good find.

I did accidentally fix it months ago via one of the PI-scheduling 
cleanups, and thus the fix is in the current -mm tree and is scheduled 
for 2.6.18 inclusion:

 http://kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.17-rc6/2.6.17-rc6-mm2/broken-out/pi-futex-scheduler-support-for-pi.patch

(see the adding of effective_prio() to set_user_nice(), instead of an 
open-coded calculation of the priority)

But i did not realize that this also fixed a bug. The effects of the bug 
are minor: a user can renice only 19 times (to go from 0 to +19), so 
there's a finite amount of extra timeslices a CPU hog might win due to 
this. I'd not include the effective_prio() change in 2.6.17 - it touches 
quite some code and we are close to the release of 2.6.17. Nevertheless 
kudos for finding and pointing out this bug!

Andrew, please add this to the changelog of
pi-futex-scheduler-support-for-pi.patch:

the effective_prio() cleanups also fix a priority-calculation bug 
pointed out by Andrey Gelman, in set_user_nice().

	Ingo