All of lore.kernel.org
 help / color / mirror / Atom feed
* [patch net-next] bonding: create netlink event when bonding option is changed
@ 2014-08-19 14:02 Jiri Pirko
  2014-08-19 20:34 ` Andy Gospodarek
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Jiri Pirko @ 2014-08-19 14:02 UTC (permalink / raw)
  To: netdev; +Cc: davem, j.vosburgh, vfalico, andy, nikolay, dingtianhong, sfeldma

Userspace needs to be notified if one changes some option.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 drivers/net/bonding/bond_options.c | 2 ++
 include/linux/netdevice.h          | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index dc73463..d8dc17f 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -625,6 +625,8 @@ int __bond_opt_set(struct bonding *bond,
 out:
 	if (ret)
 		bond_opt_error_interpret(bond, opt, ret, val);
+	else
+		call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev);
 
 	return ret;
 }
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 967ddcc..44bc4bd 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2017,6 +2017,7 @@ struct pcpu_sw_netstats {
 #define NETDEV_CHANGEUPPER	0x0015
 #define NETDEV_RESEND_IGMP	0x0016
 #define NETDEV_PRECHANGEMTU	0x0017 /* notify before mtu change happened */
+#define NETDEV_CHANGEINFODATA	0x0018
 
 int register_netdevice_notifier(struct notifier_block *nb);
 int unregister_netdevice_notifier(struct notifier_block *nb);
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [patch net-next] bonding: create netlink event when bonding option is changed
  2014-08-19 14:02 [patch net-next] bonding: create netlink event when bonding option is changed Jiri Pirko
@ 2014-08-19 20:34 ` Andy Gospodarek
  2014-08-19 20:37   ` Jiri Pirko
  2014-08-20 12:18 ` Neil Horman
  2014-08-22 19:34 ` David Miller
  2 siblings, 1 reply; 11+ messages in thread
From: Andy Gospodarek @ 2014-08-19 20:34 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: netdev, davem, j.vosburgh, vfalico, andy, nikolay, dingtianhong, sfeldma

On Tue, Aug 19, 2014 at 04:02:12PM +0200, Jiri Pirko wrote:
> Userspace needs to be notified if one changes some option.
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>  drivers/net/bonding/bond_options.c | 2 ++
>  include/linux/netdevice.h          | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index dc73463..d8dc17f 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -625,6 +625,8 @@ int __bond_opt_set(struct bonding *bond,
>  out:
>  	if (ret)
>  		bond_opt_error_interpret(bond, opt, ret, val);
> +	else
> +		call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev);
Is this valuable where the configuration change to the bonding driver
came from netlink.  Do you have a good example why this useful in the
netlink path as well as when using sysfs?  It would seem more logical to
add call_netdevice_notifiers to bonding_sysfs_store_option so anything
listening to netlink messages or the netlink cache can know to check for
changes.

>  
>  	return ret;
>  }
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 967ddcc..44bc4bd 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2017,6 +2017,7 @@ struct pcpu_sw_netstats {
>  #define NETDEV_CHANGEUPPER	0x0015
>  #define NETDEV_RESEND_IGMP	0x0016
>  #define NETDEV_PRECHANGEMTU	0x0017 /* notify before mtu change happened */
> +#define NETDEV_CHANGEINFODATA	0x0018
>  
>  int register_netdevice_notifier(struct notifier_block *nb);
>  int unregister_netdevice_notifier(struct notifier_block *nb);
> -- 
> 1.9.3
> 
> --
> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch net-next] bonding: create netlink event when bonding option is changed
  2014-08-19 20:34 ` Andy Gospodarek
@ 2014-08-19 20:37   ` Jiri Pirko
  2014-08-19 20:46     ` Andy Gospodarek
  0 siblings, 1 reply; 11+ messages in thread
From: Jiri Pirko @ 2014-08-19 20:37 UTC (permalink / raw)
  To: Andy Gospodarek
  Cc: netdev, davem, j.vosburgh, vfalico, andy, nikolay, dingtianhong, sfeldma

