Re: [PATCH v2] net_sched: sch_sfq: better struct layouts

Jarek Poplawski
2010-12-20T22:55:46Z

On Mon, Dec 20, 2010 at 06:02:05PM +0100, Eric Dumazet wrote:
> Le dimanche 19 décembre 2010 ?? 22:22 +0100, Jarek Poplawski a écrit :
>
> > I think open coding sk_buff_head is a wrong idea. Otherwise, this
> > patch looks OK to me, only a few cosmetic suggestions below.
> >
>
> I completely agree with you but this should be temporary, because David
> really wants to use list_head for skbs, I believe this will be done ;)
>
> I chose to name the list skblist to make clear where we want to plug a
> real list_head once done.
>
> Also, not using sk_buff_head saves at least 8 bytes per slot.

Alas I dumped my 486sx already :-/

> > > - sfq_index max_depth; /* Maximal depth */
> > > + sfq_index max_depth; /* depth of longest slot */
> >
> > depth and/or length? (One dimension should be enough.)
>
> maybe cur_depth ? Its not the maximal possible depth, but depth of
> longest slot, or current max depth...

Hmm... or max_depth? I meant the comment only, sorry ;-)

> > > - /* If selected queue has length q->limit, this means that
> > > - * all another queues are empty and that we do simple tail drop,
> >
> > No reason to remove this line.
>
> Well, the reason we drop this packet is not because other queues are
> empty, but because we reach max depth for this queue. (I have the idea
> to extend SFQ to allow more packets to be queued, still with a 127 limit
> per queue, and 127 flows). With 10Gbs links, a global limit of 127
> packets is short.

Right, but does this line say something else? Of course, you can find
it out by yourself too, but this comment makes reading a bit faster.

> > If you really have to do this, all these: __skb_queue_tail(),
> > __skb_dequeue(), skb_queue_head_init(), skb_peek() etc. used here
> > should stay as (local) inline functions to remain readability.
> >
>
> OK done, thanks a lot for reviewing and very useful comments !

Thanks for using them!
Jarek P.