netdev - Re: [PATCH net-next v9 1/7] sched: Add Common Applications Kept Enhanced (cake) qdisc

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: <20180514.110558.392362320880673775.davem@davemloft.net>
Date:   Mon, 14 May 2018 11:05:58 -0400 (EDT)
From:   David Miller <davem@...emloft.net>
To:     toke@...e.dk
Cc:     netdev@...r.kernel.org, cake@...ts.bufferbloat.net
Subject: Re: [PATCH net-next v9 1/7] sched: Add Common Applications Kept
 Enhanced (cake) qdisc

From: Toke Høiland-Jørgensen <toke@...e.dk>
Date: Mon, 14 May 2018 11:08:28 +0200

> David Miller <davem@...emloft.net> writes:
> 
>> From: Toke Høiland-Jørgensen <toke@...e.dk>
>> Date: Tue, 08 May 2018 16:34:19 +0200
>>
>>> +struct cake_flow {
>>> +	/* this stuff is all needed per-flow at dequeue time */
>>> +	struct sk_buff	  *head;
>>> +	struct sk_buff	  *tail;
>>
>> Please do not invent your own SKB list handling mechanism.
> 
> We didn't invent it, we inherited it from fq_codel. I was actually about
> to fix that, but then I noticed it was still around in fq_codel, and so
> let it be. I can certainly fix it anyway, but, erm, why is it acceptable
> in fq_codel but not in cake? struct sk_buff_head is not that new, is it?

I guess one argument has to do with the amount of memory consumed by this
per-flow or per-queue information, right?  Because the skb queue head has
a qlen and a spinlock regardless of whether they are used or not.

Furthermore, if you use the __skb_insert() et al. helpers, even though it
won't use the lock it will adjust the qlen counter.  And that's useless
work since you have no use for the qlen value.

Taken together, it seems that what you and fq_codel are doing is not
such a bad idea after all.  So please leave it alone.

On-and-off again, I've looked into converting skbs to using list_head
but it's a non-trivial set of work.  All over the tree the different
layers use the next/prev pointers in different ways.  Some use it for
a doubly linked list.  Some use it for a singly linked list.  Some
encode state in the prev pointer.  You name it, it's out there.

I'll try to get back to that task because obviously it'll be useful to
have code like cake and fq_codel use common helpers instead of custom
stuff.

Thanks.