netdev - Re: [PATCH 1/2] net: sch_generic: add flag IFF_FIFO_QUEUE to use pfifo

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: <a3deee8b-f1de-a9f2-339f-523022c502e6@pengutronix.de>
Date:   Wed, 27 Mar 2019 20:27:51 +0100
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>,
        Dave Taht <dave.taht@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>, kernel@...gutronix.de,
        Cong Wang <xiyou.wangcong@...il.com>,
        linux-can@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH 1/2] net: sch_generic: add flag IFF_FIFO_QUEUE to use
 pfifo_fast as default scheduler

On 3/27/19 7:53 PM, Uwe Kleine-König wrote:
> On Wed, Mar 27, 2019 at 05:56:31PM +0100, Marc Kleine-Budde wrote:
>> There is networking hardware that isn't based on Ethernet for layers 1 and 2.
>>
>> For example CAN.
>>
>> CAN is a multi-master serial bus standard for connecting Electronic Control
>> Units [ECUs] also known as nodes. A frame on the CAN bus carries up to 8 bytes
>> of payload. Frame corruption is detected by a CRC. However frame loss due to
>> corruption is possible, but a quite unusual phenomenon.
>>
>> While fq_codel works great for TCP/IP, it doesn't for CAN. There are a lot of
>> legacy protocols on top of CAN, which are not build with flow control or high
>> CAN frame drop rates in mind.
>>
>> When using fq_codel, as soon as the queue reaches a certain delay based length,
>> skbs from the head of the queue are silently dropped. Silently meaning that the
>> user space using a send() or similar syscall doesn't get an error. However
>> TCP's flow control algorithm will detect dropped packages and adjust the
> 
> s/package/packet/ here and in a few more locations in this commit log.

Thanks, fixed.

>> bandwidth accordingly.
>>
>> When using fq_codel and sending raw frames over CAN, which is the common use
>> case, the user space thinks the package has been sent without problems, because
>> send() returned without an error. pfifo_fast will drop skbs, if the queue
>> length exceeds the maximum. But with this scheduler the skbs at the tail are
>> dropped, an error (-ENOBUFS) is propagated to user space. So that the user
>> space can slow down the package generation.
>>
>> On distributions, where fq_codel is made default via CONFIG_DEFAULT_NET_SCH
>> during compile time, or set default during runtime with sysctl
>> net.core.default_qdisc (see [1]), we get a bad user experience. In my test case
>> with pfifo_fast, I can transfer thousands of million CAN frames without a frame
>> drop. On the other hand with fq_codel there is more then one lost CAN frame per
>> thousand frames.
>>
>> As pointed out fq_codel is not suited for CAN hardware, so this patch
>> introduces a new netdev_priv_flag called "IFF_FIFO_QUEUE" (in contrast to the
>> existing "IFF_NO_QUEUE").
>>
>> During transition of a netdev from down to up state the default queuing
>> discipline is attached by attach_default_qdiscs() with the help of
>> attach_one_default_qdisc(). This patch modifies attach_one_default_qdisc() to
>> attach the pfifo_fast (pfifo_fast_ops) if the "IFF_FIFO_QUEUE" flag is set.
>>
>> [1] https://github.com/systemd/systemd/issues/9194
>>
>> Cc: Dave Taht <dave.taht@...il.com>
>> Cc: Jamal Hadi Salim <jhs@...atatu.com>
>> Cc: Cong Wang <xiyou.wangcong@...il.com>
>> Cc: Jiri Pirko <jiri@...nulli.us>
>> Signed-off-by: Marc Kleine-Budde <mkl@...gutronix.de>
>> ---
>>  include/linux/netdevice.h | 3 +++
>>  net/sched/sch_generic.c   | 3 +++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 166fdc0a78b4..1867e27e3369 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1498,6 +1498,7 @@ struct net_device_ops {
>>   * @IFF_FAILOVER: device is a failover master device
>>   * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
>>   * @IFF_L3MDEV_RX_HANDLER: only invoke the rx handler of L3 master device
>> + * @IFF_FIFO_QUEUE: device must run with FIFO qdisc attached. skb drop without NET_XMIT_DROP is fatal
> 
> Do you need the FIFO property or only that the qdisc doesn't silently
> drop packets? I don't know which other qdiscs are around, but depending
> on the answer to this question other than pfifo_fast might be suitable?

No silent dropping is mandatory. No reordering of outgoing packets (if
not configured to do so) is mandatory.

Maybe there are other qdiscs that work on CAN, but pfifo_fast is a sane
default.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |



Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)