[PATCH 0/2] fixes to arp_notify for virtual machine migration use case

[PATCH 0/2] fixes to arp_notify for virtual machine migration use case

[Ian Campbell]

The following 2 patches fix two issues with arp_notify, one is a niggle
and the other makes the feature work with virtual machine migration.

The following changes since commit 94b849aaf6e22ab7bf54b0d0377a882d4892396d:
  Linus Torvalds (1):
        Merge branch 'fix/hda' of git://git.kernel.org/.../tiwai/sound-2.6

are available in the git repository at:

  git://xenbits.xensource.com/people/ianc/linux-2.6.git for-netdev/fixes

Ian Campbell (2):
      arp_notify: generate broadcast ARP reply not request.
      arp_notify: generate arp_notify event on NETDEV_CHANGE too

 net/ipv4/devinet.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

[PATCH] arp_notify: generate broadcast ARP reply not request.

[Ian Campbell]

The arp_notify option causes us to send a broadcast ARP request
with the host IP address in both the source and destination IP address
fields. More usually a gratuitous ARP packet is a broadcast ARP reply
with a broadcast destination IP and the local IP as the source

The documentation of the arp_notify option in
Documentation/networking/ip-sysctl.txt suggests this was the original
intention therefore switch it over.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Stephen Hemminger <shemminger@linux-foundation.org>
Cc: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: stable@kernel.org
---
 net/ipv4/devinet.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 90e3d63..e26f723 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1087,10 +1087,10 @@ static int inetdev_event(struct notifier_block *this, unsigned long event,
 			struct in_ifaddr *ifa = in_dev->ifa_list;
 
 			if (ifa)
-				arp_send(ARPOP_REQUEST, ETH_P_ARP,
-					 ifa->ifa_address, dev,
-					 ifa->ifa_address, NULL,
-					 dev->dev_addr, NULL);
+				arp_send(ARPOP_REPLY, ETH_P_ARP,
+					 INADDR_BROADCAST, dev,
+					 ifa->ifa_address,
+					 NULL, NULL, dev->dev_addr);
 		}
 		break;
 	case NETDEV_DOWN:
-- 
1.5.6.5

[PATCH] arp_notify: generate arp_notify event on NETDEV_CHANGE too

[Ian Campbell]

One of the use cases of the arp_notify functionality added in commit
eefef1 "net: add ARP notify option for devices" is to enable faster
network recovery after a virtual machine migration.

However a migration appears to the network subsystem as a temporary
loss of carrier rather than a down/up pair or an address change etc
therefore no gratuitous ARP is sent. Send one in the NETDEV_CHANGE
(generated by netif_carrier_{on,off}) case too. This catches carrier
down events too but that should be harmless.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Stephen Hemminger <shemminger@linux-foundation.org>
Cc: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: stable@kernel.org
---
 net/ipv4/devinet.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index e26f723..5b04124 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1081,6 +1081,7 @@ static int inetdev_event(struct notifier_block *this, unsigned long event,
 		}
 		ip_mc_up(in_dev);
 		/* fall through */
