diff mbox series

[net-next] xen-netback: support dynamic unbind/bind

Message ID 20191223095923.2458-1-pdurrant@amazon.com
State Accepted
Commit 9476654bd5e8ad42abe8ee9f9e90069ff8e60c17
Headers show
Series [net-next] xen-netback: support dynamic unbind/bind | expand

Commit Message

Paul Durrant Dec. 23, 2019, 9:59 a.m. UTC
By re-attaching RX, TX, and CTL rings during connect() rather than
assuming they are freshly allocated (i.e. assuming the counters are zero),
and avoiding forcing state to Closed in netback_remove() it is possible
for vif instances to be unbound and re-bound from and to (respectively) a
running guest.

Dynamic unbind/bind is a highly useful feature for a backend module as it
allows it to be unloaded and re-loaded (i.e. updated) without requiring
domUs to be halted.

This has been tested by running iperf as a server in the test VM and
then running a client against it in a continuous loop, whilst also
running:

while true;
  do echo vif-$DOMID-$VIF >unbind;
  echo down;
  rmmod xen-netback;
  echo unloaded;
  modprobe xen-netback;
  cd $(pwd);
  brctl addif xenbr0 vif$DOMID.$VIF;
  ip link set vif$DOMID.$VIF up;
  echo up;
  sleep 5;
  done

in dom0 from /sys/bus/xen-backend/drivers/vif to continuously unbind,
unload, re-load, re-bind and re-plumb the backend.

Clearly a performance drop was seen but no TCP connection resets were
observed during this test and moreover a parallel SSH connection into the
guest remained perfectly usable throughout.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Paul Durrant <paul@xen.org>
Cc: "David S. Miller" <davem@davemloft.net>
---
 drivers/net/xen-netback/interface.c | 10 +++++++++-
 drivers/net/xen-netback/netback.c   | 20 +++++++++++++++++---
 drivers/net/xen-netback/xenbus.c    |  5 ++---
 3 files changed, 28 insertions(+), 7 deletions(-)

Comments

