Message ID | 5578B318.1050808@cumulusnetworks.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On Wed, Jun 10, 2015 at 2:58 PM, roopa <roopa@cumulusnetworks.com> wrote: > On 6/10/15, 1:43 PM, Scott Feldman wrote: >> >> I'm getting this dump_stack when reloading rocker driver. Did some >> sysctl MPLS nodes not get cleaned up on NETDEV_UNREGISTER? >> >> Steps to repro: load rocker (on system) with rocker device, rmmod >> rocker, and then modprobe rocker. I doubt this is specific to rocker: >> and re-registration of a netdev should hit it. I am using UDEV rules >> to rename kernel's ethX to a different name. Maybe that's what >> tripped it up? >> > On a quick look, wondering if this is because mpls driver does not seem to > do a unregister and re-register sysctl > on device name change. > > diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c > index 7b3f732..ec21a5d 100644 > --- a/net/mpls/af_mpls.c > +++ b/net/mpls/af_mpls.c > @@ -564,6 +564,14 @@ static int mpls_dev_notify(struct notifier_block *this, > unsigned long event, > case NETDEV_UNREGISTER: > mpls_ifdown(dev); > break; > + case NETDEV_CHANGENAME: > + mpls_ifdown(dev); > + if ((dev->type == ARPHRD_ETHER) || > + (dev->type == ARPHRD_LOOPBACK)) { > + mdev = mpls_add_dev(dev); > + if (IS_ERR(mdev)) > + return notifier_from_errno(PTR_ERR(mdev)); > + } > } > return NOTIFY_OK; > } Roopa, I tested this patch and problem goes away. (It's missing a break statement, BTW). I didn't look into the correctness of the patch, but at first glance it seems liek the right thing to do. Maybe breaking out the renaming portions into sub-functions could keep the work done in NETDEV_CHANGENAME to a minimum. Are you sending official fix? -scott -- 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 11/06/15 00:23, Scott Feldman wrote: > On Wed, Jun 10, 2015 at 2:58 PM, roopa <roopa@cumulusnetworks.com> wrote: >> On 6/10/15, 1:43 PM, Scott Feldman wrote: >>> >>> I'm getting this dump_stack when reloading rocker driver. Did some >>> sysctl MPLS nodes not get cleaned up on NETDEV_UNREGISTER? >>> >>> Steps to repro: load rocker (on system) with rocker device, rmmod >>> rocker, and then modprobe rocker. I doubt this is specific to rocker: >>> and re-registration of a netdev should hit it. I am using UDEV rules >>> to rename kernel's ethX to a different name. Maybe that's what >>> tripped it up? >>> >> On a quick look, wondering if this is because mpls driver does not seem to >> do a unregister and re-register sysctl >> on device name change. Mea culpa. Thanks for looking at this. >> >> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c >> index 7b3f732..ec21a5d 100644 >> --- a/net/mpls/af_mpls.c >> +++ b/net/mpls/af_mpls.c >> @@ -564,6 +564,14 @@ static int mpls_dev_notify(struct notifier_block *this, >> unsigned long event, >> case NETDEV_UNREGISTER: >> mpls_ifdown(dev); >> break; >> + case NETDEV_CHANGENAME: >> + mpls_ifdown(dev); >> + if ((dev->type == ARPHRD_ETHER) || >> + (dev->type == ARPHRD_LOOPBACK)) { >> + mdev = mpls_add_dev(dev); >> + if (IS_ERR(mdev)) >> + return notifier_from_errno(PTR_ERR(mdev)); >> + } >> } >> return NOTIFY_OK; >> } > > Roopa, I tested this patch and problem goes away. (It's missing a > break statement, BTW). I didn't look into the correctness of the > patch, but at first glance it seems liek the right thing to do. Maybe > breaking out the renaming portions into sub-functions could keep the > work done in NETDEV_CHANGENAME to a minimum. I agree that breaking out the sysctl registration/unregistration is a good idea to not have to do more work than is necessary, and to avoid unintended consequences (like routes using the interface being made unusable). > > Are you sending official fix? Roopa, let me know if you'd like me to carry this forward. Thanks, Rob -- 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 6/11/15, 5:55 AM, Robert Shearman wrote: > On 11/06/15 00:23, Scott Feldman wrote: >> On Wed, Jun 10, 2015 at 2:58 PM, roopa <roopa@cumulusnetworks.com> >> wrote: >>> On 6/10/15, 1:43 PM, Scott Feldman wrote: >>>> >>>> I'm getting this dump_stack when reloading rocker driver. Did some >>>> sysctl MPLS nodes not get cleaned up on NETDEV_UNREGISTER? >>>> >>>> Steps to repro: load rocker (on system) with rocker device, rmmod >>>> rocker, and then modprobe rocker. I doubt this is specific to rocker: >>>> and re-registration of a netdev should hit it. I am using UDEV rules >>>> to rename kernel's ethX to a different name. Maybe that's what >>>> tripped it up? >>>> >>> On a quick look, wondering if this is because mpls driver does not >>> seem to >>> do a unregister and re-register sysctl >>> on device name change. > > Mea culpa. Thanks for looking at this. > >>> >>> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c >>> index 7b3f732..ec21a5d 100644 >>> --- a/net/mpls/af_mpls.c >>> +++ b/net/mpls/af_mpls.c >>> @@ -564,6 +564,14 @@ static int mpls_dev_notify(struct >>> notifier_block *this, >>> unsigned long event, >>> case NETDEV_UNREGISTER: >>> mpls_ifdown(dev); >>> break; >>> + case NETDEV_CHANGENAME: >>> + mpls_ifdown(dev); >>> + if ((dev->type == ARPHRD_ETHER) || >>> + (dev->type == ARPHRD_LOOPBACK)) { >>> + mdev = mpls_add_dev(dev); >>> + if (IS_ERR(mdev)) >>> + return >>> notifier_from_errno(PTR_ERR(mdev)); >>> + } >>> } >>> return NOTIFY_OK; >>> } >> >> Roopa, I tested this patch and problem goes away. (It's missing a >> break statement, BTW). I didn't look into the correctness of the >> patch, but at first glance it seems liek the right thing to do. Maybe >> breaking out the renaming portions into sub-functions could keep the >> work done in NETDEV_CHANGENAME to a minimum. > > I agree that breaking out the sysctl registration/unregistration is a > good idea to not have to do more work than is necessary, and to avoid > unintended consequences (like routes using the interface being made > unusable). > >> >> Are you sending official fix? > > Roopa, let me know if you'd like me to carry this forward. sorry for the delay in getting back. Its going to be a busy day for me. And i don't know the side-effects of my changes yet. you probably have a better handle on this. So if you can, yes please carry this forward. thanks!. -- 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
Robert Shearman <rshearma@brocade.com> writes: > On 11/06/15 00:23, Scott Feldman wrote: >> On Wed, Jun 10, 2015 at 2:58 PM, roopa <roopa@cumulusnetworks.com> wrote: >>> On 6/10/15, 1:43 PM, Scott Feldman wrote: >>>> >>>> I'm getting this dump_stack when reloading rocker driver. Did some >>>> sysctl MPLS nodes not get cleaned up on NETDEV_UNREGISTER? >>>> >>>> Steps to repro: load rocker (on system) with rocker device, rmmod >>>> rocker, and then modprobe rocker. I doubt this is specific to rocker: >>>> and re-registration of a netdev should hit it. I am using UDEV rules >>>> to rename kernel's ethX to a different name. Maybe that's what >>>> tripped it up? >>>> >>> On a quick look, wondering if this is because mpls driver does not seem to >>> do a unregister and re-register sysctl >>> on device name change. > > Mea culpa. Thanks for looking at this. > >>> >>> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c >>> index 7b3f732..ec21a5d 100644 >>> --- a/net/mpls/af_mpls.c >>> +++ b/net/mpls/af_mpls.c >>> @@ -564,6 +564,14 @@ static int mpls_dev_notify(struct notifier_block *this, >>> unsigned long event, >>> case NETDEV_UNREGISTER: >>> mpls_ifdown(dev); >>> break; >>> + case NETDEV_CHANGENAME: >>> + mpls_ifdown(dev); >>> + if ((dev->type == ARPHRD_ETHER) || >>> + (dev->type == ARPHRD_LOOPBACK)) { >>> + mdev = mpls_add_dev(dev); >>> + if (IS_ERR(mdev)) >>> + return notifier_from_errno(PTR_ERR(mdev)); >>> + } >>> } >>> return NOTIFY_OK; >>> } >> >> Roopa, I tested this patch and problem goes away. (It's missing a >> break statement, BTW). I didn't look into the correctness of the >> patch, but at first glance it seems liek the right thing to do. Maybe >> breaking out the renaming portions into sub-functions could keep the >> work done in NETDEV_CHANGENAME to a minimum. > > I agree that breaking out the sysctl registration/unregistration is a good idea > to not have to do more work than is necessary, and to avoid unintended > consequences (like routes using the interface being made unusable). Yes. It needs to be something like: + case NETDEV_CHANGENAME: + mdev = mpls_dev_get(dev); + if (mdev) { + mpls_dev_sysctl_unregister(mdev); + mpls_dev_sysctl_register(dev, mdev); + } + break; 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/net/mpls/af_mpls.c b/net/mpls/af_mpls.c index 7b3f732..ec21a5d 100644 --- a/net/mpls/af_mpls.c +++ b/net/mpls/af_mpls.c @@ -564,6 +564,14 @@ static int mpls_dev_notify(struct notifier_block *this, unsigned long event, case NETDEV_UNREGISTER: mpls_ifdown(dev); break; + case NETDEV_CHANGENAME: + mpls_ifdown(dev); + if ((dev->type == ARPHRD_ETHER) || + (dev->type == ARPHRD_LOOPBACK)) { + mdev = mpls_add_dev(dev); + if (IS_ERR(mdev)) + return notifier_from_errno(PTR_ERR(mdev)); + } } return NOTIFY_OK;