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

[David Miller <davem@davemloft.net>]

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

> David Miller <davem@davemloft.net> writes:
> 
>> From: Toke Høiland-Jørgensen <toke@toke.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.