Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH] tuntap: add flow control to support back pressure | From | Jason Wang <> | Date | Fri, 11 Apr 2014 10:57:55 +0800 |
| |
On Thu, 2014-04-10 at 13:29 +0300, Michael S. Tsirkin wrote: > On Wed, Apr 09, 2014 at 10:19:40PM -0400, Steven Galgano wrote: > > Add tuntap flow control support for use by back pressure routing protocols. Setting the new TUNSETIFF flag IFF_FLOW_CONTROL, will signal resources as unavailable when the tx queue limit is reached by issuing a netif_tx_stop_all_queues() rather than discarding frames. A netif_tx_wake_all_queues() is issued after reading a frame from the queue to signal resource availability. > > > > Back pressure capability was previously supported by the legacy tun default mode. This change restores that functionality, which was last present in v3.7. > > > > Reported-by: Brian Adamson <brian.adamson@nrl.navy.mil> > > Tested-by: Joseph Giovatto <jgiovatto@adjacentlink.com> > > Signed-off-by: Steven Galgano <sgalgano@adjacentlink.com> > > I don't think it's a good idea. > > This trivial flow control really created more problems than it was worth. > > In particular this blocks all flows so it's trivially easy for one flow > to block and starve all others: just send a bunch of packets to loopback > destinations that get queued all over the place. > > Luckily it was never documented so we changed the default and nothing > seems to break, but we won't be so lucky if we add an explicit API. > > > One way to implement this would be with ubuf_info callback this is > already invoked in most places where a packet might get stuck for a long > time. It's still incomplete though: this will prevent head of queue > blocking literally forever, but a single bad flow can still degrade > performance significantly.
This is send queue for tuntap. Like all other real nics, we can solve this through fairness qdiscs? > > Another alternative is to try and isolate the flows that we > can handle and throttle them. > > It's all fixable but we really need to fix the issues *before* > exposing the interface to userspace. > > > > > --- > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > index ee328ba..268130c 100644 > > --- a/drivers/net/tun.c > > +++ b/drivers/net/tun.c > > @@ -783,8 +783,19 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev) > > * number of queues. > > */ > > if (skb_queue_len(&tfile->socket.sk->sk_receive_queue) * numqueues > > - >= dev->tx_queue_len) > > - goto drop; > > + >= dev->tx_queue_len) { > > + if (tun->flags & TUN_FLOW_CONTROL) { > > + /* Resources unavailable stop transmissions */ > > + netif_tx_stop_all_queues(dev); > > + > > + /* We won't see all dropped packets individually, so > > + * over run error is more appropriate. > > + */ > > + dev->stats.tx_fifo_errors++; > > + } else { > > + goto drop; > > + } > > + } > > > > if (unlikely(skb_orphan_frags(skb, GFP_ATOMIC))) > > goto drop; > > @@ -1362,6 +1373,9 @@ static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile, > > continue; > > } > > > > + /* Wake in case resources previously signaled unavailable */ > > + netif_tx_wake_all_queues(tun->dev); > > + > > ret = tun_put_user(tun, tfile, skb, iv, len); > > kfree_skb(skb); > > break; > > @@ -1550,6 +1564,9 @@ static int tun_flags(struct tun_struct *tun) > > if (tun->flags & TUN_PERSIST) > > flags |= IFF_PERSIST; > > > > + if (tun->flags & TUN_FLOW_CONTROL) > > + flags |= IFF_FLOW_CONTROL; > > + > > return flags; > > } > > > > @@ -1732,6 +1749,11 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) > > else > > tun->flags &= ~TUN_TAP_MQ; > > > > + if (ifr->ifr_flags & IFF_FLOW_CONTROL) > > + tun->flags |= TUN_FLOW_CONTROL; > > + else > > + tun->flags &= ~TUN_FLOW_CONTROL; > > + > > /* Make sure persistent devices do not get stuck in > > * xoff state. > > */ > > @@ -1900,7 +1922,8 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, > > * This is needed because we never checked for invalid flags on > > * TUNSETIFF. */ > > return put_user(IFF_TUN | IFF_TAP | IFF_NO_PI | IFF_ONE_QUEUE | > > - IFF_VNET_HDR | IFF_MULTI_QUEUE, > > + IFF_VNET_HDR | IFF_MULTI_QUEUE | > > + IFF_FLOW_CONTROL, > > (unsigned int __user*)argp); > > } else if (cmd == TUNSETQUEUE) > > return tun_set_queue(file, &ifr); > > diff --git a/include/uapi/linux/if_tun.h b/include/uapi/linux/if_tun.h > > index e9502dd..bcf2790 100644 > > --- a/include/uapi/linux/if_tun.h > > +++ b/include/uapi/linux/if_tun.h > > @@ -36,6 +36,7 @@ > > #define TUN_PERSIST 0x0100 > > #define TUN_VNET_HDR 0x0200 > > #define TUN_TAP_MQ 0x0400 > > +#define TUN_FLOW_CONTROL 0x0800 > > > > /* Ioctl defines */ > > #define TUNSETNOCSUM _IOW('T', 200, int) > > @@ -70,6 +71,7 @@ > > #define IFF_MULTI_QUEUE 0x0100 > > #define IFF_ATTACH_QUEUE 0x0200 > > #define IFF_DETACH_QUEUE 0x0400 > > +#define IFF_FLOW_CONTROL 0x0010 > > /* read-only flag */ > > #define IFF_PERSIST 0x0800 > > #define IFF_NOFILTER 0x1000
| ![\](/images/icornerr.gif) |