Patchwork [net-next] net: call notifiers for mtu change even if iface is not up

login
register
mail settings
Submitter Jiri Pirko
Date Dec. 3, 2012, 11:16 a.m.
Message ID <1354533392-9308-1-git-send-email-jiri@resnulli.us>
Download mbox | patch
Permalink /patch/203319/
State Accepted
Delegated to: David Miller
Headers show

Comments

Jiri Pirko - Dec. 3, 2012, 11:16 a.m.
Do the same thing as in set mac. Call notifiers every time.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Neil Horman - Dec. 3, 2012, 2:18 p.m.
On Mon, Dec 03, 2012 at 12:16:32PM +0100, Jiri Pirko wrote:
> Do the same thing as in set mac. Call notifiers every time.
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>  net/core/dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 2f94df2..0685a72 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4971,7 +4971,7 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
>  	else
>  		dev->mtu = new_mtu;
>  
> -	if (!err && dev->flags & IFF_UP)
> +	if (!err)
>  		call_netdevice_notifiers(NETDEV_CHANGEMTU, dev);
>  	return err;
>  }

I'm not opposed to this change, but is there something that it expressly fixes?
While it doesn't hurt to send around mtu change events, one would presume that
listeners would pick up mtu changes when the NETDEV_UP event went' around.

Neil

--
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
Jiri Pirko - Dec. 3, 2012, 2:22 p.m.
Mon, Dec 03, 2012 at 03:18:23PM CET, nhorman@tuxdriver.com wrote:
>On Mon, Dec 03, 2012 at 12:16:32PM +0100, Jiri Pirko wrote:
>> Do the same thing as in set mac. Call notifiers every time.
>> 
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>>  net/core/dev.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 2f94df2..0685a72 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -4971,7 +4971,7 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
>>  	else
>>  		dev->mtu = new_mtu;
>>  
>> -	if (!err && dev->flags & IFF_UP)
>> +	if (!err)
>>  		call_netdevice_notifiers(NETDEV_CHANGEMTU, dev);
>>  	return err;
>>  }
>
>I'm not opposed to this change, but is there something that it expressly fixes?

This is about a consistency. To have the same behaviour as set_mac
for example.

>While it doesn't hurt to send around mtu change events, one would presume that
>listeners would pick up mtu changes when the NETDEV_UP event went' around.
>
>Neil
>
>--
>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
--
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
Neil Horman - Dec. 3, 2012, 3:22 p.m.
On Mon, Dec 03, 2012 at 03:22:29PM +0100, Jiri Pirko wrote:
> Mon, Dec 03, 2012 at 03:18:23PM CET, nhorman@tuxdriver.com wrote:
> >On Mon, Dec 03, 2012 at 12:16:32PM +0100, Jiri Pirko wrote:
> >> Do the same thing as in set mac. Call notifiers every time.
> >> 
> >> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> >> ---
> >>  net/core/dev.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index 2f94df2..0685a72 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -4971,7 +4971,7 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
> >>  	else
> >>  		dev->mtu = new_mtu;
> >>  
> >> -	if (!err && dev->flags & IFF_UP)
> >> +	if (!err)
> >>  		call_netdevice_notifiers(NETDEV_CHANGEMTU, dev);
> >>  	return err;
> >>  }
> >
> >I'm not opposed to this change, but is there something that it expressly fixes?
> 
> This is about a consistency. To have the same behaviour as set_mac
> for example.
> 
> >While it doesn't hurt to send around mtu change events, one would presume that
> >listeners would pick up mtu changes when the NETDEV_UP event went' around.
> >
> >Neil
> >
I figured, I just wanted to be sure that there wasn't a bug fix that needed
documenting in the changelog, or that listeners for this event didn't expect the
interface to already be up.  It looks pretty clean, with the exception of the
TIPC protocol.  It appears that it might require enable_bearer to be called
prior to any netdevice CHANGEMTU events, so that the eth_bearers array gets
filled out, and it looks like that has to get triggered either by user space, or
the receipt of a message over the network.  Let me know what you think

Neil

> >--
> >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
> --
> 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
> 
--
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
Jiri Pirko - Dec. 7, 2012, 12:29 p.m.
Mon, Dec 03, 2012 at 04:22:50PM CET, nhorman@tuxdriver.com wrote:
>On Mon, Dec 03, 2012 at 03:22:29PM +0100, Jiri Pirko wrote:
>> Mon, Dec 03, 2012 at 03:18:23PM CET, nhorman@tuxdriver.com wrote:
>> >On Mon, Dec 03, 2012 at 12:16:32PM +0100, Jiri Pirko wrote:
>> >> Do the same thing as in set mac. Call notifiers every time.
>> >> 
>> >> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> >> ---
>> >>  net/core/dev.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> 
>> >> diff --git a/net/core/dev.c b/net/core/dev.c
>> >> index 2f94df2..0685a72 100644
>> >> --- a/net/core/dev.c
>> >> +++ b/net/core/dev.c
>> >> @@ -4971,7 +4971,7 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
>> >>  	else
>> >>  		dev->mtu = new_mtu;
>> >>  
>> >> -	if (!err && dev->flags & IFF_UP)
>> >> +	if (!err)
>> >>  		call_netdevice_notifiers(NETDEV_CHANGEMTU, dev);
>> >>  	return err;
>> >>  }
>> >
>> >I'm not opposed to this change, but is there something that it expressly fixes?
>> 
>> This is about a consistency. To have the same behaviour as set_mac
>> for example.
>> 
>> >While it doesn't hurt to send around mtu change events, one would presume that
>> >listeners would pick up mtu changes when the NETDEV_UP event went' around.
>> >
>> >Neil
>> >
>I figured, I just wanted to be sure that there wasn't a bug fix that needed
>documenting in the changelog, or that listeners for this event didn't expect the
>interface to already be up.  It looks pretty clean, with the exception of the
>TIPC protocol.  It appears that it might require enable_bearer to be called
>prior to any netdevice CHANGEMTU events, so that the eth_bearers array gets
>filled out, and it looks like that has to get triggered either by user space, or
>the receipt of a message over the network.  Let me know what you think

