| Message ID | 1430156304-13187-2-git-send-email-anuradhak@cumulusnetworks.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Apr 27, 2015 at 10:38 AM, <anuradhak@cumulusnetworks.com> wrote: > From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> > > This patch introduces an IFF_PROTO_DOWN flag that can be used by > user space applications to notify drivers that errors have been > detected on the device. > > Signed-off-by: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> > Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com> > Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> > Signed-off-by: Wilson Kok <wkok@cumulusnetworks.com> > --- > include/uapi/linux/if.h | 4 ++++ > net/8021q/vlan_dev.c | 3 ++- > net/core/dev.c | 2 +- > 3 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h > index 9cf2394..e263bd2 100644 > --- a/include/uapi/linux/if.h > +++ b/include/uapi/linux/if.h > @@ -66,6 +66,8 @@ > * @IFF_LOWER_UP: driver signals L1 up. Volatile. > * @IFF_DORMANT: driver signals dormant. Volatile. > * @IFF_ECHO: echo sent packets. Volatile. > + * @IFF_PROTO_DOWN: protocol is down on the interface. Can be toggled > + * through sysfs. This comment is stale, right? I didn't see any sysfs support in the patchset. -- 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 Mon, Apr 27, 2015 at 11:05 PM, Scott Feldman <sfeldma@gmail.com> wrote: > On Mon, Apr 27, 2015 at 10:38 AM, <anuradhak@cumulusnetworks.com> wrote: >> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> >> >> This patch introduces an IFF_PROTO_DOWN flag that can be used by >> user space applications to notify drivers that errors have been >> detected on the device. >> >> Signed-off-by: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> >> Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com> >> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> >> Signed-off-by: Wilson Kok <wkok@cumulusnetworks.com> >> --- >> include/uapi/linux/if.h | 4 ++++ >> net/8021q/vlan_dev.c | 3 ++- >> net/core/dev.c | 2 +- >> 3 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h >> index 9cf2394..e263bd2 100644 >> --- a/include/uapi/linux/if.h >> +++ b/include/uapi/linux/if.h >> @@ -66,6 +66,8 @@ >> * @IFF_LOWER_UP: driver signals L1 up. Volatile. >> * @IFF_DORMANT: driver signals dormant. Volatile. >> * @IFF_ECHO: echo sent packets. Volatile. >> + * @IFF_PROTO_DOWN: protocol is down on the interface. Can be toggled >> + * through sysfs. > > This comment is stale, right? I didn't see any sysfs support in the patchset. Correct, No new sysfs has been added for this flag. But IFF_PROTO_DOWN can be toggled via the already existing generic flags sysfs (similar to say IFF_MULTICAST); hence the comment – root@net-next:~# echo 0x81003 > /sys/class/net/eth0/flags root@net-next:~# cat /sys/class/net/eth0/flags 0x81003 -- 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 Mon, 27 Apr 2015 10:38:21 -0700 anuradhak@cumulusnetworks.com wrote: > From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> > > This patch introduces an IFF_PROTO_DOWN flag that can be used by > user space applications to notify drivers that errors have been > detected on the device. > > Signed-off-by: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> > Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com> > Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> > Signed-off-by: Wilson Kok <wkok@cumulusnetworks.com> I worry that adding another bit to an already complex state API will break userspace. There are lots of things besides iproute2 which look at those flags including routing daemons (quagga), network manager, netplugd, and switch controllers. -- 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 Wed, Apr 29, 2015 at 3:13 PM, Stephen Hemminger <stephen@networkplumber.org> wrote: > On Mon, 27 Apr 2015 10:38:21 -0700 > anuradhak@cumulusnetworks.com wrote: > >> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> >> >> This patch introduces an IFF_PROTO_DOWN flag that can be used by >> user space applications to notify drivers that errors have been >> detected on the device. >> >> Signed-off-by: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> >> Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com> >> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> >> Signed-off-by: Wilson Kok <wkok@cumulusnetworks.com> > > I worry that adding another bit to an already complex state API > will break userspace. > > There are lots of things besides iproute2 which look at those > flags including routing daemons (quagga), network manager, netplugd, > and switch controllers. Yes, I understand your concerns here. And tried to work around introducing a separate error flag by clearing IFF_UP on proto_down/detecting errors (as Scott also brought up earlier). That implementation failed because of the following reasons - 1. There is no way to disambiguate between admin_down (!IFF_UP) and an APP/driver enforced error_down (IFF_PROTO_DOWN). Administrator or automation-scripts that monitor the config assumed that switch-port configuration had somehow fallen out of sync (and attempted to reinstate the admin_up repeatedly). 2. Automatic error recovery was not possible; consider the following scenario for e.g. a. The MLAG peer-link is down so the MLAG app on the secondary switch has proto_down’ed all the MLAG ports (including switch-port swp1) by clearing IFF_UP. b. At the same time the administrator is in the process of making some changes on the network connected to swp1. To avoid doing it live he would admin_disable swp1 (!IFF_UP) by doing an "ip link set swp1 down" (this is a no-op as event #a has already cleared IFF_UP on swp1). c. If the MLAG peer-link recovers at this point the MLAG app on the secondary switch would try to automatically recover the MLAG ports by clearing proto_down (i.e. setting IFF_UP); including on swp1. Doing that overrides the administrator’s directive to keep swp1 admin_down. Overriding an admin-down in a live network can be very dangerous so it is not possible to do auto-error-recovery unless we have a way to disambiguate between the admin and error states. -- 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 Wed, Apr 29, 2015 at 4:07 PM, Anuradha Karuppiah <anuradhak@cumulusnetworks.com> wrote: > On Wed, Apr 29, 2015 at 3:13 PM, Stephen Hemminger > <stephen@networkplumber.org> wrote: >> On Mon, 27 Apr 2015 10:38:21 -0700 >> anuradhak@cumulusnetworks.com wrote: >> >>> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> >>> >>> This patch introduces an IFF_PROTO_DOWN flag that can be used by >>> user space applications to notify drivers that errors have been >>> detected on the device. >>> >>> Signed-off-by: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> >>> Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com> >>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> >>> Signed-off-by: Wilson Kok <wkok@cumulusnetworks.com> >> >> I worry that adding another bit to an already complex state API >> will break userspace. >> >> There are lots of things besides iproute2 which look at those >> flags including routing daemons (quagga), network manager, netplugd, >> and switch controllers. > > Yes, I understand your concerns here. And tried to work around introducing > a separate error flag by clearing IFF_UP on proto_down/detecting errors (as > Scott also brought up earlier). > > That implementation failed because of the following reasons - > 1. There is no way to disambiguate between admin_down (!IFF_UP) and an > APP/driver enforced error_down (IFF_PROTO_DOWN). Administrator or > automation-scripts that monitor the config assumed that switch-port > configuration had somehow fallen out of sync (and attempted to reinstate the > admin_up repeatedly). > > 2. Automatic error recovery was not possible; consider the following scenario > for e.g. > a. The MLAG peer-link is down so the MLAG app on the secondary switch has > proto_down’ed all the MLAG ports (including switch-port swp1) by clearing > IFF_UP. > b. At the same time the administrator is in the process of making some > changes on the network connected to swp1. To avoid doing it live he would > admin_disable swp1 (!IFF_UP) by doing an "ip link set swp1 down" (this > is a no-op as event #a has already cleared IFF_UP on swp1). > c. If the MLAG peer-link recovers at this point the MLAG app on the > secondary switch would try to automatically recover the MLAG ports > by clearing proto_down (i.e. setting IFF_UP); including on swp1. Doing > that overrides the administrator’s directive to keep swp1 admin_down. > Overriding an admin-down in a live network can be very dangerous so it > is not possible to do auto-error-recovery unless we have a way to > disambiguate between the admin and error states. I have the need to disambiguate the error state but it doesn't have to be an IFF_X attribute. Stephen, Do you think it would be more easily consumable if it were a new/separate net_device attribute instead of being a new bit in "&struct net_device flags"? -- 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 Wed, 29 Apr 2015 16:25:07 -0700 Anuradha Karuppiah <anuradhak@cumulusnetworks.com> wrote: > On Wed, Apr 29, 2015 at 4:07 PM, Anuradha Karuppiah > <anuradhak@cumulusnetworks.com> wrote: > > On Wed, Apr 29, 2015 at 3:13 PM, Stephen Hemminger > > <stephen@networkplumber.org> wrote: > >> On Mon, 27 Apr 2015 10:38:21 -0700 > >> anuradhak@cumulusnetworks.com wrote: > >> > >>> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> > >>> > >>> This patch introduces an IFF_PROTO_DOWN flag that can be used by > >>> user space applications to notify drivers that errors have been > >>> detected on the device. > >>> > >>> Signed-off-by: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> > >>> Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com> > >>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> > >>> Signed-off-by: Wilson Kok <wkok@cumulusnetworks.com> > >> > >> I worry that adding another bit to an already complex state API > >> will break userspace. > >> > >> There are lots of things besides iproute2 which look at those > >> flags including routing daemons (quagga), network manager, netplugd, > >> and switch controllers. > > > > Yes, I understand your concerns here. And tried to work around introducing > > a separate error flag by clearing IFF_UP on proto_down/detecting errors (as > > Scott also brought up earlier). > > > > That implementation failed because of the following reasons - > > 1. There is no way to disambiguate between admin_down (!IFF_UP) and an > > APP/driver enforced error_down (IFF_PROTO_DOWN). Administrator or > > automation-scripts that monitor the config assumed that switch-port > > configuration had somehow fallen out of sync (and attempted to reinstate the > > admin_up repeatedly). > > > > 2. Automatic error recovery was not possible; consider the following scenario > > for e.g. > > a. The MLAG peer-link is down so the MLAG app on the secondary switch has > > proto_down’ed all the MLAG ports (including switch-port swp1) by clearing > > IFF_UP. > > b. At the same time the administrator is in the process of making some > > changes on the network connected to swp1. To avoid doing it live he would > > admin_disable swp1 (!IFF_UP) by doing an "ip link set swp1 down" (this > > is a no-op as event #a has already cleared IFF_UP on swp1). > > c. If the MLAG peer-link recovers at this point the MLAG app on the > > secondary switch would try to automatically recover the MLAG ports > > by clearing proto_down (i.e. setting IFF_UP); including on swp1. Doing > > that overrides the administrator’s directive to keep swp1 admin_down. > > Overriding an admin-down in a live network can be very dangerous so it > > is not possible to do auto-error-recovery unless we have a way to > > disambiguate between the admin and error states. > > I have the need to disambiguate the error state but it doesn't have to be an > IFF_X attribute. Stephen, Do you think it would be more easily consumable if > it were a new/separate net_device attribute instead of being a new bit in > "&struct net_device flags"? You need to separate. 1. Flags visible by existing user applications. These really can't change. 2. Flags required for special management applications. These are best handled by additional netlink nested attributes 3. Internal kernel flags. As long as they are not visible to userspace nothing should care. -- 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 Wed, Apr 29, 2015 at 4:33 PM, Stephen Hemminger <stephen@networkplumber.org> wrote: > On Wed, 29 Apr 2015 16:25:07 -0700 > Anuradha Karuppiah <anuradhak@cumulusnetworks.com> wrote: > >> On Wed, Apr 29, 2015 at 4:07 PM, Anuradha Karuppiah >> <anuradhak@cumulusnetworks.com> wrote: >> > On Wed, Apr 29, 2015 at 3:13 PM, Stephen Hemminger >> > <stephen@networkplumber.org> wrote: >> >> On Mon, 27 Apr 2015 10:38:21 -0700 >> >> anuradhak@cumulusnetworks.com wrote: >> >> >> >>> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> >> >>> >> >>> This patch introduces an IFF_PROTO_DOWN flag that can be used by >> >>> user space applications to notify drivers that errors have been >> >>> detected on the device. >> >>> >> >>> Signed-off-by: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> >> >>> Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com> >> >>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> >> >>> Signed-off-by: Wilson Kok <wkok@cumulusnetworks.com> >> >> >> >> I worry that adding another bit to an already complex state API >> >> will break userspace. >> >> >> >> There are lots of things besides iproute2 which look at those >> >> flags including routing daemons (quagga), network manager, netplugd, >> >> and switch controllers. >> > >> > Yes, I understand your concerns here. And tried to work around introducing >> > a separate error flag by clearing IFF_UP on proto_down/detecting errors (as >> > Scott also brought up earlier). >> > >> > That implementation failed because of the following reasons - >> > 1. There is no way to disambiguate between admin_down (!IFF_UP) and an >> > APP/driver enforced error_down (IFF_PROTO_DOWN). Administrator or >> > automation-scripts that monitor the config assumed that switch-port >> > configuration had somehow fallen out of sync (and attempted to reinstate the >> > admin_up repeatedly). >> > >> > 2. Automatic error recovery was not possible; consider the following scenario >> > for e.g. >> > a. The MLAG peer-link is down so the MLAG app on the secondary switch has >> > proto_down’ed all the MLAG ports (including switch-port swp1) by clearing >> > IFF_UP. >> > b. At the same time the administrator is in the process of making some >> > changes on the network connected to swp1. To avoid doing it live he would >> > admin_disable swp1 (!IFF_UP) by doing an "ip link set swp1 down" (this >> > is a no-op as event #a has already cleared IFF_UP on swp1). >> > c. If the MLAG peer-link recovers at this point the MLAG app on the >> > secondary switch would try to automatically recover the MLAG ports >> > by clearing proto_down (i.e. setting IFF_UP); including on swp1. Doing >> > that overrides the administrator’s directive to keep swp1 admin_down. >> > Overriding an admin-down in a live network can be very dangerous so it >> > is not possible to do auto-error-recovery unless we have a way to >> > disambiguate between the admin and error states. >> >> I have the need to disambiguate the error state but it doesn't have to be an >> IFF_X attribute. Stephen, Do you think it would be more easily consumable if >> it were a new/separate net_device attribute instead of being a new bit in >> "&struct net_device flags"? > > You need to separate. > 1. Flags visible by existing user applications. These really can't change. > 2. Flags required for special management applications. These are best handled > by additional netlink nested attributes > 3. Internal kernel flags. As long as they are not visible to userspace > nothing should care. Ack. I will move IFF_PROTO_DOWN out of "&struct net_device flags". PROTO_DOWN needs to be configurable/visible only to new APPs so I will add a separate "&struct net_device protodown" field and the corresponding netlink attribute. -- 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/include/uapi/linux/if.h b/include/uapi/linux/if.h index 9cf2394..e263bd2 100644 --- a/include/uapi/linux/if.h +++ b/include/uapi/linux/if.h @@ -66,6 +66,8 @@ * @IFF_LOWER_UP: driver signals L1 up. Volatile. * @IFF_DORMANT: driver signals dormant. Volatile. * @IFF_ECHO: echo sent packets. Volatile. + * @IFF_PROTO_DOWN: protocol is down on the interface. Can be toggled + * through sysfs. */ enum net_device_flags { IFF_UP = 1<<0, /* sysfs */ @@ -87,6 +89,7 @@ enum net_device_flags { IFF_LOWER_UP = 1<<16, /* volatile */ IFF_DORMANT = 1<<17, /* volatile */ IFF_ECHO = 1<<18, /* volatile */ + IFF_PROTO_DOWN = 1<<19, /* sysfs */ }; #define IFF_UP IFF_UP @@ -108,6 +111,7 @@ enum net_device_flags { #define IFF_LOWER_UP IFF_LOWER_UP #define IFF_DORMANT IFF_DORMANT #define IFF_ECHO IFF_ECHO +#define IFF_PROTO_DOWN IFF_PROTO_DOWN #define IFF_VOLATILE (IFF_LOOPBACK|IFF_POINTOPOINT|IFF_BROADCAST|IFF_ECHO|\ IFF_MASTER|IFF_SLAVE|IFF_RUNNING|IFF_LOWER_UP|IFF_DORMANT) diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c index 01d7ba8..7073ee1 100644 --- a/net/8021q/vlan_dev.c +++ b/net/8021q/vlan_dev.c @@ -537,7 +537,8 @@ static int vlan_dev_init(struct net_device *dev) /* IFF_BROADCAST|IFF_MULTICAST; ??? */ dev->flags = real_dev->flags & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI | - IFF_MASTER | IFF_SLAVE); + IFF_MASTER | IFF_SLAVE | + IFF_PROTO_DOWN); dev->state = (real_dev->state & ((1<<__LINK_STATE_NOCARRIER) | (1<<__LINK_STATE_DORMANT))) | (1<<__LINK_STATE_PRESENT); diff --git a/net/core/dev.c b/net/core/dev.c index af4a1b0..e9600fa 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -5725,7 +5725,7 @@ int __dev_change_flags(struct net_device *dev, unsigned int flags) dev->flags = (flags & (IFF_DEBUG | IFF_NOTRAILERS | IFF_NOARP | IFF_DYNAMIC | IFF_MULTICAST | IFF_PORTSEL | - IFF_AUTOMEDIA)) | + IFF_AUTOMEDIA | IFF_PROTO_DOWN)) | (dev->flags & (IFF_UP | IFF_VOLATILE | IFF_PROMISC | IFF_ALLMULTI));