Wei Liu Dec. 23, 2019, 11:35 a.m. UTC | #1
On Mon, Dec 23, 2019 at 09:59:23AM +0000, Paul Durrant wrote:
[...] 
> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
> index f15ba3de6195..0c8a02a1ead7 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -585,6 +585,7 @@ int xenvif_connect_ctrl(struct xenvif *vif, grant_ref_t ring_ref,
>  	struct net_device *dev = vif->dev;
>  	void *addr;
>  	struct xen_netif_ctrl_sring *shared;
> +	RING_IDX rsp_prod, req_prod;
>  	int err;
>  
>  	err = xenbus_map_ring_valloc(xenvif_to_xenbus_device(vif),
> @@ -593,7 +594,14 @@ int xenvif_connect_ctrl(struct xenvif *vif, grant_ref_t ring_ref,
>  		goto err;
>  
>  	shared = (struct xen_netif_ctrl_sring *)addr;
> -	BACK_RING_INIT(&vif->ctrl, shared, XEN_PAGE_SIZE);
> +	rsp_prod = READ_ONCE(shared->rsp_prod);
> +	req_prod = READ_ONCE(shared->req_prod);
> +
> +	BACK_RING_ATTACH(&vif->ctrl, shared, rsp_prod, XEN_PAGE_SIZE);
> +
> +	err = -EIO;
> +	if (req_prod - rsp_prod > RING_SIZE(&vif->ctrl))
> +		goto err_unmap;

I think it makes more sense to attach the ring after this check has been
done, but I can see you want to structure code like this to reuse the
unmap error path.

So:

Reviewed-by: Wei Liu <wei.liu@kernel.org>

Nice work btw.
Paul Durrant Dec. 23, 2019, 11:46 a.m. UTC | #2
> -----Original Message-----
> From: Wei Liu <wei.liu@kernel.org>
> Sent: 23 December 2019 11:36
> To: Durrant, Paul <pdurrant@amazon.com>
> Cc: xen-devel@lists.xenproject.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Wei Liu <wei.liu@kernel.org>; Paul Durrant
> <paul@xen.org>; David S. Miller <davem@davemloft.net>
> Subject: Re: [PATCH net-next] xen-netback: support dynamic unbind/bind
> 
> On Mon, Dec 23, 2019 at 09:59:23AM +0000, Paul Durrant wrote:
> [...]
> > diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-
> netback/interface.c
> > index f15ba3de6195..0c8a02a1ead7 100644
> > --- a/drivers/net/xen-netback/interface.c
> > +++ b/drivers/net/xen-netback/interface.c
> > @@ -585,6 +585,7 @@ int xenvif_connect_ctrl(struct xenvif *vif,
> grant_ref_t ring_ref,
> >  	struct net_device *dev = vif->dev;
> >  	void *addr;
> >  	struct xen_netif_ctrl_sring *shared;
> > +	RING_IDX rsp_prod, req_prod;
> >  	int err;
> >
> >  	err = xenbus_map_ring_valloc(xenvif_to_xenbus_device(vif),
> > @@ -593,7 +594,14 @@ int xenvif_connect_ctrl(struct xenvif *vif,
> grant_ref_t ring_ref,
> >  		goto err;
> >
> >  	shared = (struct xen_netif_ctrl_sring *)addr;
> > -	BACK_RING_INIT(&vif->ctrl, shared, XEN_PAGE_SIZE);
> > +	rsp_prod = READ_ONCE(shared->rsp_prod);
> > +	req_prod = READ_ONCE(shared->req_prod);
> > +
> > +	BACK_RING_ATTACH(&vif->ctrl, shared, rsp_prod, XEN_PAGE_SIZE);
> > +
> > +	err = -EIO;
> > +	if (req_prod - rsp_prod > RING_SIZE(&vif->ctrl))
> > +		goto err_unmap;
> 
> I think it makes more sense to attach the ring after this check has been
> done, but I can see you want to structure code like this to reuse the
> unmap error path.

Looks a little odd, agreed. The reason I did it this way is so that I can use RING_SIZE() rather than having to use __RING_SIZE(); makes the code just a little bit shorter... which reminds me I ought to neaten up blkback similarly.

> 
> So:
> 
> Reviewed-by: Wei Liu <wei.liu@kernel.org>
> 
> Nice work btw.

Thanks :-)

  Paul
David Miller Dec. 26, 2019, 11:16 p.m. UTC | #3
From: Paul Durrant <pdurrant@amazon.com>
Date: Mon, 23 Dec 2019 09:59:23 +0000

> By re-attaching RX, TX, and CTL rings during connect() rather than
> assuming they are freshly allocated (i.e. assuming the counters are zero),
> and avoiding forcing state to Closed in netback_remove() it is possible
> for vif instances to be unbound and re-bound from and to (respectively) a
> running guest.
> 
> Dynamic unbind/bind is a highly useful feature for a backend module as it
> allows it to be unloaded and re-loaded (i.e. updated) without requiring
> domUs to be halted.
> 
> This has been tested by running iperf as a server in the test VM and
> then running a client against it in a continuous loop, whilst also
> running:
> 
> while true;
>   do echo vif-$DOMID-$VIF >unbind;
>   echo down;
>   rmmod xen-netback;
>   echo unloaded;
>   modprobe xen-netback;
>   cd $(pwd);
>   brctl addif xenbr0 vif$DOMID.$VIF;
>   ip link set vif$DOMID.$VIF up;
>   echo up;
>   sleep 5;
>   done
> 
> in dom0 from /sys/bus/xen-backend/drivers/vif to continuously unbind,
> unload, re-load, re-bind and re-plumb the backend.
> 
> Clearly a performance drop was seen but no TCP connection resets were
> observed during this test and moreover a parallel SSH connection into the
> guest remained perfectly usable throughout.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Applied, thank you.
diff mbox series

Patch

diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index f15ba3de6195..0c8a02a1ead7 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -585,6 +585,7 @@  int xenvif_connect_ctrl(struct xenvif *vif, grant_ref_t ring_ref,
 	struct net_device *dev = vif->dev;
 	void *addr;
 	struct xen_netif_ctrl_sring *shared;
+	RING_IDX rsp_prod, req_prod;
 	int err;
 
 	err = xenbus_map_ring_valloc(xenvif_to_xenbus_device(vif),
@@ -593,7 +594,14 @@  int xenvif_connect_ctrl(struct xenvif *vif, grant_ref_t ring_ref,
 		goto err;
 
 	shared = (struct xen_netif_ctrl_sring *)addr;
-	BACK_RING_INIT(&vif->ctrl, shared, XEN_PAGE_SIZE);
+	rsp_prod = READ_ONCE(shared->rsp_prod);
+	req_prod = READ_ONCE(shared->req_prod);
+
+	BACK_RING_ATTACH(&vif->ctrl, shared, rsp_prod, XEN_PAGE_SIZE);
+
+	err = -EIO;
+	if (req_prod - rsp_prod > RING_SIZE(&vif->ctrl))
+		goto err_unmap;
 
 	err = bind_interdomain_evtchn_to_irq(vif->domid, evtchn);
 	if (err < 0)
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 0020b2e8c279..315dfc6ea297 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1453,7 +1453,7 @@  int xenvif_map_frontend_data_rings(struct xenvif_queue *queue,
 	void *addr;
 	struct xen_netif_tx_sring *txs;
 	struct xen_netif_rx_sring *rxs;
-
+	RING_IDX rsp_prod, req_prod;
 	int err = -ENOMEM;
 
 	err = xenbus_map_ring_valloc(xenvif_to_xenbus_device(queue->vif),
@@ -1462,7 +1462,14 @@  int xenvif_map_frontend_data_rings(struct xenvif_queue *queue,
 		goto err;
 
 	txs = (struct xen_netif_tx_sring *)addr;
-	BACK_RING_INIT(&queue->tx, txs, XEN_PAGE_SIZE);
+	rsp_prod = READ_ONCE(txs->rsp_prod);
+	req_prod = READ_ONCE(txs->req_prod);
+
+	BACK_RING_ATTACH(&queue->tx, txs, rsp_prod, XEN_PAGE_SIZE);
+
+	err = -EIO;
+	if (req_prod - rsp_prod > RING_SIZE(&queue->tx))
+		goto err;
 
 	err = xenbus_map_ring_valloc(xenvif_to_xenbus_device(queue->vif),
 				     &rx_ring_ref, 1, &addr);
@@ -1470,7 +1477,14 @@  int xenvif_map_frontend_data_rings(struct xenvif_queue *queue,
 		goto err;
 
 	rxs = (struct xen_netif_rx_sring *)addr;
-	BACK_RING_INIT(&queue->rx, rxs, XEN_PAGE_SIZE);
+	rsp_prod = READ_ONCE(rxs->rsp_prod);
+	req_prod = READ_ONCE(rxs->req_prod);
+
+	BACK_RING_ATTACH(&queue->rx, rxs, rsp_prod, XEN_PAGE_SIZE);
+
+	err = -EIO;
+	if (req_prod - rsp_prod > RING_SIZE(&queue->rx))
+		goto err;
 
 	return 0;
 
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 17b4950ec051..286054b60d47 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -954,12 +954,10 @@  static int netback_remove(struct xenbus_device *dev)
 {
 	struct backend_info *be = dev_get_drvdata(&dev->dev);
 
-	set_backend_state(be, XenbusStateClosed);
-
 	unregister_hotplug_status_watch(be);
 	if (be->vif) {
 		kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
-		xen_unregister_watchers(be->vif);
+		backend_disconnect(be);
 		xenvif_free(be->vif);
 		be->vif = NULL;
 	}
@@ -1131,6 +1129,7 @@  static struct xenbus_driver netback_driver = {
 	.remove = netback_remove,
 	.uevent = netback_uevent,
 	.otherend_changed = frontend_changed,
+	.allow_rebind = true,
 };
 
 int xenvif_xenbus_init(void)