+	case NETDEV_CHANGE:
 	case NETDEV_CHANGEADDR:
 		/* Send gratuitous ARP to notify of link change */
 		if (IN_DEV_ARP_NOTIFY(in_dev)) {
-- 
1.5.6.5

Re: [PATCH 0/2] fixes to arp_notify for virtual machine migration use case

[David Miller]

From: Ian Campbell <Ian.Campbell@citrix.com>
Date: Wed, 12 May 2010 14:39:14 +0100

> Ian Campbell (2):
>       arp_notify: generate broadcast ARP reply not request.
>       arp_notify: generate arp_notify event on NETDEV_CHANGE too

I don't agree with these changes.

For the first one, I think the documentation is just wrong and the
code is what expresses the intent.  The idea is not to spam the
world with a broadcast, only interested parties.

Patch #2 I have major issues with, carriers flapping occaisionally is
very common.  I have several interfaces which do this even on lightly
loaded networks.  Iff we decide to do something like this (big "if")
it would need to be rate limited so that it doesn't trigger due to
normal flapping.

If you want your VM networking devices to trigger this event maybe
the best thing to do is to create a special notification which
allows us to prevent from doing this ARP notify for spurious physical
network device carrier flaps.

Re: [PATCH 0/2] fixes to arp_notify for virtual machine migration use case

[Ian Campbell]

On Mon, 2010-05-24 at 07:37 +0100, David Miller wrote:
> From: Ian Campbell <Ian.Campbell@citrix.com>
> Date: Wed, 12 May 2010 14:39:14 +0100
> 
> > Ian Campbell (2):
> >       arp_notify: generate broadcast ARP reply not request.
> >       arp_notify: generate arp_notify event on NETDEV_CHANGE too
> 
> I don't agree with these changes.
> 
> For the first one, I think the documentation is just wrong and the
> code is what expresses the intent.  The idea is not to spam the
> world with a broadcast, only interested parties.

OK. So what is currently sent is a gratuitous ARP request which I hadn't
realised was a valid/normal thing to send but a bit of research shows
that it is.

In terms of MAC learning tables (which is what is interesting in the VM
migration case) I think the gratuitous ARP request and reply are
equivalent.

I guess the difference is that an ARP request doesn't cause everyone to
add an entry to their ARP cache, whereas a bcast ARP reply would cause
an ARP cache entry to appear even on hosts which may never talk to the
VM. Is that what you meant by spamming the world?

Anyway, I'll patch the docs instead in my next submission.

> Patch #2 I have major issues with, carriers flapping occaisionally is
> very common.  I have several interfaces which do this even on lightly
> loaded networks.  Iff we decide to do something like this (big "if")
> it would need to be rate limited so that it doesn't trigger due to
> normal flapping.

FWIW arp_notify is an option which is disabled by default.

> If you want your VM networking devices to trigger this event maybe
> the best thing to do is to create a special notification which
> allows us to prevent from doing this ARP notify for spurious physical
> network device carrier flaps.

I actually started down this path before I figured out how to make the
existing notifications work for my use cases.

Anyway, assuming the fact that arp_notify is disabled by default hasn't
changed your mind, would adding NETDEV_NOTIFY_PEERS triggered by
netif_notify_peers() be appropriate or would it be preferable to simply
add netif_notify_peers() which generates the existing NETDEV_CHANGEADDR?

I don't think there's any need for a new sysctl so I'll gate the new
option on the existing arp_notify one.

Ian.

Re: [PATCH 0/2] fixes to arp_notify for virtual machine migration use case

[David Miller]

From: Ian Campbell <Ian.Campbell@citrix.com>
Date: Tue, 25 May 2010 10:20:01 +0100

> Anyway, assuming the fact that arp_notify is disabled by default hasn't
> changed your mind, would adding NETDEV_NOTIFY_PEERS triggered by
> netif_notify_peers() be appropriate or would it be preferable to simply
> add netif_notify_peers() which generates the existing NETDEV_CHANGEADDR?

A seperate NETDEV_NOTIFY_PEERS seems the best idea to me.

> I don't think there's any need for a new sysctl so I'll gate the new
> option on the existing arp_notify one.

Yep, sounds good.

Re: [PATCH 0/2] fixes to arp_notify for virtual machine migration use case

[Ian Campbell]

On Wed, 2010-05-26 at 07:08 +0100, David Miller wrote:
> From: Ian Campbell <Ian.Campbell@citrix.com>
> Date: Tue, 25 May 2010 10:20:01 +0100
> 
> > Anyway, assuming the fact that arp_notify is disabled by default hasn't
> > changed your mind, would adding NETDEV_NOTIFY_PEERS triggered by
> > netif_notify_peers() be appropriate or would it be preferable to simply
> > add netif_notify_peers() which generates the existing NETDEV_CHANGEADDR?
> 
> A seperate NETDEV_NOTIFY_PEERS seems the best idea to me.
> 
> > I don't think there's any need for a new sysctl so I'll gate the new
> > option on the existing arp_notify one.
> 
> Yep, sounds good.

The following changes since commit e40152ee1e1c7a63f4777791863215e3faa37a86:
  Linus Torvalds (1):
        Linus 2.6.34

are available in the git repository at:

  git://xenbits.xensource.com/people/ianc/linux-2.6.git for-netdev/fixes

Ian Campbell (3):
      arp_notify: document that a gratuitous ARP request is sent when this option is enabled
      arp_notify: allow drivers to explicitly request a notification event.
      xen: netfront: explicitly generate arp_notify event after migration.

 Documentation/networking/ip-sysctl.txt |    2 +-
 drivers/net/xen-netfront.c             |    1 +
 include/linux/netdevice.h              |    2 ++
 include/linux/notifier.h               |    1 +
 net/ipv4/devinet.c                     |    1 +
 net/sched/sch_generic.c                |   18 ++++++++++++++++++
 6 files changed, 24 insertions(+), 1 deletions(-)

[PATCH 1/3] arp_notify: document that a gratuitous ARP request is sent when this option is enabled

[Ian Campbell]

This option causes a gratuitous ARP request, not a reply as the documentation
currently suggests.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Stephen Hemminger <shemminger@linux-foundation.org>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
---
 Documentation/networking/ip-sysctl.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index 8b72c88..bd13884 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -872,7 +872,7 @@ arp_ignore - INTEGER
 arp_notify - BOOLEAN
 	Define mode for notification of address and device changes.
 	0 - (default): do nothing
-	1 - Generate gratuitous arp replies when device is brought up
+	1 - Generate gratuitous arp requests when device is brought up
 	    or hardware address changes.
 
 arp_accept - BOOLEAN
-- 
1.5.6.5

[PATCH 2/3] arp_notify: allow drivers to explicitly request a notification event.

[Ian Campbell]

Currently such notifications are only generated when the device comes up or the
address changes. However one use case for these notifications is to enable
faster network recovery after a virtual machine migration (by causing switches
to relearn their MAC tables). A migration appears to the network stack as a
temporary loss of carrier and therefore does not trigger either of the current
conditions. Rather than adding carrier up as a trigger (which can cause issues
when interfaces a flapping) simply add an interface which the driver can use
to explicitly trigger the notification.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Stephen Hemminger <shemminger@linux-foundation.org>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: stable@kernel.org
---
 include/linux/netdevice.h |    2 ++
 include/linux/notifier.h  |    1 +
 net/ipv4/devinet.c        |    1 +
 net/sched/sch_generic.c   |   18 ++++++++++++++++++
 4 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index fa8b476..4737996 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1661,6 +1661,8 @@ extern void netif_carrier_on(struct net_device *dev);
 
 extern void netif_carrier_off(struct net_device *dev);
 
+extern void netif_notify_peers(struct net_device *dev);
+
 /**
  *	netif_dormant_on - mark device as dormant.
  *	@dev: network device
diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index fee6c2f..45477f2 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -203,6 +203,7 @@ static inline int notifier_to_errno(int ret)
 #define NETDEV_BONDING_NEWTYPE  0x000F
 #define NETDEV_POST_INIT	0x0010
 #define NETDEV_UNREGISTER_BATCH 0x0011
+#define NETDEV_NOTIFY_PEERS	0x0012
 
 #define SYS_DOWN	0x0001	/* Notify of system down */
 #define SYS_RESTART	SYS_DOWN
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 90e3d63..c12777f 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1081,6 +1081,7 @@ static int inetdev_event(struct notifier_block *this, unsigned long event,
 		}
 		ip_mc_up(in_dev);
 		/* fall through */
+	case NETDEV_NOTIFY_PEERS:
 	case NETDEV_CHANGEADDR:
 		/* Send gratuitous ARP to notify of link change */
 		if (IN_DEV_ARP_NOTIFY(in_dev)) {
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index ff4dd53..2d7ca9e 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -325,6 +325,24 @@ void netif_carrier_off(struct net_device *dev)
 }
 EXPORT_SYMBOL(netif_carrier_off);
 
+/**
+ * 	netif_notify_peers - notify network peers about existence of @dev
+ * 	@dev: network device
+ *
+ * Generate traffic such that interested network peers are aware of
+ * @dev, such as by generating a gratuitous ARP. This may be used when
+ * a device wants to inform the rest of the network about some sort of
+ * reconfiguration such as a failover event or virtual machine
+ * migration.
+ */
+void netif_notify_peers(struct net_device *dev)
+{
+	rtnl_lock();
+	call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, dev);
+	rtnl_unlock();
+}
+EXPORT_SYMBOL(netif_notify_peers);
+
 /* "NOOP" scheduler: the best scheduler, recommended for all interfaces
    under all circumstances. It is difficult to invent anything faster or
    cheaper.
-- 
1.5.6.5

[PATCH 3/3] xen: netfront: explicitly generate arp_notify event after migration.

[Ian Campbell]

Use newly introduced netif_notify_peers() method to ensure a gratuitous ARP is
generated after a migration.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Stephen Hemminger <shemminger@linux-foundation.org>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: xen-devel@lists.xensource.com
Cc: stable@kernel.org
---
 drivers/net/xen-netfront.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index d504e2b..b50fedc 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -1621,6 +1621,7 @@ static void backend_changed(struct xenbus_device *dev,
 		if (xennet_connect(netdev) != 0)
 			break;
 		xenbus_switch_state(dev, XenbusStateConnected);
+		netif_notify_peers(netdev);
 		break;
 
 	case XenbusStateClosing:
-- 
1.5.6.5

Re: [PATCH 0/2] fixes to arp_notify for virtual machine migration use case

[David Miller]

From: Ian Campbell <Ian.Campbell@citrix.com>
Date: Wed, 26 May 2010 11:08:12 +0100

> Ian Campbell (3):
>       arp_notify: document that a gratuitous ARP request is sent when this option is enabled
>       arp_notify: allow drivers to explicitly request a notification event.
>       xen: netfront: explicitly generate arp_notify event after migration.

Applied to net-next-2.6, there was a new bonding notifier added meanwhile
and I changed the number of your new notifier to fix the conflict.