netdev - RE: [RFC PATCH] sched: only dequeue if packet can be queued to hardware queue.

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: <80769D7B14936844A23C0C43D9FBCF0F1598E4A3@orsmsx501.amr.corp.intel.com>
Date:	Fri, 19 Sep 2008 09:26:29 -0700
From:	"Duyck, Alexander H" <alexander.h.duyck@...el.com>
To:	Jarek Poplawski <jarkao2@...il.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"kaber@...sh.net" <kaber@...sh.net>
Subject: RE: [RFC PATCH] sched: only dequeue if packet can be queued to
	hardware queue.

Jarek Poplawski wrote:
> On Thu, Sep 18, 2008 at 09:44:19PM +0200, Jarek Poplawski wrote:
>> Alexander Duyck wrote, On 09/18/2008 08:43 AM:
>> ...
>>> ---
>>> This patch changes the behavior of the sch->dequeue to only
>>> dequeue a packet if the queue it is bound for is not currently
>>> stopped.  This functionality is provided via a new op called
>>> smart_dequeue.
>>>
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@...el.com>
>>> ---
>
> Alexander, I guess you've seen my last messages/patches in this thread
> and noticed that this __netif_schedule() problem is present both in
> this RFC patch and sch_multiq: if skb isn't dequeued because of inner
> tx_queue stopped state check, there is missing __netif_schedule()
> before exit from qdisc_run().
>
> Jarek P.

Actually most of that is handled in the fact that netif_tx_wake_queue
will call __netif_schedule when it decides to wake up a queue that has
been stopped.  Putting it in skb_dequeue is unnecessary, and the way
you did it would cause issues because you should be scheduling the root
qdisc, not the one currently doing the dequeue.

My bigger concern is the fact one queue is back to stopping all queues.
By using one global XOFF state, you create head-of-line blocking.  I can
see the merit in this approach but the problem is for multiqueue the
queues really should be independent.  What your change does is reduce the
benefit of having multiqueue by throwing in a new state which pretty much
matches what netif_stop/wake_queue used to do in the 2.6.26 version of tx
multiqueue.

Also changing dequeue_skb will likely cause additional issues for
several qdiscs as it doesn't report anything up to parent queues, as a
result you will end up with qdiscs like prio acting more like multiq
because they won't know if a queue is empty, stopped, or throttled.
Also I believe this will cause a panic on pfifo_fast and several other
qdiscs because some check to see if there are packets in the queue and
if so dequeue with the assumption that they will get a packet out.  You
will need to add checks for this to resolve this issue.

The one thing I liked about my approach was that after I was done you
could have prio as a parent of multiq leaves or multiq as the parent of
prio leaves and all the hardware queues would receive the packets in the
same order as the result.

Thanks,

Alex
--
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