I'm looking at the code. What is the difference between CHANGEMTU and
CHANGEADDR here? It looks to me that it should work for both in the same
way.


>
>Neil
>
>> >--
>> >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
>> --
>> 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
>> 
--
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
Neil Horman - Dec. 7, 2012, 3:07 p.m.
On Fri, Dec 07, 2012 at 01:29:20PM +0100, Jiri Pirko wrote:
> Mon, Dec 03, 2012 at 04:22:50PM CET, nhorman@tuxdriver.com wrote:
> >On Mon, Dec 03, 2012 at 03:22:29PM +0100, Jiri Pirko wrote:
> >> Mon, Dec 03, 2012 at 03:18:23PM CET, nhorman@tuxdriver.com wrote:
> >> >On Mon, Dec 03, 2012 at 12:16:32PM +0100, Jiri Pirko wrote:
> >> >> Do the same thing as in set mac. Call notifiers every time.
> >> >> 
> >> >> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> >> >> ---
> >> >>  net/core/dev.c | 2 +-
> >> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >> 
> >> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> >> index 2f94df2..0685a72 100644
> >> >> --- a/net/core/dev.c
> >> >> +++ b/net/core/dev.c
> >> >> @@ -4971,7 +4971,7 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
> >> >>  	else
> >> >>  		dev->mtu = new_mtu;
> >> >>  
> >> >> -	if (!err && dev->flags & IFF_UP)
> >> >> +	if (!err)
> >> >>  		call_netdevice_notifiers(NETDEV_CHANGEMTU, dev);
> >> >>  	return err;
> >> >>  }
> >> >
> >> >I'm not opposed to this change, but is there something that it expressly fixes?
> >> 
> >> This is about a consistency. To have the same behaviour as set_mac
> >> for example.
> >> 
> >> >While it doesn't hurt to send around mtu change events, one would presume that
> >> >listeners would pick up mtu changes when the NETDEV_UP event went' around.
> >> >
> >> >Neil
> >> >
> >I figured, I just wanted to be sure that there wasn't a bug fix that needed
> >documenting in the changelog, or that listeners for this event didn't expect the
> >interface to already be up.  It looks pretty clean, with the exception of the
> >TIPC protocol.  It appears that it might require enable_bearer to be called
> >prior to any netdevice CHANGEMTU events, so that the eth_bearers array gets
> >filled out, and it looks like that has to get triggered either by user space, or
> >the receipt of a message over the network.  Let me know what you think
> 
> I'm looking at the code. What is the difference between CHANGEMTU and
> CHANGEADDR here? It looks to me that it should work for both in the same
> way.
> 
Hmm, they do work in the same way, which likely means that tipc is probably
begging to oops there regardless of having your patch or not :)

I'll get around to fixing that shortly.  Since your patch then isn't going to
break anything that isn't already broken
Acked-by: Neil Horman <nhorman@tuxdriver.com>

> 
> >
> >Neil
> >
> >> >--
> >> >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
> >> --
> >> 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
> >> 
> 
--
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
David Miller - Dec. 7, 2012, 5:23 p.m.
From: Neil Horman <nhorman@tuxdriver.com>
Date: Fri, 7 Dec 2012 10:07:08 -0500

> On Fri, Dec 07, 2012 at 01:29:20PM +0100, Jiri Pirko wrote:
>> Mon, Dec 03, 2012 at 04:22:50PM CET, nhorman@tuxdriver.com wrote:
>> >On Mon, Dec 03, 2012 at 03:22:29PM +0100, Jiri Pirko wrote:
>> >> Mon, Dec 03, 2012 at 03:18:23PM CET, nhorman@tuxdriver.com wrote:
>> >> >On Mon, Dec 03, 2012 at 12:16:32PM +0100, Jiri Pirko wrote:
>> >> >> Do the same thing as in set mac. Call notifiers every time.
>> >> >> 
>> >> >> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
 ...
> Acked-by: Neil Horman <nhorman@tuxdriver.com>

Applied.
--
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

Patch

diff --git a/net/core/dev.c b/net/core/dev.c
index 2f94df2..0685a72 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4971,7 +4971,7 @@  int dev_set_mtu(struct net_device *dev, int new_mtu)
 	else
 		dev->mtu = new_mtu;
 
-	if (!err && dev->flags & IFF_UP)
+	if (!err)
 		call_netdevice_notifiers(NETDEV_CHANGEMTU, dev);
 	return err;
 }