| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1328006229.26983.343.camel@zakaz.uk.xensource.com>
Date: Tue, 31 Jan 2012 10:37:09 +0000
From: Ian Campbell <Ian.Campbell@...rix.com>
To: Wei Liu <wei.liu2@...rix.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"xen-devel@...ts.xensource.com" <xen-devel@...ts.xensource.com>,
"konrad.wilk@...cle.com" <konrad.wilk@...cle.com>
Subject: Re: [RFC PATCH V3 14/16] netback: split event channels support
On Mon, 2012-01-30 at 14:45 +0000, Wei Liu wrote:
> 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 <wei.liu2@...rix.com>
> ---
> 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.
> /* 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.
[...]
> @@ -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.
> + 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...
> + 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.
> + "%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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists