Message ID | 1369131824-6318-1-git-send-email-timo.teras@iki.fi |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Tue, 2013-05-21 at 13:23 +0300, Timo Teräs 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. > > Signed-off-by: Timo Teräs <timo.teras@iki.fi> > --- > > This patch makes no sense at all. > > > > The state bit in ->priv_flags is a boolean stating whether the > > notified should do something or not. > > > > But you're setting it to match what IFF_NOARP is. > > > > You should set it any time IFF_NOARP _changes_, and then clear > > the bit when the notifier clears the neighbour entries. > > IFF_NOARP_CHANGED is set according to "changes = dev->flags ^ old_flags;" > which reflect the change. But I agree that the clearing out bit was > misplaced. This is especially true as it seems NETDEV_CHANGE can be > notified from another place too. [...] None of the other persistent flags have a transient flag for changes; why should IFF_NOARP? Why not cache the IFF_NOARP state in the in_device and then compare and update that in arp_netdev_event(). Ben.
On Tue, 21 May 2013 17:25:19 +0100 Ben Hutchings <bhutchings@solarflare.com> wrote: > On Tue, 2013-05-21 at 13:23 +0300, Timo Teräs 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. > > > > Signed-off-by: Timo Teräs <timo.teras@iki.fi> > > --- > > > This patch makes no sense at all. > > > > > > The state bit in ->priv_flags is a boolean stating whether the > > > notified should do something or not. > > > > > > But you're setting it to match what IFF_NOARP is. > > > > > > You should set it any time IFF_NOARP _changes_, and then clear > > > the bit when the notifier clears the neighbour entries. > > > > IFF_NOARP_CHANGED is set according to "changes = dev->flags ^ > > old_flags;" which reflect the change. But I agree that the clearing > > out bit was misplaced. This is especially true as it seems > > NETDEV_CHANGE can be notified from another place too. > [...] > > None of the other persistent flags have a transient flag for changes; > why should IFF_NOARP? Why not cache the IFF_NOARP state in the > in_device and then compare and update that in arp_netdev_event(). This was the earlier suggestion I got, so I followed that. NOARP flag is also used in IPv6 side (quick look would indicate that ndisc code needs similar fix), so it would be useful to have this generally available. Would it be worthwhile to consider adding "flags_changed" to struct net_device or some other mechanism add this info about all flags? -- 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 Tue, 2013-05-21 at 21:29 +0300, Timo Teras wrote: > On Tue, 21 May 2013 17:25:19 +0100 > Ben Hutchings <bhutchings@solarflare.com> wrote: > > > On Tue, 2013-05-21 at 13:23 +0300, Timo Teräs 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. > > > > > > Signed-off-by: Timo Teräs <timo.teras@iki.fi> > > > --- > > > > This patch makes no sense at all. > > > > > > > > The state bit in ->priv_flags is a boolean stating whether the > > > > notified should do something or not. > > > > > > > > But you're setting it to match what IFF_NOARP is. > > > > > > > > You should set it any time IFF_NOARP _changes_, and then clear > > > > the bit when the notifier clears the neighbour entries. > > > > > > IFF_NOARP_CHANGED is set according to "changes = dev->flags ^ > > > old_flags;" which reflect the change. But I agree that the clearing > > > out bit was misplaced. This is especially true as it seems > > > NETDEV_CHANGE can be notified from another place too. > > [...] > > > > None of the other persistent flags have a transient flag for changes; > > why should IFF_NOARP? Why not cache the IFF_NOARP state in the > > in_device and then compare and update that in arp_netdev_event(). > > This was the earlier suggestion I got, so I followed that. NOARP flag > is also used in IPv6 side (quick look would indicate that ndisc code > needs similar fix), so it would be useful to have this generally > available. Sorry, I missed that IFF_NOARP is not really just about ARP. > Would it be worthwhile to consider adding "flags_changed" to struct > net_device or some other mechanism add this info about all flags? 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. Ben.
From: Timo Teräs <timo.teras@iki.fi> Date: Tue, 21 May 2013 13:23:44 +0300 > 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> I agree with Ben that we should have a "orig_priv_flags" or something like that to implement this, rather than adding transient state flags to ->priv_flags. -- 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 Thu, 23 May 2013 00:01:36 -0700 (PDT) David Miller <davem@davemloft.net> wrote: > From: Timo Teräs <timo.teras@iki.fi> > Date: Tue, 21 May 2013 13:23:44 +0300 > > > 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> > > I agree with Ben that we should have a "orig_priv_flags" or > something like that to implement this, rather than adding > transient state flags to ->priv_flags. Sure. I'll split this to two patches then and do this. I would prefer "flags_changed" as then the notifiers can easily test on the change. Usually the care only about the new state, or if it changed. Otherwise the change calculation is needed on all notify callbacks. Will post this briefly, unless you feel that "orig_flags" is still preferred. -- 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/include/uapi/linux/if.h b/include/uapi/linux/if.h index 1ec407b..1be8b35 100644 --- a/include/uapi/linux/if.h +++ b/include/uapi/linux/if.h @@ -83,6 +83,8 @@ #define IFF_SUPP_NOFCS 0x80000 /* device supports sending custom FCS */ #define IFF_LIVE_ADDR_CHANGE 0x100000 /* device supports hardware address * change when it's running */ +#define IFF_NOARP_CHANGED 0x200000 /* Set during NETDEV_CHANGE notifier + * if IFF_NOARP has changed */ #define IF_GET_IFACE 0x0001 /* for querying only */ diff --git a/net/core/dev.c b/net/core/dev.c index 18e9730..ce30761 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -4699,8 +4699,12 @@ 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))) { + if (changes & IFF_NOARP) + dev->priv_flags |= IFF_NOARP_CHANGED; call_netdevice_notifiers(NETDEV_CHANGE, dev); + dev->priv_flags &= ~IFF_NOARP_CHANGED; + } } /** diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c index 247ec19..375b2f2 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->priv_flags & IFF_NOARP_CHANGED) + neigh_changeaddr(&arp_tbl, dev); + break; default: break; }