From: Paolo Valente <paolo.valente@xxxxxxxxxx>Sorry for trying this silly short cut
Date: Tue, 20 Nov 2012 18:45:13 +0100
- struct sk_buff *skb;
+ struct sk_buff *skb = NULL;
This is not really an improvement,
that NULL is passed eventually into qdisc_bstats_update().Got it. Actually, if the first qfq_peek_skb returns NULL, then the example version that you are proposing apparently may behave in a different way than the original one: in your proposal the scheduler tries to switch to a new aggregate and may return a non-NULL value, whereas the original version would immediately return NULL. I guess that this slightly different behavior is fine as well, and I am preparing a new patch that integrates these changes.
Please make the logic easier for the compiler to digest.
For example, restructure the top-level logic into something like:
skb = NULL;
if (!list_empty(&in_serv_agg->active))
skb = qfq_peek_skb(in_serv_agg, &cl, &len);
else
len = 0; /* no more active classes in the in-service agg */
if (len == 0 || in_serv_agg->budget < len) {
...
/*
* If we get here, there are other aggregates queued:
* choose the new aggregate to serve.
*/
in_serv_agg = q->in_serv_agg = qfq_choose_next_agg(q);
skb = qfq_peek_skb(in_serv_agg, &cl, &len);
}
if (!skb)
return NULL;
That way it is clearer, to both humans and the compiler, what is
going on here.
Thanks.