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 <[email protected]>
---
Cc: Wei Liu <[email protected]>
Cc: Paul Durrant <[email protected]>
Cc: "David S. Miller" <[email protected]>
---
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(-)
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)
--
2.20.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 <[email protected]>
Nice work btw.
> -----Original Message-----
> From: Wei Liu <[email protected]>
> Sent: 23 December 2019 11:36
> To: Durrant, Paul <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; Wei Liu <[email protected]>; Paul Durrant
> <[email protected]>; David S. Miller <[email protected]>
> 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 <[email protected]>
>
> Nice work btw.
Thanks :-)
Paul
From: Paul Durrant <[email protected]>
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 <[email protected]>
Applied, thank you.