Tue, Aug 19, 2014 at 10:34:01PM CEST, gospo@cumulusnetworks.com wrote:
>On Tue, Aug 19, 2014 at 04:02:12PM +0200, Jiri Pirko wrote:
>> Userspace needs to be notified if one changes some option.
>> 
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>>  drivers/net/bonding/bond_options.c | 2 ++
>>  include/linux/netdevice.h          | 1 +
>>  2 files changed, 3 insertions(+)
>> 
>> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>> index dc73463..d8dc17f 100644
>> --- a/drivers/net/bonding/bond_options.c
>> +++ b/drivers/net/bonding/bond_options.c
>> @@ -625,6 +625,8 @@ int __bond_opt_set(struct bonding *bond,
>>  out:
>>  	if (ret)
>>  		bond_opt_error_interpret(bond, opt, ret, val);
>> +	else
>> +		call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev);
>Is this valuable where the configuration change to the bonding driver
>came from netlink.  Do you have a good example why this useful in the
>netlink path as well as when using sysfs?  It would seem more logical to
>add call_netdevice_notifiers to bonding_sysfs_store_option so anything
>listening to netlink messages or the netlink cache can know to check for
>changes.

But this is called for sysfs as well.

Example of use is that application can monitor rtnetlink to see if for
example active slave changed, or whatever other option changed.

>
>>  
>>  	return ret;
>>  }
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 967ddcc..44bc4bd 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -2017,6 +2017,7 @@ struct pcpu_sw_netstats {
>>  #define NETDEV_CHANGEUPPER	0x0015
>>  #define NETDEV_RESEND_IGMP	0x0016
>>  #define NETDEV_PRECHANGEMTU	0x0017 /* notify before mtu change happened */
>> +#define NETDEV_CHANGEINFODATA	0x0018
>>  
>>  int register_netdevice_notifier(struct notifier_block *nb);
>>  int unregister_netdevice_notifier(struct notifier_block *nb);
>> -- 
>> 1.9.3
>> 
>> --
>> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch net-next] bonding: create netlink event when bonding option is changed
  2014-08-19 20:37   ` Jiri Pirko
@ 2014-08-19 20:46     ` Andy Gospodarek
  2014-08-20  6:44       ` Jiri Pirko
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Gospodarek @ 2014-08-19 20:46 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: netdev, davem, j.vosburgh, vfalico, andy, nikolay, dingtianhong, sfeldma

On Tue, Aug 19, 2014 at 10:37:48PM +0200, Jiri Pirko wrote:
> Tue, Aug 19, 2014 at 10:34:01PM CEST, gospo@cumulusnetworks.com wrote:
> >On Tue, Aug 19, 2014 at 04:02:12PM +0200, Jiri Pirko wrote:
> >> Userspace needs to be notified if one changes some option.
> >> 
> >> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> >> ---
> >>  drivers/net/bonding/bond_options.c | 2 ++
> >>  include/linux/netdevice.h          | 1 +
> >>  2 files changed, 3 insertions(+)
> >> 
> >> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> >> index dc73463..d8dc17f 100644
> >> --- a/drivers/net/bonding/bond_options.c
> >> +++ b/drivers/net/bonding/bond_options.c
> >> @@ -625,6 +625,8 @@ int __bond_opt_set(struct bonding *bond,
> >>  out:
> >>  	if (ret)
> >>  		bond_opt_error_interpret(bond, opt, ret, val);
> >> +	else
> >> +		call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev);
> >Is this valuable where the configuration change to the bonding driver
> >came from netlink.  Do you have a good example why this useful in the
> >netlink path as well as when using sysfs?  It would seem more logical to
> >add call_netdevice_notifiers to bonding_sysfs_store_option so anything
> >listening to netlink messages or the netlink cache can know to check for
> >changes.
> 
> But this is called for sysfs as well.
Sorry for the confusion.  I was suggesting that this call is *only* used
for sysfs changes not for changes from sysfs and netlink as it would
with your patch.

> Example of use is that application can monitor rtnetlink to see if for
> example active slave changed, or whatever other option changed.
Any application monitoring netlink messages will already know about the
configuration change if it came down via netlink.  There would not be a
need for an extra message.

My question was:  Is there a need for 2 netlink messages to indicate
that a bonding configuration change was needed when the config came over
netlink.  I did not see the need for 2 netlink messages in this case and
was asking for clarification from you.

> 
> >
> >>  
> >>  	return ret;
> >>  }
> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >> index 967ddcc..44bc4bd 100644
> >> --- a/include/linux/netdevice.h
> >> +++ b/include/linux/netdevice.h
> >> @@ -2017,6 +2017,7 @@ struct pcpu_sw_netstats {
> >>  #define NETDEV_CHANGEUPPER	0x0015
> >>  #define NETDEV_RESEND_IGMP	0x0016
> >>  #define NETDEV_PRECHANGEMTU	0x0017 /* notify before mtu change happened */
> >> +#define NETDEV_CHANGEINFODATA	0x0018
> >>  
> >>  int register_netdevice_notifier(struct notifier_block *nb);
> >>  int unregister_netdevice_notifier(struct notifier_block *nb);
> >> -- 
> >> 1.9.3
> >> 
> >> --
> >> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch net-next] bonding: create netlink event when bonding option is changed
  2014-08-19 20:46     ` Andy Gospodarek
