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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20101220225540.GA2052@del.dom.local>
Date:	Mon, 20 Dec 2010 23:55:40 +0100
From:	Jarek Poplawski <jarkao2@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Patrick McHardy <kaber@...sh.net>,
	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v2] net_sched: sch_sfq: better struct layouts

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html