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

[Jarek Poplawski <jarkao2@gmail.com>]

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.