netdev - Re: [PATCH 8/9]: sch_hfsc: Use ->requeue queue instead of ops.

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: <20080820220931.GA3071@ami.dom.local>
Date:	Thu, 21 Aug 2008 00:09:32 +0200
From:	Jarek Poplawski <jarkao2@...il.com>
To:	Patrick McHardy <kaber@...sh.net>
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH 8/9]: sch_hfsc: Use ->requeue queue instead of ops.

On Tue, Aug 19, 2008 at 03:07:10PM +0200, Patrick McHardy wrote:
> David Miller wrote:
>> From: Patrick McHardy <kaber@...sh.net>
>> Date: Mon, 18 Aug 2008 16:11:57 +0200
>>
>>> I think we really need either ->requeue or a real ->peek operation.
>>
>> All the code duplication and complexity is what I'm trying to avoid.
>>
>> I see no value in overhauling and auditing all of these ->requeue()
>> implementations and how they return status codes when the facility
>> itself is largely superfluous.
>>
>> Maybe we can simply add a "bool peek" argument or some flags to
>> ->dequeue() instead.
>
> Yes, that should work. It might get a big ugly though since the
> ->dequeue functions have to make sure not to modify any state
> while peeking.

I'm not sure what are conclusions here wrt. this patchset, but since
David made this mistake and added me to CC, here are my doubts:

- maybe I miss something, but it seems there is something strange with
  using qdisc_dequeue(), e.g. how htb_dequeue_queue() in this call
  skb = qdisc_dequeue(cl->un.leaf.q);
  can ever get anything here?:
  struct sk_buff *skb = __skb_dequeue(&sch->requeue);
  Isn't it requeued in root qdisc? But even if it's OK, isn't there
  needed some additional code to control queue length?

- initially David wrote about simplifying this, so I thought it's
  about some simple buffer outside of qdiscs' code; now it's a bit
  more than this; sure, it's simpler but I guess, soon, after a few
  (ADSL?) fixes and optimizations there will be probably no difference.
  Peeking doesn't look to me necessarily simpler either.

So, IMHO, if it's not going to be something really simple I doubt it's
worth to bother with this. Anyway, I hope David will give some warning
yet before merging this.

Thanks,
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