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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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