Patchwork tun mq failure

login
register
mail settings
Submitter Michael S. Tsirkin
Date Jan. 23, 2013, 11:41 a.m.
Message ID <20130123114102.GA10426@redhat.com>
Download mbox | patch
Permalink /patch/214894/
State RFC
Delegated to: David Miller
Headers show

Comments

Michael S. Tsirkin - Jan. 23, 2013, 11:41 a.m.
On Wed, Jan 23, 2013 at 12:06:40PM +0100, Hannes Frederic Sowa wrote:
> On Wed, Jan 23, 2013 at 12:05:16PM +0200, Michael S. Tsirkin wrote:
> > This is when trying to start a VPN using some old openvpn binary so MQ
> > is not set.
> > 
> > So
> > 1. I think we should limit allocation of MQ to when MQ flag is set in SETIFF.
> > 2. order 7 allocation is 2^^7 pages - about half a megabyte of contigious
> >    memory. This is quite likely to fail.
> >    Let's start with a small limit on number of queues, like 8?
> >    Then we know it will succeed.
> >    Longer term we might want to solve it differently.
> 
> This has been come up before:
> http://thread.gmane.org/gmane.linux.network/255647/focus=255902
> 
> I think a solution to this problem is still outstanding.

Right. What (at least I) missed is that it's the
queue array allocation that fails here.
So I think something like the following will sort the first issue
(compiled only):

For the second, for 3.8 maybe the prudent thing to do is
to set MAX_TAP_QUEUES to a small value, like 8, to avoid
userspace relying on a large number of queues being available,
and look at a better way to do this longer term, like
using an array of pointers.

--->

tun: don't waste memory on unused queues

If MQ flag is off, we never attach more than 1 queue.
So let's not allocate memory for the unused ones.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Wang - Jan. 23, 2013, 12:10 p.m.
On 01/23/2013 07:41 PM, Michael S. Tsirkin wrote:
> On Wed, Jan 23, 2013 at 12:06:40PM +0100, Hannes Frederic Sowa wrote:
>> On Wed, Jan 23, 2013 at 12:05:16PM +0200, Michael S. Tsirkin wrote:
>>> This is when trying to start a VPN using some old openvpn binary so MQ
>>> is not set.
>>>
>>> So
>>> 1. I think we should limit allocation of MQ to when MQ flag is set in SETIFF.
>>> 2. order 7 allocation is 2^^7 pages - about half a megabyte of contigious
>>>    memory. This is quite likely to fail.
>>>    Let's start with a small limit on number of queues, like 8?
>>>    Then we know it will succeed.
>>>    Longer term we might want to solve it differently.
>> This has been come up before:
>> http://thread.gmane.org/gmane.linux.network/255647/focus=255902
>>
>> I think a solution to this problem is still outstanding.
> Right. What (at least I) missed is that it's the
> queue array allocation that fails here.
> So I think something like the following will sort the first issue
> (compiled only):
>
> For the second, for 3.8 maybe the prudent thing to do is
> to set MAX_TAP_QUEUES to a small value, like 8, to avoid
> userspace relying on a large number of queues being available,
> and look at a better way to do this longer term, like
> using an array of pointers.

