diff mbox

[RFC,2/2] tun: don't set a default qdisc

Message ID 1a216fb6cade7dcd7d2c1d4ce113a1ee6e3fdeb6.1460393493.git.pabeni@redhat.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Paolo Abeni April 13, 2016, 9:04 a.m. UTC
This patch disables the default qdisc by explicitly setting the
IFF_NO_QUEUE private flag so that now the tun xmit path do not
require any lock by default.

The default qdisc was first removed as a side effect of commit
f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/tun.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Michael S. Tsirkin April 13, 2016, 10:26 a.m. UTC | #1
On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
> This patch disables the default qdisc by explicitly setting the
> IFF_NO_QUEUE private flag so that now the tun xmit path do not
> require any lock by default.
> 
> The default qdisc was first removed as a side effect of commit
> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

I wonder about this back and forth.
Jason, do you see a workload where the default qdisc
is preferable?

> ---
>  drivers/net/tun.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 42992dc..0a0b63c 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1081,6 +1081,7 @@ static void tun_net_init(struct net_device *dev)
>  
>  		break;
>  	}
> +	dev->priv_flags |= IFF_NO_QUEUE;
>  }
>  
>  /* Character device part */
> -- 
> 1.8.3.1
David Miller April 13, 2016, 3:22 p.m. UTC | #2
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Wed, 13 Apr 2016 13:26:51 +0300

> On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
>> This patch disables the default qdisc by explicitly setting the
>> IFF_NO_QUEUE private flag so that now the tun xmit path do not
>> require any lock by default.
>> 
>> The default qdisc was first removed as a side effect of commit
>> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
>> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
>> 
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> I wonder about this back and forth.
> Jason, do you see a workload where the default qdisc
> is preferable?

I don't think you should change this, putting the default back was a
bug fix.

I think this series is taking way too many liberties in order to
achieve locklessness, in my opinion.

If you have proper multi-queue, the qdisc lock and the netdev transmit
lock are almost completely free.
Jason Wang April 14, 2016, 6:49 a.m. UTC | #3
On 04/13/2016 06:26 PM, Michael S. Tsirkin wrote:
> On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
>> This patch disables the default qdisc by explicitly setting the
>> IFF_NO_QUEUE private flag so that now the tun xmit path do not
>> require any lock by default.
>>
>> The default qdisc was first removed as a side effect of commit
>> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
>> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
>>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> I wonder about this back and forth.
> Jason, do you see a workload where the default qdisc
> is preferable?

I don't know, but we used to behave like this so we'd better keep it.

An interesting thing is I vaguely remember that you have some concern
when I propose IFF_NO_QUEUE for macvtap[1] :)

I think this could be done by management or more safe by introducing a
new tun flag (TUN_NO_QUEUE).

[1] https://lkml.org/lkml/2015/8/24/147

>
>> ---
>>  drivers/net/tun.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 42992dc..0a0b63c 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -1081,6 +1081,7 @@ static void tun_net_init(struct net_device *dev)
>>  
>>  		break;
>>  	}
>> +	dev->priv_flags |= IFF_NO_QUEUE;
>>  }
>>  
>>  /* Character device part */
>> -- 
>> 1.8.3.1
Michael S. Tsirkin April 14, 2016, 9:05 a.m. UTC | #4
On Thu, Apr 14, 2016 at 02:49:28PM +0800, Jason Wang wrote:
> 
> 
> On 04/13/2016 06:26 PM, Michael S. Tsirkin wrote:
> > On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
> >> This patch disables the default qdisc by explicitly setting the
> >> IFF_NO_QUEUE private flag so that now the tun xmit path do not
> >> require any lock by default.
> >>
> >> The default qdisc was first removed as a side effect of commit
> >> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
> >> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
> >>
> >> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > I wonder about this back and forth.
> > Jason, do you see a workload where the default qdisc
> > is preferable?
> 
> I don't know, but we used to behave like this so we'd better keep it.
> 
> An interesting thing is I vaguely remember that you have some concern
> when I propose IFF_NO_QUEUE for macvtap[1] :)
> 
> [1] https://lkml.org/lkml/2015/8/24/147

It's the same concern - that we aren't fully addressing
the problem, so if user configures a qdisc, we are back to square 1.
It's especially annoying that IIUC in this setup, if one
does configured a non default qdisc, there's no way to go back.
It doesn't necessarily mean we must not do it as an intermediate step,
though.

> 
> I think this could be done by management or more safe by introducing a
> new tun flag (TUN_NO_QUEUE).

What exactly does this flag do/mean?

> >
> >> ---
> >>  drivers/net/tun.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> >> index 42992dc..0a0b63c 100644
> >> --- a/drivers/net/tun.c
> >> +++ b/drivers/net/tun.c
> >> @@ -1081,6 +1081,7 @@ static void tun_net_init(struct net_device *dev)
> >>  
> >>  		break;
> >>  	}
> >> +	dev->priv_flags |= IFF_NO_QUEUE;
> >>  }
> >>  
> >>  /* Character device part */
> >> -- 
> >> 1.8.3.1
Jason Wang April 14, 2016, 9:07 a.m. UTC | #5
On 04/14/2016 05:05 PM, Michael S. Tsirkin wrote:
> On Thu, Apr 14, 2016 at 02:49:28PM +0800, Jason Wang wrote:
>> > 
>> > 
>> > On 04/13/2016 06:26 PM, Michael S. Tsirkin wrote:
>>> > > On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
>>>> > >> This patch disables the default qdisc by explicitly setting the
>>>> > >> IFF_NO_QUEUE private flag so that now the tun xmit path do not
>>>> > >> require any lock by default.
>>>> > >>
>>>> > >> The default qdisc was first removed as a side effect of commit
>>>> > >> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
>>>> > >> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
>>>> > >>
>>>> > >> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> > > I wonder about this back and forth.
>>> > > Jason, do you see a workload where the default qdisc
>>> > > is preferable?
>> > 
>> > I don't know, but we used to behave like this so we'd better keep it.
>> > 
>> > An interesting thing is I vaguely remember that you have some concern
>> > when I propose IFF_NO_QUEUE for macvtap[1] :)
>> > 
>> > [1] https://lkml.org/lkml/2015/8/24/147
> It's the same concern - that we aren't fully addressing
> the problem, so if user configures a qdisc, we are back to square 1.
> It's especially annoying that IIUC in this setup, if one
> does configured a non default qdisc, there's no way to go back.
> It doesn't necessarily mean we must not do it as an intermediate step,
> though.
>
>> > 
>> > I think this could be done by management or more safe by introducing a
>> > new tun flag (TUN_NO_QUEUE).
> What exactly does this flag do/mean?

It means we don't need qdisc for this tuntap, so we can set IFF_NO_QUEUE
flag.
Michael S. Tsirkin April 14, 2016, 9:10 a.m. UTC | #6
On Thu, Apr 14, 2016 at 05:07:50PM +0800, Jason Wang wrote:
> 
> 
> On 04/14/2016 05:05 PM, Michael S. Tsirkin wrote:
> > On Thu, Apr 14, 2016 at 02:49:28PM +0800, Jason Wang wrote:
> >> > 
> >> > 
> >> > On 04/13/2016 06:26 PM, Michael S. Tsirkin wrote:
> >>> > > On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
> >>>> > >> This patch disables the default qdisc by explicitly setting the
> >>>> > >> IFF_NO_QUEUE private flag so that now the tun xmit path do not
> >>>> > >> require any lock by default.
> >>>> > >>
> >>>> > >> The default qdisc was first removed as a side effect of commit
> >>>> > >> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
> >>>> > >> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
> >>>> > >>
> >>>> > >> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> >>> > > I wonder about this back and forth.
> >>> > > Jason, do you see a workload where the default qdisc
> >>> > > is preferable?
> >> > 
> >> > I don't know, but we used to behave like this so we'd better keep it.
> >> > 
> >> > An interesting thing is I vaguely remember that you have some concern
> >> > when I propose IFF_NO_QUEUE for macvtap[1] :)
> >> > 
> >> > [1] https://lkml.org/lkml/2015/8/24/147
> > It's the same concern - that we aren't fully addressing
> > the problem, so if user configures a qdisc, we are back to square 1.
> > It's especially annoying that IIUC in this setup, if one
> > does configured a non default qdisc, there's no way to go back.
> > It doesn't necessarily mean we must not do it as an intermediate step,
> > though.
> >
> >> > 
> >> > I think this could be done by management or more safe by introducing a
> >> > new tun flag (TUN_NO_QUEUE).
> > What exactly does this flag do/mean?
> 
> It means we don't need qdisc for this tuntap, so we can set IFF_NO_QUEUE
> flag.

But what does it mean for the user? When to set it and when not to set
it?
Jason Wang April 14, 2016, 9:21 a.m. UTC | #7
On 04/14/2016 05:10 PM, Michael S. Tsirkin wrote:
> On Thu, Apr 14, 2016 at 05:07:50PM +0800, Jason Wang wrote:
>>
>> On 04/14/2016 05:05 PM, Michael S. Tsirkin wrote:
>>> On Thu, Apr 14, 2016 at 02:49:28PM +0800, Jason Wang wrote:
>>>>>
>>>>> On 04/13/2016 06:26 PM, Michael S. Tsirkin wrote:
>>>>>>> On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
>>>>>>>>> This patch disables the default qdisc by explicitly setting the
>>>>>>>>> IFF_NO_QUEUE private flag so that now the tun xmit path do not
>>>>>>>>> require any lock by default.
>>>>>>>>>
>>>>>>>>> The default qdisc was first removed as a side effect of commit
>>>>>>>>> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
>>>>>>>>> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
>>>>>>>>>
>>>>>>>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>>>>>> I wonder about this back and forth.
>>>>>>> Jason, do you see a workload where the default qdisc
>>>>>>> is preferable?
>>>>> I don't know, but we used to behave like this so we'd better keep it.
>>>>>
>>>>> An interesting thing is I vaguely remember that you have some concern
>>>>> when I propose IFF_NO_QUEUE for macvtap[1] :)
>>>>>
>>>>> [1] https://lkml.org/lkml/2015/8/24/147
>>> It's the same concern - that we aren't fully addressing
>>> the problem, so if user configures a qdisc, we are back to square 1.
>>> It's especially annoying that IIUC in this setup, if one
>>> does configured a non default qdisc, there's no way to go back.
>>> It doesn't necessarily mean we must not do it as an intermediate step,
>>> though.
>>>
>>>>> I think this could be done by management or more safe by introducing a
>>>>> new tun flag (TUN_NO_QUEUE).
>>> What exactly does this flag do/mean?
>> It means we don't need qdisc for this tuntap, so we can set IFF_NO_QUEUE
>> flag.
> But what does it mean for the user? When to set it and when not to set
> it?

It was used for user that don't want qdisc (e.g for the user that only
cares about performance).
Michael S. Tsirkin April 14, 2016, 10:01 a.m. UTC | #8
On Thu, Apr 14, 2016 at 05:21:40PM +0800, Jason Wang wrote:
> 
> 
> On 04/14/2016 05:10 PM, Michael S. Tsirkin wrote:
> > On Thu, Apr 14, 2016 at 05:07:50PM +0800, Jason Wang wrote:
> >>
> >> On 04/14/2016 05:05 PM, Michael S. Tsirkin wrote:
> >>> On Thu, Apr 14, 2016 at 02:49:28PM +0800, Jason Wang wrote:
> >>>>>
> >>>>> On 04/13/2016 06:26 PM, Michael S. Tsirkin wrote:
> >>>>>>> On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
> >>>>>>>>> This patch disables the default qdisc by explicitly setting the
> >>>>>>>>> IFF_NO_QUEUE private flag so that now the tun xmit path do not
> >>>>>>>>> require any lock by default.
> >>>>>>>>>
> >>>>>>>>> The default qdisc was first removed as a side effect of commit
> >>>>>>>>> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
> >>>>>>>>> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> >>>>>>> I wonder about this back and forth.
> >>>>>>> Jason, do you see a workload where the default qdisc
> >>>>>>> is preferable?
> >>>>> I don't know, but we used to behave like this so we'd better keep it.
> >>>>>
> >>>>> An interesting thing is I vaguely remember that you have some concern
> >>>>> when I propose IFF_NO_QUEUE for macvtap[1] :)
> >>>>>
> >>>>> [1] https://lkml.org/lkml/2015/8/24/147
> >>> It's the same concern - that we aren't fully addressing
> >>> the problem, so if user configures a qdisc, we are back to square 1.
> >>> It's especially annoying that IIUC in this setup, if one
> >>> does configured a non default qdisc, there's no way to go back.
> >>> It doesn't necessarily mean we must not do it as an intermediate step,
> >>> though.
> >>>
> >>>>> I think this could be done by management or more safe by introducing a
> >>>>> new tun flag (TUN_NO_QUEUE).
> >>> What exactly does this flag do/mean?
> >> It means we don't need qdisc for this tuntap, so we can set IFF_NO_QUEUE
> >> flag.
> > But what does it mean for the user? When to set it and when not to set
> > it?
> 
> It was used for user that don't want qdisc (e.g for the user that only
> cares about performance).

However
- for tun, how is a fifo qdisc functionally different?
- it doesn't prevent configuring a qdisc, does it?
Hannes Frederic Sowa April 14, 2016, 10:09 a.m. UTC | #9
On 14.04.2016 12:01, Michael S. Tsirkin wrote:
> On Thu, Apr 14, 2016 at 05:21:40PM +0800, Jason Wang wrote:
>>
>>
>> On 04/14/2016 05:10 PM, Michael S. Tsirkin wrote:
>>> On Thu, Apr 14, 2016 at 05:07:50PM +0800, Jason Wang wrote:
>>>>
>>>> On 04/14/2016 05:05 PM, Michael S. Tsirkin wrote:
>>>>> On Thu, Apr 14, 2016 at 02:49:28PM +0800, Jason Wang wrote:
>>>>>>>
>>>>>>> On 04/13/2016 06:26 PM, Michael S. Tsirkin wrote:
>>>>>>>>> On Wed, Apr 13, 2016 at 11:04:47AM +0200, Paolo Abeni wrote:
>>>>>>>>>>> This patch disables the default qdisc by explicitly setting the
>>>>>>>>>>> IFF_NO_QUEUE private flag so that now the tun xmit path do not
>>>>>>>>>>> require any lock by default.
>>>>>>>>>>>
>>>>>>>>>>> The default qdisc was first removed as a side effect of commit
>>>>>>>>>>> f84bb1eac027 ("net: fix IFF_NO_QUEUE for drivers using alloc_netdev")
>>>>>>>>>>> and recently restored with commit 016adb7260f4 ("tuntap: restore default qdisc")
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>>>>>>>> I wonder about this back and forth.
>>>>>>>>> Jason, do you see a workload where the default qdisc
>>>>>>>>> is preferable?
>>>>>>> I don't know, but we used to behave like this so we'd better keep it.
>>>>>>>
>>>>>>> An interesting thing is I vaguely remember that you have some concern
>>>>>>> when I propose IFF_NO_QUEUE for macvtap[1] :)
>>>>>>>
>>>>>>> [1] https://lkml.org/lkml/2015/8/24/147
>>>>> It's the same concern - that we aren't fully addressing
>>>>> the problem, so if user configures a qdisc, we are back to square 1.
>>>>> It's especially annoying that IIUC in this setup, if one
>>>>> does configured a non default qdisc, there's no way to go back.
>>>>> It doesn't necessarily mean we must not do it as an intermediate step,
>>>>> though.
>>>>>
>>>>>>> I think this could be done by management or more safe by introducing a
>>>>>>> new tun flag (TUN_NO_QUEUE).
>>>>> What exactly does this flag do/mean?
>>>> It means we don't need qdisc for this tuntap, so we can set IFF_NO_QUEUE
>>>> flag.
>>> But what does it mean for the user? When to set it and when not to set
>>> it?
>>
>> It was used for user that don't want qdisc (e.g for the user that only
>> cares about performance).
>
> However
> - for tun, how is a fifo qdisc functionally different?

pfifo wouldn't matter match, but the pfifo_qdisc classifies packets into 
different bands and does alter the way ip packets are transmitted 
outside. With the LLTX change I don't see how in any case a queue should 
build up, so I don't really see this as a problem.

The only reason why this change could do harm is scripting and API 
compatibility, e.g. tools expect after creation a qdisc to be present to 
modify its behavior.

> - it doesn't prevent configuring a qdisc, does it?

Correct, you can still add qdiscs later on and switch back to no_queue, 
also. We are just talking about changing the default.

Bye,
Hannes
diff mbox

Patch

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 42992dc..0a0b63c 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1081,6 +1081,7 @@  static void tun_net_init(struct net_device *dev)
 
 		break;
 	}
+	dev->priv_flags |= IFF_NO_QUEUE;
 }
 
 /* Character device part */