@ 2014-08-20  6:44       ` Jiri Pirko
  2014-08-20 11:11         ` Veaceslav Falico
  2014-08-20 14:00         ` Andy Gospodarek
  0 siblings, 2 replies; 11+ messages in thread
From: Jiri Pirko @ 2014-08-20  6:44 UTC (permalink / raw)
  To: Andy Gospodarek
  Cc: netdev, davem, j.vosburgh, vfalico, andy, nikolay, dingtianhong, sfeldma

Tue, Aug 19, 2014 at 10:46:34PM CEST, gospo@cumulusnetworks.com wrote:
>On Tue, Aug 19, 2014 at 10:37:48PM +0200, Jiri Pirko wrote:
>> Tue, Aug 19, 2014 at 10:34:01PM CEST, gospo@cumulusnetworks.com wrote:
>> >On Tue, Aug 19, 2014 at 04:02:12PM +0200, Jiri Pirko wrote:
>> >> Userspace needs to be notified if one changes some option.
>> >> 
>> >> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> >> ---
>> >>  drivers/net/bonding/bond_options.c | 2 ++
>> >>  include/linux/netdevice.h          | 1 +
>> >>  2 files changed, 3 insertions(+)
>> >> 
>> >> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>> >> index dc73463..d8dc17f 100644
>> >> --- a/drivers/net/bonding/bond_options.c
>> >> +++ b/drivers/net/bonding/bond_options.c
>> >> @@ -625,6 +625,8 @@ int __bond_opt_set(struct bonding *bond,
>> >>  out:
>> >>  	if (ret)
>> >>  		bond_opt_error_interpret(bond, opt, ret, val);
>> >> +	else
>> >> +		call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev);
>> >Is this valuable where the configuration change to the bonding driver
>> >came from netlink.  Do you have a good example why this useful in the
>> >netlink path as well as when using sysfs?  It would seem more logical to
>> >add call_netdevice_notifiers to bonding_sysfs_store_option so anything
>> >listening to netlink messages or the netlink cache can know to check for
>> >changes.
>> 
>> But this is called for sysfs as well.
>Sorry for the confusion.  I was suggesting that this call is *only* used
>for sysfs changes not for changes from sysfs and netlink as it would
>with your patch.
>
>> Example of use is that application can monitor rtnetlink to see if for
>> example active slave changed, or whatever other option changed.
>Any application monitoring netlink messages will already know about the
>configuration change if it came down via netlink.  There would not be a
>need for an extra message.
>
>My question was:  Is there a need for 2 netlink messages to indicate
>that a bonding configuration change was needed when the config came over
>netlink.  I did not see the need for 2 netlink messages in this case and
>was asking for clarification from you.

Imagine 2 applications. One is just monitoring netlink events ("ip
mon"), the second is setting up bond via netlink. Now, the second sends
a message to kernel, that is unicast to kernel. The first application
does not see that message. Therefore there is need to generate the event
message in kernel and send it back to userspace via multicast.
That message the first application will see. And that is exactly what
my patch is doing.

>
>> 
>> >
>> >>  
>> >>  	return ret;
>> >>  }
>> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> >> index 967ddcc..44bc4bd 100644
>> >> --- a/include/linux/netdevice.h
>> >> +++ b/include/linux/netdevice.h
>> >> @@ -2017,6 +2017,7 @@ struct pcpu_sw_netstats {
>> >>  #define NETDEV_CHANGEUPPER	0x0015
>> >>  #define NETDEV_RESEND_IGMP	0x0016
>> >>  #define NETDEV_PRECHANGEMTU	0x0017 /* notify before mtu change happened */
>> >> +#define NETDEV_CHANGEINFODATA	0x0018
>> >>  
>> >>  int register_netdevice_notifier(struct notifier_block *nb);
>> >>  int unregister_netdevice_notifier(struct notifier_block *nb);
>> >> -- 
>> >> 1.9.3
>> >> 
>> >> --
>> >> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch net-next] bonding: create netlink event when bonding option is changed
  2014-08-20  6:44       ` Jiri Pirko
