| Message ID | 20100913114315.GA859019@jupiter.n2.diac24.net |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com> My inclination is that the best way to handle this is to would be to push the device deletion for vlan and macvlan devices into the device core, where we could get the advantage of batched deletions. Right now vlan and macvlan devices are the only devices I know of that cause other devices to be removed during unregister, so removing that specialness seems reasonable. However not being able to move the primary vlan to a different network namespace is usability bug with no real alternatives. There are not any other patches on the table right now, and your patch below seems obviously correct. Let's get this merged before we forget about this, bug fix. David Lamparter <equinox@diac24.net> writes: > previously, if a vlan master device was moved from one network namespace > to another, all 802.1q and macvlan slaves were deleted. > > we can use dev->reg_state to figure out whether dev_change_net_namespace > is happening, since that won't set dev->reg_state NETREG_UNREGISTERING. > so, this changes 8021q and macvlan to ignore NETDEV_UNREGISTER when > reg_state is not NETREG_UNREGISTERING. > > Signed-off-by: David Lamparter <equinox@diac24.net> > -- > > On Tue, Aug 24, 2010 at 01:50:56PM +0200, David Lamparter wrote: >> attached patch makes it possible to keep macvlan / 802.1q slave devices >> on moving their master to a different namespace. as the opposite works >> without problems (moving the slaves into a different namespace), this >> shouldn't cause problems either. > > I would like to resurrect this. After the small excursion into > NETREG_NETNS_MOVING i've reevaluated my personal opinion and think the > behaviour is the desired one and this patch is the cleanest way to > implement it. While it depends on somewhat "fine print" on the reg_state > state machine, the probability of major screw-ups in this region is > rather small and it also quite likely makes a nice large boom when it is > unduly tampered with. > > > -David > > > drivers/net/macvlan.c | 4 ++++ > net/8021q/vlan.c | 4 ++++ > net/core/dev.c | 4 ++++ > 3 files changed, 12 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c > index f15fe2c..f43f8e4 100644 > --- a/drivers/net/macvlan.c > +++ b/drivers/net/macvlan.c > @@ -754,6 +754,10 @@ static int macvlan_device_event(struct notifier_block *unused, > } > break; > case NETDEV_UNREGISTER: > + /* twiddle thumbs on netns device moves */ > + if (dev->reg_state != NETREG_UNREGISTERING) > + break; > + > list_for_each_entry_safe(vlan, next, &port->vlans, list) > vlan->dev->rtnl_link_ops->dellink(vlan->dev, NULL); > break; > diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c > index 3c1c8c1..cdc37e8 100644 > --- a/net/8021q/vlan.c > +++ b/net/8021q/vlan.c > @@ -516,6 +516,10 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event, > break; > > case NETDEV_UNREGISTER: > + /* twiddle thumbs on netns device moves */ > + if (dev->reg_state != NETREG_UNREGISTERING) > + break; > + > /* Delete all VLANs for this dev. */ > grp->killall = 1; > > diff --git a/net/core/dev.c b/net/core/dev.c > index 1f466e8..b1589bc 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -5618,6 +5618,10 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char > > /* Notify protocols, that we are about to destroy > this device. They should clean all the things. > + > + Note that dev->reg_state stays at NETREG_REGISTERED. > + This is wanted because this way 8021q and macvlan know > + the device is just moving and can keep their slaves up. > */ > call_netdevice_notifiers(NETDEV_UNREGISTER, dev); > call_netdevice_notifiers(NETDEV_UNREGISTER_BATCH, dev); -- 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
On 09/13/2010 01:43 PM, David Lamparter wrote: > previously, if a vlan master device was moved from one network namespace > to another, all 802.1q and macvlan slaves were deleted. > > we can use dev->reg_state to figure out whether dev_change_net_namespace > is happening, since that won't set dev->reg_state NETREG_UNREGISTERING. > so, this changes 8021q and macvlan to ignore NETDEV_UNREGISTER when > reg_state is not NETREG_UNREGISTERING. > > Signed-off-by: David Lamparter<equinox@diac24.net> > Acked-by: Daniel Lezcano <daniel.lezcano@free.fr> -- 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
Am 15.09.2010 04:10, schrieb Eric W. Biederman: > > Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com> > > My inclination is that the best way to handle this is to would be to > push the device deletion for vlan and macvlan devices into the device > core, where we could get the advantage of batched deletions. We've added batched deletion to both about a year ago, what exactly is the problem? > Right > now vlan and macvlan devices are the only devices I know of that cause > other devices to be removed during unregister, so removing that > specialness seems reasonable. Actually all devices can cause this when used as a lower device by vlan or macvlan. Both vlan and macvlan are useless without a lower device, so I don't see why we shouldn't remove them when the lower device is unregistered. > However not being able to move the primary vlan to a different network > namespace is usability bug with no real alternatives. There are > not any other patches on the table right now, and your patch below > seems obviously correct. Let's get this merged before we forget about > this, bug fix. Looks reasonable to me. Acked-by: Patrick McHardy <kaber@trash.net> -- 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
Patrick McHardy <kaber@trash.net> writes: > Am 15.09.2010 04:10, schrieb Eric W. Biederman: >> >> Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com> >> >> My inclination is that the best way to handle this is to would be to >> push the device deletion for vlan and macvlan devices into the device >> core, where we could get the advantage of batched deletions. > > We've added batched deletion to both about a year ago, what exactly > is the problem? My problem is that while the both have dellink implemented we don't really batch their deletes as best we can. For macvlan when the underlying device is unregistered we delete each macvlan device one by one. For vlans we do batch the deletes, but not in the same batch as the underlying device. >> Right >> now vlan and macvlan devices are the only devices I know of that cause >> other devices to be removed during unregister, so removing that >> specialness seems reasonable. > > Actually all devices can cause this when used as a lower device by > vlan or macvlan. Both vlan and macvlan are useless without a lower > device, so I don't see why we shouldn't remove them when the lower > device is unregistered. I definitely think we should remove the devices when the lower device is destroyed/removed. However unregistered is a slightly different concept, and arguably macvlan and vlan devices are much more intimately connected to their underlying device than that. What I meant is we should implement the deletion differently. By doing something like having a list of all derived devices for a device, that we could walk and call dellink on from rollback_registered_many. In theory that could destroy a whole pile of macvlan and vlan devices with various different stackings one on the other all in the same batch. Eric -- 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
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c index f15fe2c..f43f8e4 100644 --- a/drivers/net/macvlan.c +++ b/drivers/net/macvlan.c @@ -754,6 +754,10 @@ static int macvlan_device_event(struct notifier_block *unused, } break; case NETDEV_UNREGISTER: + /* twiddle thumbs on netns device moves */ + if (dev->reg_state != NETREG_UNREGISTERING) + break; + list_for_each_entry_safe(vlan, next, &port->vlans, list) vlan->dev->rtnl_link_ops->dellink(vlan->dev, NULL); break; diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c index 3c1c8c1..cdc37e8 100644 --- a/net/8021q/vlan.c +++ b/net/8021q/vlan.c @@ -516,6 +516,10 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event, break; case NETDEV_UNREGISTER: + /* twiddle thumbs on netns device moves */ + if (dev->reg_state != NETREG_UNREGISTERING) + break; + /* Delete all VLANs for this dev. */ grp->killall = 1; diff --git a/net/core/dev.c b/net/core/dev.c index 1f466e8..b1589bc 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -5618,6 +5618,10 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char /* Notify protocols, that we are about to destroy this device. They should clean all the things. + + Note that dev->reg_state stays at NETREG_REGISTERED. + This is wanted because this way 8021q and macvlan know + the device is just moving and can keep their slaves up. */ call_netdevice_notifiers(NETDEV_UNREGISTER, dev); call_netdevice_notifiers(NETDEV_UNREGISTER_BATCH, dev);