Re: [2.6.16-rc6 patch] fix interactive task starvation

Andrew Morton
2006-03-18T06:25:00Z

Mike Galbraith <efault@gmx.de> wrote:
>
> > Does this have to be a macro?
> >
>
> I suppose not, now inlined.
>

It would be nice to uninline the function and then to modify it in a
followup patch. That way, we get to see what changed, which is one of the
reasons to not use megamacros (sorry).

> +static inline int expired_starving(runqueue_t *rq)
> +{
> + int limit = STARVATION_LIMIT * rq->nr_running, starving;
> +
> + if (!limit || !rq->expired_timestamp)
> + return 0;
> + starving = jiffies - rq->expired_timestamp >= limit;
> + starving += rq->curr->static_prio > rq->best_expired_prio;
> +
> + return starving;
> +}

ick. Is that really what that macros does??

The function returns a boolean, so we should short-circuit the evaluation
where possible.

static inline int expired_starving(runqueue_t *rq)
{
int limit;

/* Comment goes here */
if (!rq->expired_timestamp)
return 0;

limit = STARVATION_LIMIT * rq->nr_running;

/* Here too */
if (!limit)
return 0;

/* And here */
if (jiffies - rq->expired_timestamp >= limit)
return 1;

/* And here */
if (rq->curr->static_prio > rq->best_expired_prio)
return 1;

/* And here */
return 0;
}

This way

a) We get somewhere to put comments describing each step of the logic.

b) We get to select the order of the comparisons in decreasing
(probability*expensiveness) order.

See how you're performing an unneeded multiplication if
!rq->expired_timestamp?

c) See how the first test of `limit' comes after that multiplication?
STARVATION_LIMIT is a constant (isn't it?) If so, we need only test
rq->nr_running.

d) The next guy who comes along has to update the comments ;)