Message-ID: <1397185365.6230.3.camel@localhost>
Date:	Fri, 11 Apr 2014 11:02:45 +0800
From:	Jason Wang <jasowang@...hat.com>
To:	Steven Galgano <sgalgano@...acentlink.com>
Cc:	"Michael S. Tsirkin" <mst@...hat.com>, davem@...emloft.net,
	xemul@...allels.com, wuzhy@...ux.vnet.ibm.com, therbert@...gle.com,
	yamato@...hat.com, richardcochran@...il.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Brian Adamson <Brian.Adamson@....navy.mil>,
	Joseph Giovatto <jgiovatto@...acentlink.com>
Subject: Re: [PATCH] tuntap: add flow control to support back pressure

On Thu, 2014-04-10 at 21:42 -0400, Steven Galgano wrote:
> On 04/10/2014 06:29 AM, 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@....navy.mil>
> >> Tested-by: Joseph Giovatto <jgiovatto@...acentlink.com>
> >> Signed-off-by: Steven Galgano <sgalgano@...acentlink.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.
> > 
> > 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.
> > 
> > 
> > 
> 
> It was only after recent upgrades that we picked up a newer kernel and
> discovered the change to the default tun mode.
> 
> The new default behavior has broken applications that depend on the
> legacy behavior. Although not documented, the legacy behavior was well
> known at least to those working in the back pressure research community.
> The default legacy mode was/is a valid use case although I am not sure
> it fits with recent multiqueue support.
> 
> When back pressure protocols are running over a tun interface they
> require legacy flow control in order to communicate congestion detected
> on the physical media they are using. Multiqueues do not apply here.
> These protocols only use one queue, so netif_tx_stop_all_queues() is the
> necessary behavior.
> 
> I'm not tied to the idea of IFF_FLOW_CONTROL. I was aiming for middle
> ground where an application controlling the tun interface can state it
> wants the legacy flow control behavior understanding its limitations
> concerning multiple queues.
> 
> What if we resurrect IFF_ONE_QUEUE and use that as a mechanism to
> indicate legacy flow control. A tun instance initially configured with
> IFF_ONE_QUEUE would not be allowed to attach or detach queues with
> TUNSETQUEUE and any additional opens with the same device name would
> fail. This mode would use the
> netif_tx_stop_all_queues()/netif_tx_wake_all_queues() flow control
> mechanism.
> 

Even if you choose this method, using
netif_tx_stop_queue()/netif_tx_wake_queue() should still be ok and more
readable.
> If a tun application wants the current default behavior with only a
> single queue, it would not set the IFF_ONE_QUEUE flag. Not setting
> IFF_MULTI_QUEUE would not imply IFF_ONE_QUEUE.
> 
> I'd be happy to implement this change if it is an acceptable solution.
> This would allow multiqueue tun development to advance while still
> supporting use cases dependent on legacy flow control.
> 
> -steve
> 
> >> ---
> >> 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
> --
> 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


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

Powered by blists - more mailing lists