| Message ID | 20191223095923.2458-1-pdurrant@amazon.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9476654bd5e8ad42abe8ee9f9e90069ff8e60c17 |
| Headers | show |
| Series | [net-next] xen-netback: support dynamic unbind/bind | expand |
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.
> -----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
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 --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)
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(-)