@ 2014-08-20 11:11         ` Veaceslav Falico
  2014-08-20 14:00         ` Andy Gospodarek
  1 sibling, 0 replies; 11+ messages in thread
From: Veaceslav Falico @ 2014-08-20 11:11 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: Andy Gospodarek, netdev, davem, j.vosburgh, andy, nikolay,
	dingtianhong, sfeldma

On Wed, Aug 20, 2014 at 08:44:58AM +0200, Jiri Pirko wrote:
>Tue, Aug 19, 2014 at 10:46:34PM CEST, gospo@cumulusnetworks.com wrote:
>>On Tue, Aug 19, 2014 at 10:37:48PM +0200, Jiri Pirko wrote:
>>> Tue, Aug 19, 2014 at 10:34:01PM CEST, gospo@cumulusnetworks.com wrote:
>>> >On Tue, Aug 19, 2014 at 04:02:12PM +0200, Jiri Pirko wrote:
...snip...
>>> >>  	if (ret)
>>> >>  		bond_opt_error_interpret(bond, opt, ret, val);
>>> >> +	else
>>> >> +		call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev);
...snip...
>>My question was:  Is there a need for 2 netlink messages to indicate
>>that a bonding configuration change was needed when the config came over
>>netlink.  I did not see the need for 2 netlink messages in this case and
>>was asking for clarification from you.
>
>Imagine 2 applications. One is just monitoring netlink events ("ip
>mon"), the second is setting up bond via netlink. Now, the second sends
>a message to kernel, that is unicast to kernel. The first application
>does not see that message. Therefore there is need to generate the event
>message in kernel and send it back to userspace via multicast.
>That message the first application will see. And that is exactly what
>my patch is doing.

Seems fair, as there's no other way to catch those modifications, even
though they're known to the "changing" application.

Acked-by: Veaceslav Falico <vfalico@gmail.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch net-next] bonding: create netlink event when bonding option is changed
  2014-08-19 14:02 [patch net-next] bonding: create netlink event when bonding option is changed Jiri Pirko
  2014-08-19 20:34 ` Andy Gospodarek
@ 2014-08-20 12:18 ` Neil Horman
  2014-08-20 12:49   ` Jiri Pirko
  2014-08-22 19:34 ` David Miller
  2 siblings, 1 reply; 11+ messages in thread
From: Neil Horman @ 2014-08-20 12:18 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: netdev, davem, j.vosburgh, vfalico, andy, nikolay, dingtianhong, sfeldma

On Tue, Aug 19, 2014 at 04:02:12PM +0200, Jiri Pirko wrote:
> Userspace needs to be notified if one changes some option.
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>  drivers/net/bonding/bond_options.c | 2 ++
>  include/linux/netdevice.h          | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index dc73463..d8dc17f 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -625,6 +625,8 @@ int __bond_opt_set(struct bonding *bond,
>  out:
>  	if (ret)
>  		bond_opt_error_interpret(bond, opt, ret, val);
> +	else
> +		call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev);
>  
>  	return ret;
>  }
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 967ddcc..44bc4bd 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2017,6 +2017,7 @@ struct pcpu_sw_netstats {
>  #define NETDEV_CHANGEUPPER	0x0015
>  #define NETDEV_RESEND_IGMP	0x0016
>  #define NETDEV_PRECHANGEMTU	0x0017 /* notify before mtu change happened */
> +#define NETDEV_CHANGEINFODATA	0x0018
>  
Do you need to create a new netdev event here?  Seems like it might be more
concise to reuse NETDEV_CHANGE and expand the netdev_notifier_change_info
struct.

Neil

>  int register_netdevice_notifier(struct notifier_block *nb);
>  int unregister_netdevice_notifier(struct notifier_block *nb);
> -- 
> 1.9.3
> 
> --
> 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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch net-next] bonding: create netlink event when bonding option is changed
  2014-08-20 12:18 ` Neil Horman
@ 2014-08-20 12:49   ` Jiri Pirko
  0 siblings, 0 replies; 11+ messages in thread
From: Jiri Pirko @ 2014-08-20 12:49 UTC (permalink / raw)
  To: Neil Horman
  Cc: netdev, davem, j.vosburgh, vfalico, andy, nikolay, dingtianhong, sfeldma

