[PATCH v3 REPOST] xen-netfront: delay gARP until backend switches to Connected

[PATCH v3 REPOST] xen-netfront: delay gARP until backend switches to Connected

[Laszlo Ersek]

After a guest is live migrated, the xen-netfront driver emits a gratuitous
ARP message, so that networking hardware on the target host's subnet can
take notice, and public routing to the guest is re-established. However,
if the packet appears on the backend interface before the backend is added
to the target host's bridge, the packet is lost, and the migrated guest's
peers become unable to talk to the guest.

A sufficient two-parts condition to prevent the above is:

(1) ensure that the backend only moves to Connected xenbus state after its
hotplug scripts completed, ie. the netback interface got added to the
bridge; and

(2) ensure the frontend only queues the gARP when it sees the backend move
to Connected.

These two together provide complete ordering. Sub-condition (1) is
satisfied by pvops commit 43223efd9bfd.

In general, the full condition is sufficient, not necessary, because,
according to [1], live migration has been working for a long time without
satisfying sub-condition (2). However, after 43223efd9bfd was backported
to the RHEL-5 host to ensure (1), (2) still proved necessary in the RHEL-6
guest. This patch intends to provide (2) for upstream.

The Reviewed-by line comes from [2].

[1] http://old-list-archives.xen.org/xen-devel/2011-06/msg01969.html
[2] http://old-list-archives.xen.org/xen-devel/2011-07/msg00484.html

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/net/xen-netfront.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index d29365a..f033656 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -1646,7 +1646,6 @@ static void netback_changed(struct xenbus_device *dev,
 	case XenbusStateInitialised:
 	case XenbusStateReconfiguring:
 	case XenbusStateReconfigured:
-	case XenbusStateConnected:
 	case XenbusStateUnknown:
 	case XenbusStateClosed:
 		break;
@@ -1657,6 +1656,9 @@ static void netback_changed(struct xenbus_device *dev,
 		if (xennet_connect(netdev) != 0)
 			break;
 		xenbus_switch_state(dev, XenbusStateConnected);
+		break;
+
+	case XenbusStateConnected:
 		netif_notify_peers(netdev);
 		break;
 
-- 
1.7.4.4

Re: [PATCH v3 REPOST] xen-netfront: delay gARP until backend switches to Connected

[David Miller]

From: Laszlo Ersek <lersek@redhat.com>
Date: Fri,  9 Dec 2011 12:38:58 +0100

> These two together provide complete ordering. Sub-condition (1) is
> satisfied by pvops commit 43223efd9bfd.

I don't see this commit in Linus's tree, so I doubt it's valid for
me to apply this as a bug fix to my 'net' tree since the precondition
pvops commit isn't upstream as far as I can tell.

Where did you intend me to apply this patch, and how did you expect
the dependent commit to make it's way into the tree so that this
fix is complete?

BTW, you should always explicitly specificy the answers to all the
questions in the previous paragraph, otherwise (like right now) we
go back and forth wasting time establishing these facts.

The way to say which tree the patch is intended for is to specify
it in the Subject like, f.e. "[PATCH net-next v3 REPOST] ..."

Re: [PATCH v3 REPOST] xen-netfront: delay gARP until backend switches to Connected

[David Miller]

From: Laszlo Ersek <lersek@redhat.com>
Date: Fri,  9 Dec 2011 12:38:58 +0100

> These two together provide complete ordering. Sub-condition (1) is
> satisfied by pvops commit 43223efd9bfd.

I don't see this commit in Linus's tree, so I doubt it's valid for
me to apply this as a bug fix to my 'net' tree since the precondition
pvops commit isn't upstream as far as I can tell.

Where did you intend me to apply this patch, and how did you expect
the dependent commit to make it's way into the tree so that this
fix is complete?

BTW, you should always explicitly specificy the answers to all the
questions in the previous paragraph, otherwise (like right now) we
go back and forth wasting time establishing these facts.

The way to say which tree the patch is intended for is to specify
it in the Subject like, f.e. "[PATCH net-next v3 REPOST] ..."

Re: [PATCH v3 REPOST] xen-netfront: delay gARP until backend switches to Connected

[Ian Campbell]

On Fri, 2011-12-09 at 18:45 +0000, David Miller wrote:
> From: Laszlo Ersek <lersek@redhat.com>
> Date: Fri,  9 Dec 2011 12:38:58 +0100
> 
> > These two together provide complete ordering. Sub-condition (1) is
> > satisfied by pvops commit 43223efd9bfd.
> 
> I don't see this commit in Linus's tree,

The referenced commit is in
git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git#xen/next-2.6.32  some people call the "pvops tree" but there's no reason to expect someone outside the Xen world to know that...

A better reference would have been 6b0b80ca7165 in
git://xenbits.xen.org/people/ianc/linux-2.6.git#upstream/dom0/backend/netback-history which is the precise branch that was flattened to make f942dc2552b8, which is the upstream commit that added netback, so this change is already in upstream.

Ian.

Re: [PATCH v3 REPOST] xen-netfront: delay gARP until backend switches to Connected

[Ian Campbell]

On Fri, 2011-12-09 at 18:45 +0000, David Miller wrote:
> From: Laszlo Ersek <lersek@redhat.com>
> Date: Fri,  9 Dec 2011 12:38:58 +0100
> 
> > These two together provide complete ordering. Sub-condition (1) is
> > satisfied by pvops commit 43223efd9bfd.
> 
> I don't see this commit in Linus's tree,

The referenced commit is in
git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git#xen/next-2.6.32  some people call the "pvops tree" but there's no reason to expect someone outside the Xen world to know that...

A better reference would have been 6b0b80ca7165 in
git://xenbits.xen.org/people/ianc/linux-2.6.git#upstream/dom0/backend/netback-history which is the precise branch that was flattened to make f942dc2552b8, which is the upstream commit that added netback, so this change is already in upstream.

Ian.

Re: [PATCH v3 REPOST] xen-netfront: delay gARP until backend switches to Connected

[Ian Campbell]

On Fri, 2011-12-09 at 18:45 +0000, David Miller wrote:
> From: Laszlo Ersek <lersek@redhat.com>
> Date: Fri,  9 Dec 2011 12:38:58 +0100
> 
> > These two together provide complete ordering. Sub-condition (1) is
> > satisfied by pvops commit 43223efd9bfd.
> 
> I don't see this commit in Linus's tree,

The referenced commit is in
git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git#xen/next-2.6.32  some people call the "pvops tree" but there's no reason to expect someone outside the Xen world to know that...

A better reference would have been 6b0b80ca7165 in
git://xenbits.xen.org/people/ianc/linux-2.6.git#upstream/dom0/backend/netback-history which is the precise branch that was flattened to make f942dc2552b8, which is the upstream commit that added netback, so this change is already in upstream.

Ian.

Re: [PATCH v3 REPOST] xen-netfront: delay gARP until backend switches to Connected

[David Miller]

From: Ian Campbell <Ian.Campbell@citrix.com>
Date: Fri, 9 Dec 2011 21:23:00 +0000

> On Fri, 2011-12-09 at 18:45 +0000, David Miller wrote:
>> From: Laszlo Ersek <lersek@redhat.com>
>> Date: Fri,  9 Dec 2011 12:38:58 +0100
>> 
>> > These two together provide complete ordering. Sub-condition (1) is
>> > satisfied by pvops commit 43223efd9bfd.
>> 
>> I don't see this commit in Linus's tree,
> 
> The referenced commit is in
> git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git#xen/next-2.6.32  some people call the "pvops tree" but there's no reason to expect someone outside the Xen world to know that...
> 
> A better reference would have been 6b0b80ca7165 in
> git://xenbits.xen.org/people/ianc/linux-2.6.git#upstream/dom0/backend/netback-history which is the precise branch that was flattened to make f942dc2552b8, which is the upstream commit that added netback, so this change is already in upstream.

I want the commit message fixed so someone seeing the commit ID can
figure out what it actually refers to.

Re: [PATCH v3 REPOST] xen-netfront: delay gARP until backend switches to Connected

[David Miller]

From: Ian Campbell <Ian.Campbell@citrix.com>
Date: Fri, 9 Dec 2011 21:23:00 +0000

> On Fri, 2011-12-09 at 18:45 +0000, David Miller wrote:
>> From: Laszlo Ersek <lersek@redhat.com>
>> Date: Fri,  9 Dec 2011 12:38:58 +0100
>> 
>> > These two together provide complete ordering. Sub-condition (1) is
>> > satisfied by pvops commit 43223efd9bfd.
>> 
>> I don't see this commit in Linus's tree,
> 
> The referenced commit is in
> git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git#xen/next-2.6.32  some people call the "pvops tree" but there's no reason to expect someone outside the Xen world to know that...
> 
> A better reference would have been 6b0b80ca7165 in
> git://xenbits.xen.org/people/ianc/linux-2.6.git#upstream/dom0/backend/netback-history which is the precise branch that was flattened to make f942dc2552b8, which is the upstream commit that added netback, so this change is already in upstream.

I want the commit message fixed so someone seeing the commit ID can
figure out what it actually refers to.

[PATCH net-next v4] xen-netfront: delay gARP until backend switches to Connected

[Laszlo Ersek]

After a guest is live migrated, the xen-netfront driver emits a gratuitous
ARP message, so that networking hardware on the target host's subnet can
take notice, and public routing to the guest is re-established. However,
if the packet appears on the backend interface before the backend is added
to the target host's bridge, the packet is lost, and the migrated guest's
peers become unable to talk to the guest.

A sufficient two-parts condition to prevent the above is:

(1) ensure that the backend only moves to Connected xenbus state after its
hotplug scripts completed, ie. the netback interface got added to the
bridge; and

(2) ensure the frontend only queues the gARP when it sees the backend move
to Connected.

These two together provide complete ordering. Sub-condition (1) is already
satisfied by commit f942dc2552b8 in Linus' tree, based on commit
6b0b80ca7165 from [1].

In general, the full condition is sufficient, not necessary, because,
according to [2], live migration has been working for a long time without
satisfying sub-condition (2). However, after 6b0b80ca7165 was backported
to the RHEL-5 host to ensure (1), (2) still proved necessary in the RHEL-6
guest. This patch intends to provide (2) for upstream.

The Reviewed-by line comes from [3].

[1] git://xenbits.xen.org/people/ianc/linux-2.6.git#upstream/dom0/backend/netback-history
[2] http://old-list-archives.xen.org/xen-devel/2011-06/msg01969.html
[3] http://old-list-archives.xen.org/xen-devel/2011-07/msg00484.html

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/net/xen-netfront.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index d29365a..f033656 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -1646,7 +1646,6 @@ static void netback_changed(struct xenbus_device *dev,
 	case XenbusStateInitialised:
 	case XenbusStateReconfiguring:
 	case XenbusStateReconfigured:
-	case XenbusStateConnected:
 	case XenbusStateUnknown:
 	case XenbusStateClosed:
 		break;
@@ -1657,6 +1656,9 @@ static void netback_changed(struct xenbus_device *dev,
 		if (xennet_connect(netdev) != 0)
 			break;
 		xenbus_switch_state(dev, XenbusStateConnected);
+		break;
+
+	case XenbusStateConnected:
 		netif_notify_peers(netdev);
 		break;
 
-- 
1.7.4.4

Re: [PATCH net-next v4] xen-netfront: delay gARP until backend switches to Connected

[Jan Beulich]

>>> On 11.12.11 at 12:48, Laszlo Ersek <lersek@redhat.com> wrote:
> After a guest is live migrated, the xen-netfront driver emits a gratuitous
> ARP message, so that networking hardware on the target host's subnet can
> take notice, and public routing to the guest is re-established. However,
> if the packet appears on the backend interface before the backend is added
> to the target host's bridge, the packet is lost, and the migrated guest's
> peers become unable to talk to the guest.
> 
> A sufficient two-parts condition to prevent the above is:
> 
> (1) ensure that the backend only moves to Connected xenbus state after its
> hotplug scripts completed, ie. the netback interface got added to the
> bridge; and
> 
> (2) ensure the frontend only queues the gARP when it sees the backend move
> to Connected.
> 
> These two together provide complete ordering. Sub-condition (1) is already
> satisfied by commit f942dc2552b8 in Linus' tree, based on commit
> 6b0b80ca7165 from [1].
> 
> In general, the full condition is sufficient, not necessary, because,
> according to [2], live migration has been working for a long time without
> satisfying sub-condition (2). However, after 6b0b80ca7165 was backported
> to the RHEL-5 host to ensure (1), (2) still proved necessary in the RHEL-6
> guest. This patch intends to provide (2) for upstream.
> 
> The Reviewed-by line comes from [3].
> 
> [1] 
> git://xenbits.xen.org/people/ianc/linux-2.6.git#upstream/dom0/backend/netbac
> k-history
> [2] http://old-list-archives.xen.org/xen-devel/2011-06/msg01969.html 
> [3] http://old-list-archives.xen.org/xen-devel/2011-07/msg00484.html 
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Ian Campbell <ian.campbell@citrix.com>

Wouldn't all of this equally apply to the 2.6.18 kernel and its derivates?

Jan

> ---
>  drivers/net/xen-netfront.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index d29365a..f033656 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -1646,7 +1646,6 @@ static void netback_changed(struct xenbus_device *dev,
>  	case XenbusStateInitialised:
>  	case XenbusStateReconfiguring:
>  	case XenbusStateReconfigured:
> -	case XenbusStateConnected:
>  	case XenbusStateUnknown:
>  	case XenbusStateClosed:
>  		break;
> @@ -1657,6 +1656,9 @@ static void netback_changed(struct xenbus_device *dev,
>  		if (xennet_connect(netdev) != 0)
>  			break;
>  		xenbus_switch_state(dev, XenbusStateConnected);
> +		break;
> +
> +	case XenbusStateConnected:
>  		netif_notify_peers(netdev);
>  		break;
>  
> -- 
> 1.7.4.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com 
> http://lists.xensource.com/xen-devel 

Re: [PATCH net-next v4] xen-netfront: delay gARP until backend switches to Connected

[Laszlo Ersek]

On 01/02/12 15:50, Jan Beulich wrote:

> Wouldn't all of this equally apply to the 2.6.18 kernel and its derivates?

I haven't checked the linux-2.6.18-xen.hg source at all before because 
the (first) affected RHEL-5 netfront patch 
("linux-2.6-xen-xennet-coordinate-ARP-with-backend-network-status.patch") states,

     The frontend part of the patch is not applicable in upstream
     Linux as there is no gratuitous ARP packet support there at all.
     While the backend upstream is dead as far as I know.  So this
     is RHEL5-specific.

Regarding the RHEL-5 host side,

 >> (1) ensure that the backend only moves to Connected xenbus state
 >> after its hotplug scripts completed, ie. the netback interface got
 >> added to the bridge; and

 >> Sub-condition (1) is already satisfied by commit f942dc2552b8 in
 >> Linus' tree, based on commit 6b0b80ca7165 from [1].

 >> [1] 
git://xenbits.xen.org/people/ianc/linux-2.6.git#upstream/dom0/backend/netback-history

I figured the backend change is "already upstream". (Actually where I 
took it from was 43223efd9bfd in Jeremy's tree.)

So, what do you want me to do? I think I could get the frontend patch 
right without compiling linux-2.6.18-xen.hg. The backend is more messy 
however.

Thanks
Laszlo

Re: [PATCH net-next v4] xen-netfront: delay gARP until backend switches to Connected

[Jan Beulich]

>>> On 02.01.12 at 17:54, Laszlo Ersek <lersek@redhat.com> wrote:
> On 01/02/12 15:50, Jan Beulich wrote:
> 
>> Wouldn't all of this equally apply to the 2.6.18 kernel and its derivates?
> 
> I haven't checked the linux-2.6.18-xen.hg source at all before because 
> the (first) affected RHEL-5 netfront patch 
> ("linux-2.6-xen-xennet-coordinate-ARP-with-backend-network-status.patch") states,
> 
>      The frontend part of the patch is not applicable in upstream
>      Linux as there is no gratuitous ARP packet support there at all.
>      While the backend upstream is dead as far as I know.  So this
>      is RHEL5-specific.
> 
> Regarding the RHEL-5 host side,
> 
>  >> (1) ensure that the backend only moves to Connected xenbus state
>  >> after its hotplug scripts completed, ie. the netback interface got
>  >> added to the bridge; and
> 
>  >> Sub-condition (1) is already satisfied by commit f942dc2552b8 in
>  >> Linus' tree, based on commit 6b0b80ca7165 from [1].
> 
>  >> [1] 
> git://xenbits.xen.org/people/ianc/linux-2.6.git#upstream/dom0/backend/netback-
> history
> 
> I figured the backend change is "already upstream". (Actually where I 
> took it from was 43223efd9bfd in Jeremy's tree.)
> 
> So, what do you want me to do? I think I could get the frontend patch 
> right without compiling linux-2.6.18-xen.hg. The backend is more messy 
> however.

I've got both backend and frontend changes ready for that tree - all I
was really after was some double checking that both changes logically
apply to that tree.

Jan

Re: [PATCH net-next v4] xen-netfront: delay gARP until backend switches to Connected

[Laszlo Ersek]

On 01/02/12 18:04, Jan Beulich wrote:

> I've got both backend and frontend changes ready for that tree - all I
> was really after was some double checking that both changes logically
> apply to that tree.

I think (hope :)) so. In the RHEL-5 kernel they seem to work together fine.

Thanks
Laszlo

[PATCH v3 REPOST] xen-netfront: delay gARP until backend switches to Connected

[Laszlo Ersek]

After a guest is live migrated, the xen-netfront driver emits a gratuitous
ARP message, so that networking hardware on the target host's subnet can
take notice, and public routing to the guest is re-established. However,
if the packet appears on the backend interface before the backend is added
to the target host's bridge, the packet is lost, and the migrated guest's
peers become unable to talk to the guest.

A sufficient two-parts condition to prevent the above is:

(1) ensure that the backend only moves to Connected xenbus state after its
hotplug scripts completed, ie. the netback interface got added to the
bridge; and

(2) ensure the frontend only queues the gARP when it sees the backend move
to Connected.

These two together provide complete ordering. Sub-condition (1) is
satisfied by pvops commit 43223efd9bfd.

In general, the full condition is sufficient, not necessary, because,
according to [1], live migration has been working for a long time without
satisfying sub-condition (2). However, after 43223efd9bfd was backported
to the RHEL-5 host to ensure (1), (2) still proved necessary in the RHEL-6
guest. This patch intends to provide (2) for upstream.

The Reviewed-by line comes from [2].

[1] http://old-list-archives.xen.org/xen-devel/2011-06/msg01969.html
[2] http://old-list-archives.xen.org/xen-devel/2011-07/msg00484.html

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/net/xen-netfront.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index d29365a..f033656 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -1646,7 +1646,6 @@ static void netback_changed(struct xenbus_device *dev,
 	case XenbusStateInitialised:
 	case XenbusStateReconfiguring:
 	case XenbusStateReconfigured:
-	case XenbusStateConnected:
 	case XenbusStateUnknown:
 	case XenbusStateClosed:
 		break;
@@ -1657,6 +1656,9 @@ static void netback_changed(struct xenbus_device *dev,
 		if (xennet_connect(netdev) != 0)
 			break;
 		xenbus_switch_state(dev, XenbusStateConnected);
+		break;
+
+	case XenbusStateConnected:
 		netif_notify_peers(netdev);
 		break;
 
-- 
1.7.4.4