* [PATCH net-next 1/2] net: inform NETDEV_CHANGE callbacks which flags were changed
@ 2013-05-23 9:58 Timo Teräs
2013-05-23 9:58 ` [PATCH net-next 2/2] arp: flush arp cache on IFF_NOARP change Timo Teräs
2013-05-23 16:31 ` [PATCH net-next 1/2] net: inform NETDEV_CHANGE callbacks which flags were changed Ben Hutchings
0 siblings, 2 replies; 5+ messages in thread
From: Timo Teräs @ 2013-05-23 9:58 UTC (permalink / raw)
To: netdev; +Cc: Timo Teräs, Ben Hutchings
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(-)
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;
+ }
}
/**
--
1.8.2.3
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH net-next 2/2] arp: flush arp cache on IFF_NOARP change
2013-05-23 9:58 [PATCH net-next 1/2] net: inform NETDEV_CHANGE callbacks which flags were changed Timo Teräs
@ 2013-05-23 9:58 ` Timo Teräs
2013-05-23 10:23 ` David Laight
2013-05-23 16:31 ` [PATCH net-next 1/2] net: inform NETDEV_CHANGE callbacks which flags were changed Ben Hutchings
1 sibling, 1 reply; 5+ messages in thread
From: Timo Teräs @ 2013-05-23 9:58 UTC (permalink / raw)
To: netdev; +Cc: Timo Teräs
IFF_NOARP affects what kind of neighbor entries are created
(nud NOARP or nud INCOMPLETE). If the flag changes, flush the arp
cache to refresh all entries.
Signed-off-by: Timo Teräs <timo.teras@iki.fi>
---
net/ipv4/arp.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 247ec19..0a15fb7 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -1241,6 +1241,10 @@ static int arp_netdev_event(struct notifier_block *this, unsigned long event,
neigh_changeaddr(&arp_tbl, dev);
rt_cache_flush(dev_net(dev));
break;
+ case NETDEV_CHANGE:
+ if (dev->flags_changed & IFF_NOARP)
+ neigh_changeaddr(&arp_tbl, dev);
+ break;
default:
break;
}
--
1.8.2.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [PATCH net-next 2/2] arp: flush arp cache on IFF_NOARP change
2013-05-23 9:58 ` [PATCH net-next 2/2] arp: flush arp cache on IFF_NOARP change Timo Teräs
@ 2013-05-23 10:23 ` David Laight
2013-05-23 12:01 ` Timo Teras
0 siblings, 1 reply; 5+ messages in thread
From: David Laight @ 2013-05-23 10:23 UTC (permalink / raw)
To: Timo Teräs, netdev
> IFF_NOARP affects what kind of neighbor entries are created
> (nud NOARP or nud INCOMPLETE). If the flag changes, flush the arp
> cache to refresh all entries.
Might someone want to use this to stop further arp table
entries being created?
In which case you don't want anything flushed.
David
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next 2/2] arp: flush arp cache on IFF_NOARP change
2013-05-23 10:23 ` David Laight
@ 2013-05-23 12:01 ` Timo Teras
0 siblings, 0 replies; 5+ messages in thread
From: Timo Teras @ 2013-05-23 12:01 UTC (permalink / raw)
To: David Laight; +Cc: netdev
On Thu, 23 May 2013 11:23:28 +0100
"David Laight" <David.Laight@ACULAB.COM> wrote:
> > IFF_NOARP affects what kind of neighbor entries are created
> > (nud NOARP or nud INCOMPLETE). If the flag changes, flush the arp
> > cache to refresh all entries.
>
> Might someone want to use this to stop further arp table
> entries being created?
> In which case you don't want anything flushed.
I don't think that would make any sense. The entries we have would
expire soon, and the other hosts would not be able to get arp replies
for our host.
Normally this flag is not changed. My use case is with ip gre tunnels,
and starting opennhrp daemon. Opennhrp enables address resolution via
netlink for gre tunnels - it turns off NOARP flag and configures real
ARP off and enables netlink ARPD requests. If before startup there was
traffic to gre tunnel, there will be stale NOARP entries preventing
traffic after daemon is started.
- Timo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next 1/2] net: inform NETDEV_CHANGE callbacks which flags were changed
2013-05-23 9:58 [PATCH net-next 1/2] net: inform NETDEV_CHANGE callbacks which flags were changed Timo Teräs
2013-05-23 9:58 ` [PATCH net-next 2/2] arp: flush arp cache on IFF_NOARP change Timo Teräs
@ 2013-05-23 16:31 ` Ben Hutchings
1 sibling, 0 replies; 5+ messages in thread
From: Ben Hutchings @ 2013-05-23 16:31 UTC (permalink / raw)
To: Timo Teräs; +Cc: netdev
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;
> + }
> }
>
> /**
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-05-23 16:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-23 9:58 [PATCH net-next 1/2] net: inform NETDEV_CHANGE callbacks which flags were changed Timo Teräs
2013-05-23 9:58 ` [PATCH net-next 2/2] arp: flush arp cache on IFF_NOARP change Timo Teräs
2013-05-23 10:23 ` David Laight
2013-05-23 12:01 ` Timo Teras
2013-05-23 16:31 ` [PATCH net-next 1/2] net: inform NETDEV_CHANGE callbacks which flags were changed Ben Hutchings
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).