Wed, Aug 20, 2014 at 02:18:28PM CEST, nhorman@tuxdriver.com wrote:
>On Tue, Aug 19, 2014 at 04:02:12PM +0200, Jiri Pirko wrote:
>> Userspace needs to be notified if one changes some option.
>> 
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>>  drivers/net/bonding/bond_options.c | 2 ++
>>  include/linux/netdevice.h          | 1 +
>>  2 files changed, 3 insertions(+)
>> 
>> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>> index dc73463..d8dc17f 100644
>> --- a/drivers/net/bonding/bond_options.c
>> +++ b/drivers/net/bonding/bond_options.c
>> @@ -625,6 +625,8 @@ int __bond_opt_set(struct bonding *bond,
>>  out:
>>  	if (ret)
>>  		bond_opt_error_interpret(bond, opt, ret, val);
>> +	else
>> +		call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev);
>>  
>>  	return ret;
>>  }
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 967ddcc..44bc4bd 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -2017,6 +2017,7 @@ struct pcpu_sw_netstats {
>>  #define NETDEV_CHANGEUPPER	0x0015
>>  #define NETDEV_RESEND_IGMP	0x0016
>>  #define NETDEV_PRECHANGEMTU	0x0017 /* notify before mtu change happened */
>> +#define NETDEV_CHANGEINFODATA	0x0018
>>  
>Do you need to create a new netdev event here?  Seems like it might be more
>concise to reuse NETDEV_CHANGE and expand the netdev_notifier_change_info
>struct.

No, I would not like to do that. There are many notifiers processing
NETDEV_CHANGE now. Plus there is no limitation in adding event number.
Plus since this it not part of any api, it can be easily changed. I
prefer to add another event number.

>
>Neil
>
>>  int register_netdevice_notifier(struct notifier_block *nb);
>>  int unregister_netdevice_notifier(struct notifier_block *nb);
>> -- 
>> 1.9.3
>> 
>> --
>> 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
>> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch net-next] bonding: create netlink event when bonding option is changed
  2014-08-20  6:44       ` Jiri Pirko
  2014-08-20 11:11         ` Veaceslav Falico
@ 2014-08-20 14:00         ` Andy Gospodarek
  2014-08-20 17:49           ` Stephen Hemminger
  1 sibling, 1 reply; 11+ messages in thread
From: Andy Gospodarek @ 2014-08-20 14:00 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: netdev, davem, j.vosburgh, vfalico, andy, nikolay, dingtianhong, sfeldma

On Wed, Aug 20, 2014 at 08:44:58AM +0200, Jiri Pirko wrote:
> Tue, Aug 19, 2014 at 10:46:34PM CEST, gospo@cumulusnetworks.com wrote:
> >On Tue, Aug 19, 2014 at 10:37:48PM +0200, Jiri Pirko wrote:
> >> Tue, Aug 19, 2014 at 10:34:01PM CEST, gospo@cumulusnetworks.com wrote:
> >> >On Tue, Aug 19, 2014 at 04:02:12PM +0200, Jiri Pirko wrote:
> >> >> Userspace needs to be notified if one changes some option.
> >> >> 
> >> >> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> >> >> ---
> >> >>  drivers/net/bonding/bond_options.c | 2 ++
> >> >>  include/linux/netdevice.h          | 1 +
> >> >>  2 files changed, 3 insertions(+)
> >> >> 
> >> >> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> >> >> index dc73463..d8dc17f 100644
> >> >> --- a/drivers/net/bonding/bond_options.c
> >> >> +++ b/drivers/net/bonding/bond_options.c
> >> >> @@ -625,6 +625,8 @@ int __bond_opt_set(struct bonding *bond,
> >> >>  out:
> >> >>  	if (ret)
> >> >>  		bond_opt_error_interpret(bond, opt, ret, val);
> >> >> +	else
> >> >> +		call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev);
> >> >Is this valuable where the configuration change to the bonding driver
> >> >came from netlink.  Do you have a good example why this useful in the
> >> >netlink path as well as when using sysfs?  It would seem more logical to
> >> >add call_netdevice_notifiers to bonding_sysfs_store_option so anything
> >> >listening to netlink messages or the netlink cache can know to check for
> >> >changes.
> >> 
> >> But this is called for sysfs as well.
> >Sorry for the confusion.  I was suggesting that this call is *only* used
> >for sysfs changes not for changes from sysfs and netlink as it would
> >with your patch.
> >
> >> Example of use is that application can monitor rtnetlink to see if for
> >> example active slave changed, or whatever other option changed.
> >Any application monitoring netlink messages will already know about the
> >configuration change if it came down via netlink.  There would not be a
> >need for an extra message.
> >
> >My question was:  Is there a need for 2 netlink messages to indicate
> >that a bonding configuration change was needed when the config came over
> >netlink.  I did not see the need for 2 netlink messages in this case and
> >was asking for clarification from you.
> 
> Imagine 2 applications. One is just monitoring netlink events ("ip
> mon"), the second is setting up bond via netlink. Now, the second sends
> a message to kernel, that is unicast to kernel. The first application
> does not see that message. Therefore there is need to generate the event
> message in kernel and send it back to userspace via multicast.
> That message the first application will see. And that is exactly what
> my patch is doing.

Agreed.  Thanks for having the discussion on this.

Acked-by: Andy Gospodarek <gospo@cumulusnetworks.com>

> 
> >
> >> 
> >> >
> >> >>  
> >> >>  	return ret;
> >> >>  }
> >> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >> >> index 967ddcc..44bc4bd 100644
> >> >> --- a/include/linux/netdevice.h
> >> >> +++ b/include/linux/netdevice.h
> >> >> @@ -2017,6 +2017,7 @@ struct pcpu_sw_netstats {
> >> >>  #define NETDEV_CHANGEUPPER	0x0015
> >> >>  #define NETDEV_RESEND_IGMP	0x0016
> >> >>  #define NETDEV_PRECHANGEMTU	0x0017 /* notify before mtu change happened */
> >> >> +#define NETDEV_CHANGEINFODATA	0x0018
> >> >>  
> >> >>  int register_netdevice_notifier(struct notifier_block *nb);
> >> >>  int unregister_netdevice_notifier(struct notifier_block *nb);
> >> >> -- 
> >> >> 1.9.3
> >> >> 
> >> >> --
> >> >> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch net-next] bonding: create netlink event when bonding option is changed
  2014-08-20 14:00         ` Andy Gospodarek
@ 2014-08-20 17:49           ` Stephen Hemminger
  0 siblings, 0 replies; 11+ messages in thread