Sure, this is just the method I reply in that thread. Not sure 8 is the
best, but since it fit into one page, should be ok. Maybe we can use
flex array to avoid high order memory allocation in the longer term.
>
> --->
>
> tun: don't waste memory on unused queues
>
> If MQ flag is off, we never attach more than 1 queue.
> So let's not allocate memory for the unused ones.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index af372d0..813d303 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1577,6 +1577,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  	else {
>  		char *name;
>  		unsigned long flags = 0;
> +		unsigned int max_tap_queues;
>  
>  		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
>  			return -EPERM;
> @@ -1599,9 +1600,13 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  		if (*ifr->ifr_name)
>  			name = ifr->ifr_name;
>  
> +		if (ifr->ifr_flags & IFF_MULTI_QUEUE)
> +			max_tap_queues = MAX_TAP_QUEUES;
> +		else
> +			max_tap_queues = 1;
>  		dev = alloc_netdev_mqs(sizeof(struct tun_struct), name,
>  				       tun_setup,
> -				       MAX_TAP_QUEUES, MAX_TAP_QUEUES);
> +				       max_tap_queues, max_tap_queues);
>  		if (!dev)
>  			return -ENOMEM;
>  

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael S. Tsirkin - Jan. 23, 2013, 1:18 p.m.
On Wed, Jan 23, 2013 at 08:10:35PM +0800, Jason Wang wrote:
> On 01/23/2013 07:41 PM, Michael S. Tsirkin wrote:
> > On Wed, Jan 23, 2013 at 12:06:40PM +0100, Hannes Frederic Sowa wrote:
> >> On Wed, Jan 23, 2013 at 12:05:16PM +0200, Michael S. Tsirkin wrote:
> >>> This is when trying to start a VPN using some old openvpn binary so MQ
> >>> is not set.
> >>>
> >>> So
> >>> 1. I think we should limit allocation of MQ to when MQ flag is set in SETIFF.
> >>> 2. order 7 allocation is 2^^7 pages - about half a megabyte of contigious
> >>>    memory. This is quite likely to fail.
> >>>    Let's start with a small limit on number of queues, like 8?
> >>>    Then we know it will succeed.
> >>>    Longer term we might want to solve it differently.
> >> This has been come up before:
> >> http://thread.gmane.org/gmane.linux.network/255647/focus=255902
> >>
> >> I think a solution to this problem is still outstanding.
> > Right. What (at least I) missed is that it's the
> > queue array allocation that fails here.
> > So I think something like the following will sort the first issue
> > (compiled only):
> >
> > For the second, for 3.8 maybe the prudent thing to do is
> > to set MAX_TAP_QUEUES to a small value, like 8, to avoid
> > userspace relying on a large number of queues being available,
> > and look at a better way to do this longer term, like
> > using an array of pointers.
> 
> Sure, this is just the method I reply in that thread. Not sure 8 is the
> best, but since it fit into one page, should be ok. Maybe we can use
> flex array to avoid high order memory allocation in the longer term.

Right so let's for now use  DEFAULT_MAX_NUM_RSS_QUEUES as Eric suggested.

> >
> > --->
> >
> > tun: don't waste memory on unused queues
> >
> > If MQ flag is off, we never attach more than 1 queue.
> > So let's not allocate memory for the unused ones.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > ---
> >
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index af372d0..813d303 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -1577,6 +1577,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
> >  	else {
> >  		char *name;
> >  		unsigned long flags = 0;
> > +		unsigned int max_tap_queues;
> >  
> >  		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
> >  			return -EPERM;
> > @@ -1599,9 +1600,13 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
> >  		if (*ifr->ifr_name)
> >  			name = ifr->ifr_name;
> >  
> > +		if (ifr->ifr_flags & IFF_MULTI_QUEUE)
> > +			max_tap_queues = MAX_TAP_QUEUES;
> > +		else
> > +			max_tap_queues = 1;
> >  		dev = alloc_netdev_mqs(sizeof(struct tun_struct), name,
> >  				       tun_setup,
> > -				       MAX_TAP_QUEUES, MAX_TAP_QUEUES);
> > +				       max_tap_queues, max_tap_queues);
> >  		if (!dev)
> >  			return -ENOMEM;
> >  
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index af372d0..813d303 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1577,6 +1577,7 @@  static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 	else {
 		char *name;
 		unsigned long flags = 0;
+		unsigned int max_tap_queues;
 
 		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
 			return -EPERM;
@@ -1599,9 +1600,13 @@  static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 		if (*ifr->ifr_name)
 			name = ifr->ifr_name;
 
+		if (ifr->ifr_flags & IFF_MULTI_QUEUE)
+			max_tap_queues = MAX_TAP_QUEUES;
+		else
+			max_tap_queues = 1;
 		dev = alloc_netdev_mqs(sizeof(struct tun_struct), name,
 				       tun_setup,
-				       MAX_TAP_QUEUES, MAX_TAP_QUEUES);
+				       max_tap_queues, max_tap_queues);
 		if (!dev)
 			return -ENOMEM;