Re: [RFC PATCH V3 14/16] netback: split event channels support
From: Ian Campbell <hidden>
Date: 2012-01-31 10:37:11
Also in:
xen-devel
On Mon, 2012-01-30 at 14:45 +0000, Wei Liu wrote:
quoted hunk ↗ jump to hunk
Originally, netback and netfront only use one event channel to do tx / rx notification. This may cause unnecessary wake-up of NAPI / kthread. When guest tx is completed, netback will only notify tx_irq. Also modify xenvif_protocol0 to reflect this change. Rx protocol only notifies rx_irq. If split-event-channels feature is not activated, rx_irq = tx_irq, so RX protocol will just work as expected. Signed-off-by: Wei Liu <redacted> --- drivers/net/xen-netback/common.h | 9 ++- drivers/net/xen-netback/interface.c | 90 ++++++++++++++++++++----- drivers/net/xen-netback/netback.c | 2 +- drivers/net/xen-netback/xenbus.c | 52 ++++++++++++--- drivers/net/xen-netback/xenvif_rx_protocol0.c | 2 +- 5 files changed, 123 insertions(+), 32 deletions(-)diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h index f3d95b3..376f0bf 100644 --- a/drivers/net/xen-netback/common.h +++ b/drivers/net/xen-netback/common.h@@ -100,8 +100,10 @@ struct xenvif { u8 fe_dev_addr[6]; - /* Physical parameters of the comms window. */ - unsigned int irq; + /* when split_irq == 0, only use tx_irq */ + int split_irq; + unsigned int tx_irq; + unsigned int rx_irq;
Can you get rid of split_irq by setting tx_irq == rx_irq in that case
and simplify the code by doing so?
I think this should work even for places like:
if (!vif->split_irq)
enable_irq(vif->tx_irq);
else {
enable_irq(vif->tx_irq);
enable_irq(vif->rx_irq);
}
Just by doing
enable_irq(vif->tx_irq);
enable_irq(vif->rx_irq);
Since enable/disable_irq maintain a count and so it will do the right
thing if they happen to be the same.
quoted hunk ↗ jump to hunk
/* The shared tx ring and index. */ struct xen_netif_tx_back_ring tx;@@ -162,7 +164,8 @@ struct xenvif *xenvif_alloc(struct device *parent, int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref[], unsigned int tx_ring_order, unsigned long rx_ring_ref[], unsigned int rx_ring_order, - unsigned int evtchn, unsigned int rx_protocol); + unsigned int evtchn[], int split_evtchn, + unsigned int rx_protocol); void xenvif_disconnect(struct xenvif *vif); int xenvif_xenbus_init(void);diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c index 0f05f03..afccd5d 100644 --- a/drivers/net/xen-netback/interface.c +++ b/drivers/net/xen-netback/interface.c@@ -46,15 +46,31 @@ int xenvif_schedulable(struct xenvif *vif) return netif_running(vif->dev) && netif_carrier_ok(vif->dev); } -static irqreturn_t xenvif_interrupt(int irq, void *dev_id) +static irqreturn_t xenvif_tx_interrupt(int irq, void *dev_id) +{ + struct xenvif *vif = dev_id; + + if (RING_HAS_UNCONSUMED_REQUESTS(&vif->tx)) + napi_schedule(&vif->napi); + + return IRQ_HANDLED; +} + +static irqreturn_t xenvif_rx_interrupt(int irq, void *dev_id) { struct xenvif *vif = dev_id; if (xenvif_schedulable(vif) && vif->event != NULL) vif->event(vif); - if (RING_HAS_UNCONSUMED_REQUESTS(&vif->tx)) - napi_schedule(&vif->napi); + return IRQ_HANDLED; +} + +static irqreturn_t xenvif_interrupt(int irq, void *dev_id) +{ + xenvif_tx_interrupt(0, dev_id);
Might as well pass irq down. [...]
quoted hunk ↗ jump to hunk
@@ -308,13 +334,14 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid, int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref[], unsigned int tx_ring_ref_count, unsigned long rx_ring_ref[], unsigned int rx_ring_ref_count, - unsigned int evtchn, unsigned int rx_protocol) + unsigned int evtchn[], int split_evtchn,
Explicitly tx_evtchn and rx_evtchn would be clearer than remembering that [0]==tx and [1]==rx I think.
quoted hunk ↗ jump to hunk
+ unsigned int rx_protocol) { int err = -ENOMEM; struct xen_netif_tx_sring *txs; /* Already connected through? */ - if (vif->irq) + if (vif->tx_irq) return 0; __module_get(THIS_MODULE);@@ -345,13 +372,35 @@ int xenvif_connect(struct xenvif *vif, if (vif->setup(vif)) goto err_rx_unmap; - err = bind_interdomain_evtchn_to_irqhandler( - vif->domid, evtchn, xenvif_interrupt, 0, - vif->dev->name, vif); - if (err < 0) - goto err_rx_unmap; - vif->irq = err; - disable_irq(vif->irq); + if (!split_evtchn) {
Presumably this is one of the places where you do have to care about split vs non. I did consider whether simply registering two handlers for the interrupt in a shared-interrupt style would work, but I think that way lies madness and confusion...
quoted hunk ↗ jump to hunk
+ err = bind_interdomain_evtchn_to_irqhandler( + vif->domid, evtchn[0], xenvif_interrupt, 0, + vif->dev->name, vif); + if (err < 0) + goto err_rx_unmap; + vif->tx_irq = vif->rx_irq = err; + disable_irq(vif->tx_irq); + vif->split_irq = 0; + } else { + err = bind_interdomain_evtchn_to_irqhandler( + vif->domid, evtchn[0], xenvif_tx_interrupt, + 0, vif->dev->name, vif); + if (err < 0) + goto err_rx_unmap; + vif->tx_irq = err; + disable_irq(vif->tx_irq); + + err = bind_interdomain_evtchn_to_irqhandler( + vif->domid, evtchn[1], xenvif_rx_interrupt, + 0, vif->dev->name, vif); + if (err < 0) { + unbind_from_irqhandler(vif->tx_irq, vif); + goto err_rx_unmap; + } + vif->rx_irq = err; + disable_irq(vif->rx_irq); + vif->split_irq = 1; + } init_waitqueue_head(&vif->wq); vif->task = kthread_create(xenvif_kthread,diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c index 4067286..c5a3b27 100644 --- a/drivers/net/xen-netback/xenbus.c +++ b/drivers/net/xen-netback/xenbus.c@@ -131,6 +131,14 @@ static int netback_probe(struct xenbus_device *dev, goto abort_transaction; } + err = xenbus_printf(xbt, dev->nodename, + "split-event-channels",
Usually we use "feature-FOO" as the names for these sorts of nodes.
quoted hunk ↗ jump to hunk
+ "%u", 1); + if (err) { + message = "writing split-event-channels"; + goto abort_transaction; + } + err = xenbus_transaction_end(xbt, 0); } while (err == -EAGAIN);@@ -408,7 +416,7 @@ static int connect_rings(struct backend_info *be) { struct xenvif *vif = be->vif; struct xenbus_device *dev = be->dev; - unsigned int evtchn, rx_copy; + unsigned int evtchn[2], split_evtchn, rx_copy;
Another case where I think two vars is better than a small array.
int err; int val; unsigned long tx_ring_ref[NETBK_MAX_RING_PAGES];
Ian.