From: Stephen Hemminger @ 2014-08-20 17:49 UTC (permalink / raw)
  To: Andy Gospodarek
  Cc: Jiri Pirko, netdev, davem, j.vosburgh, vfalico, andy, nikolay,
	dingtianhong, sfeldma

On Wed, 20 Aug 2014 10:00:51 -0400
Andy Gospodarek <gospo@cumulusnetworks.com> wrote:

> > >
> > >My question was:  Is there a need for 2 netlink messages to indicate
> > >that a bonding configuration change was needed when the config came over
> > >netlink.  I did not see the need for 2 netlink messages in this case and
> > >was asking for clarification from you.  
> > 
> > Imagine 2 applications. One is just monitoring netlink events ("ip
> > mon"), the second is setting up bond via netlink. Now, the second sends
> > a message to kernel, that is unicast to kernel. The first application
> > does not see that message. Therefore there is need to generate the event
> > message in kernel and send it back to userspace via multicast.
> > That message the first application will see. And that is exactly what
> > my patch is doing.  
> 
> Agreed.  Thanks for having the discussion on this.
> 
> Acked-by: Andy Gospodarek <gospo@cumulusnetworks.com>

Quagga uses socket filter to solve this problem.
The application filters out it's own route change events.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [patch net-next] bonding: create netlink event when bonding option is changed
  2014-08-19 14:02 [patch net-next] bonding: create netlink event when bonding option is changed Jiri Pirko
  2014-08-19 20:34 ` Andy Gospodarek
  2014-08-20 12:18 ` Neil Horman
@ 2014-08-22 19:34 ` David Miller
  2 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2014-08-22 19:34 UTC (permalink / raw)
  To: jiri; +Cc: netdev, j.vosburgh, vfalico, andy, nikolay, dingtianhong, sfeldma

From: Jiri Pirko <jiri@resnulli.us>
Date: Tue, 19 Aug 2014 16:02:12 +0200

> Userspace needs to be notified if one changes some option.
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

Applied.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2014-08-22 19:34 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-19 14:02 [patch net-next] bonding: create netlink event when bonding option is changed Jiri Pirko
2014-08-19 20:34 ` Andy Gospodarek
2014-08-19 20:37   ` Jiri Pirko
2014-08-19 20:46     ` Andy Gospodarek
2014-08-20  6:44       ` Jiri Pirko
2014-08-20 11:11         ` Veaceslav Falico
2014-08-20 14:00         ` Andy Gospodarek
2014-08-20 17:49           ` Stephen Hemminger
2014-08-20 12:18 ` Neil Horman
2014-08-20 12:49   ` Jiri Pirko
2014-08-22 19:34 ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.