netdev - Re: [PATCH 0/3] net: Byte queue limit patch series

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: <BANLkTinFmfJbYxvZ+6rrAy+6XsfwF5an5Q@mail.gmail.com>
Date:	Sun, 1 May 2011 19:41:34 -0700
From:	Tom Herbert <therbert@...gle.com>
To:	David Miller <davem@...emloft.net>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH 0/3] net: Byte queue limit patch series

On Fri, Apr 29, 2011 at 11:54 AM, David Miller <davem@...emloft.net> wrote:
> From: Tom Herbert <therbert@...gle.com>
> Date: Mon, 25 Apr 2011 21:38:06 -0700 (PDT)
>
>> This patch series implements byte queue limits (bql) for NIC TX queues.
>
> I like the idea, I don't like how much drivers have to be involved
> in this.
>
> TX queue handling is already too involved for driver writers, so
> having them need to get all of these new hooks right is a non-starter.
>
> I don't even think it's necessary to be honest, I think you can hide
> to bulk of it.
>
> alloc_etherdev_mq() can initialize the per-txq bql instances.
>
> Add new interface, netdev_free_tx_skb(txq, skb) which can do the
> completion accounting.  Actually the 'txq' argument is probably
> superfluous as it can be obtained from the skb itself.
>
Okay, but I think the call at the end of TX completion processing is
still probably needed.  The algorithm is trying to determine the
number of bytes that completed at each interrupt.

> dev_hard_start_xmit() can do the "sent" processing.
>
> Then the only thing you're doing is replacing dev_kfree_skb*()
> in the TX path of the driver with the new netdev_free_tx_skb()
> thing.
>
> Maybe you can add a "netdev_tx_queue_reset()" hook for asynchronous
> device resets.  Otherwise the bql reset can occur in generic code
> right at ->ndo_open().
>
Reset probably isn't necessary anyway if all the skb's are properly
completed with netdev_free_tx_skb.

> Finally, you manage the bql limit logic in the existing generic netdev
> TX start/stop interfaces.  If the user asks for "start" but the bql
> is overlimit, simply ignore the request.  The driver will just signal
> another "start" when the next TX packet completes.
>
> Similarly, when the qdisc is queuing up a packet to
> dev_hard_start_xmit() you can, for example, preemptively do a "stop"
> on the queue if you find bql is overlimit.
>
Unfortunately, there is still an additional complexity if we don't
piggy back on the logic in the driver to stop the queue.  I believe
that either this would require another queue state for queue being
stopped for bql which looks pretty cumbersome, so that wrapping this
in a qdisc might be a better possibility.

Tom

> Thanks Tom.
>
>
--
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