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

[Andrew Morton <akpm@osdl.org>]

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 ;)