Messages in this thread | | | Date | Fri, 17 Mar 2006 22:22:03 -0800 | From | Andrew Morton <> | Subject | Re: [2.6.16-rc6 patch] fix interactive task starvation |
| |
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 ;)
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |