| Message ID | 1a216fb6cade7dcd7d2c1d4ce113a1ee6e3fdeb6.1460393493.git.pabeni@redhat.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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.
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
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
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.
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?
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).
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?
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 --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 */
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(+)