| Message ID | 20190327165632.10711-2-mkl@pengutronix.de |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/2] net: sch_generic: add flag IFF_FIFO_QUEUE to use pfifo_fast as default scheduler | expand |
On Wed, Mar 27, 2019 at 9:56 AM Marc Kleine-Budde <mkl@pengutronix.de> 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 > 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. I wonder if we just need to allow arbitrary default qdisc per netdevice while you are on it. A private flag is simply a boolean, perhaps in the future other type of devices wants other default qdiscs, so that could make it more flexible. Just a thought. Thanks.
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. > 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@gmail.com> > Cc: Jamal Hadi Salim <jhs@mojatatu.com> > Cc: Cong Wang <xiyou.wangcong@gmail.com> > Cc: Jiri Pirko <jiri@resnulli.us> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.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? Best regards Uwe
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@gmail.com> >> Cc: Jamal Hadi Salim <jhs@mojatatu.com> >> Cc: Cong Wang <xiyou.wangcong@gmail.com> >> Cc: Jiri Pirko <jiri@resnulli.us> >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.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
On 3/27/19 6:14 PM, Cong Wang wrote: > On Wed, Mar 27, 2019 at 9:56 AM Marc Kleine-Budde <mkl@pengutronix.de> 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 >> 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. > > I wonder if we just need to allow arbitrary default qdisc per netdevice > while you are on it. A private flag is simply a boolean, perhaps in the > future other type of devices wants other default qdiscs, so that could > make it more flexible. From my point of view there is networking hardware that use protocols that work with (i.e. benefit from) fq_codel (hash flow/queue/head drop). The silent head drop is the most prominent reason why it doesn't work on CAN. I haven't dug deep enough into the code to see if skb->hash is used or what the flow dissector will do on CAN frames. So reordering of CAN frames (if something else than skb->priority is used) might be a problem, too. From my point of view, if your networking hardware and the protocols on top don't like re-ordering or silent head drop, than pfifo_fast is probably a good default choice. I discussed the problem a bit at netdev 0x13 and one point someone mentioned is that if there is a generic set this qdisc function people might start to add this to network drivers to "optimize" them for their special workflow or test case. Doubts aside, how should an arbitrary default qdisc per netdevice look like? Add a string "default_qdisc" to the netdev? Lookup qdisc by string during DOWN->UP transition? What do if that qdisc is not compiled into the kernel? Or rather use an array of qdiscs with one of sch_generic defaults? Marc
On Wed, Mar 27, 2019 at 1:11 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > Doubts aside, how should an arbitrary default qdisc per netdevice look > like? Add a string "default_qdisc" to the netdev? Lookup qdisc by string > during DOWN->UP transition? What do if that qdisc is not compiled into > the kernel? Or rather use an array of qdiscs with one of sch_generic > defaults? I think you can just save a Qdisc_ops pointer in netdevice, like how we install the default qdisc: 38 const struct Qdisc_ops *default_qdisc_ops = &pfifo_fast_ops; 39 EXPORT_SYMBOL(default_qdisc_ops); And hard-code whatever default into your netdevice init code. At least for pfifo_fast, you don't need to worry about module loading. If you really do, you can call qdisc_lookup_default() and request_module() like what qdisc_set_default() does. Of course, you can refactor qdisc_set_default() and call it too. Thanks.
Marc Kleine-Budde <mkl@pengutronix.de> writes: > On 3/27/19 6:14 PM, Cong Wang wrote: >> On Wed, Mar 27, 2019 at 9:56 AM Marc Kleine-Budde <mkl@pengutronix.de> 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 >>> 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. >> >> I wonder if we just need to allow arbitrary default qdisc per netdevice >> while you are on it. A private flag is simply a boolean, perhaps in the >> future other type of devices wants other default qdiscs, so that could >> make it more flexible. > > From my point of view there is networking hardware that use protocols > that work with (i.e. benefit from) fq_codel (hash flow/queue/head drop). > > The silent head drop is the most prominent reason why it doesn't work on > CAN. I haven't dug deep enough into the code to see if skb->hash is used > or what the flow dissector will do on CAN frames. So reordering of CAN > frames (if something else than skb->priority is used) might be a > problem, too. > > From my point of view, if your networking hardware and the protocols on > top don't like re-ordering or silent head drop, than pfifo_fast is > probably a good default choice. > > I discussed the problem a bit at netdev 0x13 and one point someone > mentioned is that if there is a generic set this qdisc function people > might start to add this to network drivers to "optimize" them for > their special workflow or test case. I think I was one of the people you spoke with about this. I agree that the flag approach makes sense, since I view the requirements of the CAN protocol as very specifically being met by a FIFO queue. And yeah I do think we should push back on every device type defining each own arbitrary qdisc default; having the two very specific exceptions "no queue" and "FIFO queue" to the general qdisc default setting makes it explicit that this is for special cases only, and that any other optimisation of the qdisc configuration should be done in userspace. -Toke
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 */ enum netdev_priv_flags { IFF_802_1Q_VLAN = 1<<0, @@ -1530,6 +1531,7 @@ enum netdev_priv_flags { IFF_FAILOVER = 1<<27, IFF_FAILOVER_SLAVE = 1<<28, IFF_L3MDEV_RX_HANDLER = 1<<29, + IFF_FIFO_QUEUE = 1<<30, }; #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN @@ -1561,6 +1563,7 @@ enum netdev_priv_flags { #define IFF_FAILOVER IFF_FAILOVER #define IFF_FAILOVER_SLAVE IFF_FAILOVER_SLAVE #define IFF_L3MDEV_RX_HANDLER IFF_L3MDEV_RX_HANDLER +#define IFF_FIFO_QUEUE IFF_FIFO_QUEUE /** * struct net_device - The DEVICE structure. diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index 81356ef38d1d..c309d0751cbc 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -1049,6 +1049,9 @@ static void attach_one_default_qdisc(struct net_device *dev, struct Qdisc *qdisc; const struct Qdisc_ops *ops = default_qdisc_ops; + if (dev->priv_flags & IFF_FIFO_QUEUE) + ops = &pfifo_fast_ops; + if (dev->priv_flags & IFF_NO_QUEUE) ops = &noqueue_qdisc_ops;
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 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@gmail.com> Cc: Jamal Hadi Salim <jhs@mojatatu.com> Cc: Cong Wang <xiyou.wangcong@gmail.com> Cc: Jiri Pirko <jiri@resnulli.us> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> --- include/linux/netdevice.h | 3 +++ net/sched/sch_generic.c | 3 +++ 2 files changed, 6 insertions(+)