diff mbox

[net-next,1/2] net: inform NETDEV_CHANGE callbacks which flags were changed

Message ID 1369303109-12003-1-git-send-email-timo.teras@iki.fi
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Timo Teras May 23, 2013, 9:58 a.m. UTC
In certain cases (like the follow up commit to arp.c) will need to
check which flags actually changed to avoid excessive work.

Ben Hutchings nicely worded why to put these transient flags to
struct net_device for the time being:
> It's inelegant to put transient data associated with an event in a
> persistent data structure.  On the other hand, having every user cache
> the old state is pretty awful as well.
>
> Really, netdev notifiers should be changed to accept a structure that
> encapsulates the changes rather than just a pointer to the net_device.
> But making such a change would be an enormous pain and error-prone
> because notifier functions aren't type-safe.
>
> As an interim solution, I think either the general priv_flags_changed or
> old_priv_flags would be preferable to defining extra transient flags.

Signed-off-by: Timo Teräs <timo.teras@iki.fi>
Cc: Ben Hutchings <bhutchings@solarflare.com>
---
 include/linux/netdevice.h | 4 +++-
 net/core/dev.c            | 5 ++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

Comments

Ben Hutchings May 23, 2013, 4:31 p.m. UTC | #1
On Thu, 2013-05-23 at 12:58 +0300, Timo Teräs wrote:
> In certain cases (like the follow up commit to arp.c) will need to
> check which flags actually changed to avoid excessive work.
> 
> Ben Hutchings nicely worded why to put these transient flags to
> struct net_device for the time being:
> > It's inelegant to put transient data associated with an event in a
> > persistent data structure.  On the other hand, having every user cache
> > the old state is pretty awful as well.
> >
> > Really, netdev notifiers should be changed to accept a structure that
> > encapsulates the changes rather than just a pointer to the net_device.
> > But making such a change would be an enormous pain and error-prone
> > because notifier functions aren't type-safe.
> >
> > As an interim solution, I think either the general priv_flags_changed or
> > old_priv_flags would be preferable to defining extra transient flags.
> 
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> Cc: Ben Hutchings <bhutchings@solarflare.com>

Could you delete the two instances of 'priv_' from the quoted text?  I
was confused about to which flags field was being changed.  With that,
you may add 'Acked-by: Ben Hutchings <bhutchings@solarflare.com>'

Ben.

> ---
>  include/linux/netdevice.h | 4 +++-
>  net/core/dev.c            | 5 ++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 7dd535d..86c155a 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1112,7 +1112,9 @@ struct net_device {
>  	/* Hardware header description */
>  	const struct header_ops *header_ops;
>  
> -	unsigned int		flags;	/* interface flags (a la BSD)	*/
> +	unsigned int		flags;		/* interface flags (a la BSD)	*/
> +	unsigned int		flags_changed;	/* flags that are being changed
> +						 * valid during NETDEV_CHANGE notifier */
>  	unsigned int		priv_flags; /* Like 'flags' but invisible to userspace.
>  					     * See if.h for definitions. */
>  	unsigned short		gflags;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 7229bc3..221634f 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4741,8 +4741,11 @@ void __dev_notify_flags(struct net_device *dev, unsigned int old_flags)
>  	}
>  
>  	if (dev->flags & IFF_UP &&
> -	    (changes & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI | IFF_VOLATILE)))
> +	    (changes & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI | IFF_VOLATILE))) {
> +		dev->flags_changed = changes;
>  		call_netdevice_notifiers(NETDEV_CHANGE, dev);
> +		dev->flags_changed = 0;
> +	}
>  }
>  
>  /**
diff mbox

Patch

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 7dd535d..86c155a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1112,7 +1112,9 @@  struct net_device {
 	/* Hardware header description */
 	const struct header_ops *header_ops;
 
-	unsigned int		flags;	/* interface flags (a la BSD)	*/
+	unsigned int		flags;		/* interface flags (a la BSD)	*/
+	unsigned int		flags_changed;	/* flags that are being changed
+						 * valid during NETDEV_CHANGE notifier */
 	unsigned int		priv_flags; /* Like 'flags' but invisible to userspace.
 					     * See if.h for definitions. */
 	unsigned short		gflags;
diff --git a/net/core/dev.c b/net/core/dev.c
index 7229bc3..221634f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4741,8 +4741,11 @@  void __dev_notify_flags(struct net_device *dev, unsigned int old_flags)
 	}
 
 	if (dev->flags & IFF_UP &&
-	    (changes & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI | IFF_VOLATILE)))
+	    (changes & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI | IFF_VOLATILE))) {
+		dev->flags_changed = changes;
 		call_netdevice_notifiers(NETDEV_CHANGE, dev);
+		dev->flags_changed = 0;
+	}
 }
 
 /**