diff mbox

[net-next,V3,2/3] xen-netfront: split event channels support for Xen frontend driver

Message ID 1369240487-18834-3-git-send-email-wei.liu2@citrix.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Wei Liu May 22, 2013, 4:34 p.m. UTC
This patch adds a new feature called feature-split-event-channels for
netfront, enabling it to handle TX and RX events separately.

If netback does not support this feature, it falls back to use single event
channel.

If netfront fails to setup split event channels, it will try falling back to
single event channel.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/net/xen-netfront.c |  178 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 149 insertions(+), 29 deletions(-)

Comments

Annie.li May 22, 2013, 7:32 p.m. UTC | #1
On 2013-5-22 12:34, Wei Liu wrote:
> [...]
>   
> -static irqreturn_t xennet_interrupt(int irq, void *dev_id)
> +static irqreturn_t xennet_tx_interrupt(int irq, void *dev_id)
>   {
> -	struct net_device *dev = dev_id;
> -	struct netfront_info *np = netdev_priv(dev);
> +	struct netfront_info *np = dev_id;
> +	struct net_device *dev = np->netdev;
>   	unsigned long flags;
>   
>   	spin_lock_irqsave(&np->tx_lock, flags);
> +	xennet_tx_buf_gc(dev);
> +	spin_unlock_irqrestore(&np->tx_lock, flags);
>   
> -	if (likely(netif_carrier_ok(dev))) {
> -		xennet_tx_buf_gc(dev);
> -		/* Under tx_lock: protects access to rx shared-ring indexes. */
> -		if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx))
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t xennet_rx_interrupt(int irq, void *dev_id)
> +{
> +	struct netfront_info *np = dev_id;
> +	struct net_device *dev = np->netdev;
> +
> +	if (likely(netif_carrier_ok(dev) &&
> +		   RING_HAS_UNCONSUMED_RESPONSES(&np->rx)))
>   			napi_schedule(&np->napi);
> -	}
>   
> -	spin_unlock_irqrestore(&np->tx_lock, flags);

Originally, netfront protects access to rx shared-ring with tx_lock, you 
remove this protection here. It is better to protect the ring access by 
a sperate rx_lock then.

Thanks
Annie

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wei Liu May 22, 2013, 8:20 p.m. UTC | #2
On Wed, May 22, 2013 at 8:32 PM, annie li <annie.li@oracle.com> wrote:
>
>
> Originally, netfront protects access to rx shared-ring with tx_lock, you
> remove this protection here. It is better to protect the ring access by a
> sperate rx_lock then.
>

TX ring and RX ring are separate rings. I don't think that comment / code
makes sense any more. My stress test confirms that.


Wei.

> Thanks
> Annie
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Annie.li May 22, 2013, 8:35 p.m. UTC | #3
On 2013-5-22 16:20, Wei Liu wrote:
> On Wed, May 22, 2013 at 8:32 PM, annie li <annie.li@oracle.com> wrote:
>>
>> Originally, netfront protects access to rx shared-ring with tx_lock, you
>> remove this protection here. It is better to protect the ring access by a
>> sperate rx_lock then.
>>
> TX ring and RX ring are separate rings. I don't think that comment / code
> makes sense any more. My stress test confirms that.

Yes, they are separate rings. Actually I am not sure why 
RING_HAS_UNCONSUMED_RESPONSES(&np->rx) is protected by any tx_lock 
originally. But for xennet_rx_interrupt, it is better to use rx_lock to 
protect RING_HAS_UNCONSUMED_RESPONSES(&np->rx).

Thanks
Annie
>
>
> Wei.
>
>> Thanks
>> Annie
>>
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wei Liu May 22, 2013, 8:38 p.m. UTC | #4
On Wed, May 22, 2013 at 9:35 PM, annie li <annie.li@oracle.com> wrote:
>
> On 2013-5-22 16:20, Wei Liu wrote:
>>
>> On Wed, May 22, 2013 at 8:32 PM, annie li <annie.li@oracle.com> wrote:
>>>
>>>
>>> Originally, netfront protects access to rx shared-ring with tx_lock, you
>>> remove this protection here. It is better to protect the ring access by a
>>> sperate rx_lock then.
>>>
>> TX ring and RX ring are separate rings. I don't think that comment / code
>> makes sense any more. My stress test confirms that.
>
>
> Yes, they are separate rings. Actually I am not sure why
> RING_HAS_UNCONSUMED_RESPONSES(&np->rx) is protected by any tx_lock
> originally. But for xennet_rx_interrupt, it is better to use rx_lock to
> protect RING_HAS_UNCONSUMED_RESPONSES(&np->rx).
>

This doesn't make sense to me either. Xen ring protocol is designed to
be lock-free.
And in netfront's case there is no concurrent access to the ring.


Wei.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wei Liu May 23, 2013, 1:46 p.m. UTC | #5
On Wed, May 22, 2013 at 09:38:20PM +0100, Wei Liu wrote:
> On Wed, May 22, 2013 at 9:35 PM, annie li <annie.li@oracle.com> wrote:
> >
> > On 2013-5-22 16:20, Wei Liu wrote:
> >>
> >> On Wed, May 22, 2013 at 8:32 PM, annie li <annie.li@oracle.com> wrote:
> >>>
> >>>
> >>> Originally, netfront protects access to rx shared-ring with tx_lock, you
> >>> remove this protection here. It is better to protect the ring access by a
> >>> sperate rx_lock then.
> >>>
> >> TX ring and RX ring are separate rings. I don't think that comment / code
> >> makes sense any more. My stress test confirms that.
> >
> >
> > Yes, they are separate rings. Actually I am not sure why
> > RING_HAS_UNCONSUMED_RESPONSES(&np->rx) is protected by any tx_lock
> > originally. But for xennet_rx_interrupt, it is better to use rx_lock to
> > protect RING_HAS_UNCONSUMED_RESPONSES(&np->rx).
> >
> 
> This doesn't make sense to me either. Xen ring protocol is designed to
> be lock-free.
> And in netfront's case there is no concurrent access to the ring.
> 
> 

Annie, did I answer your questions / relieve your concern?


Wei.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Annie.li May 23, 2013, 2:14 p.m. UTC | #6
On 2013-5-23 9:46, Wei Liu wrote:
> On Wed, May 22, 2013 at 09:38:20PM +0100, Wei Liu wrote:
>> On Wed, May 22, 2013 at 9:35 PM, annie li <annie.li@oracle.com> wrote:
>>> On 2013-5-22 16:20, Wei Liu wrote:
>>>> On Wed, May 22, 2013 at 8:32 PM, annie li <annie.li@oracle.com> wrote:
>>>>>
>>>>> Originally, netfront protects access to rx shared-ring with tx_lock, you
>>>>> remove this protection here. It is better to protect the ring access by a
>>>>> sperate rx_lock then.
>>>>>
>>>> TX ring and RX ring are separate rings. I don't think that comment / code
>>>> makes sense any more. My stress test confirms that.
>>>
>>> Yes, they are separate rings. Actually I am not sure why
>>> RING_HAS_UNCONSUMED_RESPONSES(&np->rx) is protected by any tx_lock
>>> originally. But for xennet_rx_interrupt, it is better to use rx_lock to
>>> protect RING_HAS_UNCONSUMED_RESPONSES(&np->rx).
>>>
>> This doesn't make sense to me either. Xen ring protocol is designed to
>> be lock-free.
>> And in netfront's case there is no concurrent access to the ring.
>>
>>
> Annie, did I answer your questions / relieve your concern?

Yes, I think you are correct.

Thanks
Annie
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 5770e3b..62238a0 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -85,7 +85,15 @@  struct netfront_info {
 
 	struct napi_struct napi;
 
-	unsigned int evtchn;
+	/* Split event channels support, tx_* == rx_* when using
+	 * single event channel.
+	 */
+	unsigned int tx_evtchn, rx_evtchn;
+	unsigned int tx_irq, rx_irq;
+	/* Only used when split event channels support is enabled */
+	char tx_irq_name[IFNAMSIZ+4]; /* DEVNAME-tx */
+	char rx_irq_name[IFNAMSIZ+4]; /* DEVNAME-rx */
+
 	struct xenbus_device *xbdev;
 
 	spinlock_t   tx_lock;
@@ -330,7 +338,7 @@  no_skb:
  push:
 	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&np->rx, notify);
 	if (notify)
-		notify_remote_via_irq(np->netdev->irq);
+		notify_remote_via_irq(np->rx_irq);
 }
 
 static int xennet_open(struct net_device *dev)
@@ -623,7 +631,7 @@  static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&np->tx, notify);
 	if (notify)
-		notify_remote_via_irq(np->netdev->irq);
+		notify_remote_via_irq(np->tx_irq);
 
 	u64_stats_update_begin(&stats->syncp);
 	stats->tx_bytes += skb->len;
@@ -1254,23 +1262,35 @@  static int xennet_set_features(struct net_device *dev,
 	return 0;
 }
 
-static irqreturn_t xennet_interrupt(int irq, void *dev_id)
+static irqreturn_t xennet_tx_interrupt(int irq, void *dev_id)
 {
-	struct net_device *dev = dev_id;
-	struct netfront_info *np = netdev_priv(dev);
+	struct netfront_info *np = dev_id;
+	struct net_device *dev = np->netdev;
 	unsigned long flags;
 
 	spin_lock_irqsave(&np->tx_lock, flags);
+	xennet_tx_buf_gc(dev);
+	spin_unlock_irqrestore(&np->tx_lock, flags);
 
-	if (likely(netif_carrier_ok(dev))) {
-		xennet_tx_buf_gc(dev);
-		/* Under tx_lock: protects access to rx shared-ring indexes. */
-		if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx))
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t xennet_rx_interrupt(int irq, void *dev_id)
+{
+	struct netfront_info *np = dev_id;
+	struct net_device *dev = np->netdev;
+
+	if (likely(netif_carrier_ok(dev) &&
+		   RING_HAS_UNCONSUMED_RESPONSES(&np->rx)))
 			napi_schedule(&np->napi);
-	}
 
-	spin_unlock_irqrestore(&np->tx_lock, flags);
+	return IRQ_HANDLED;
+}
 
+static irqreturn_t xennet_interrupt(int irq, void *dev_id)
+{
+	xennet_tx_interrupt(irq, dev_id);
+	xennet_rx_interrupt(irq, dev_id);
 	return IRQ_HANDLED;
 }
 
@@ -1451,9 +1471,14 @@  static void xennet_disconnect_backend(struct netfront_info *info)
 	spin_unlock_irq(&info->tx_lock);
 	spin_unlock_bh(&info->rx_lock);
 
-	if (info->netdev->irq)
-		unbind_from_irqhandler(info->netdev->irq, info->netdev);
-	info->evtchn = info->netdev->irq = 0;
+	if (info->tx_irq && (info->tx_irq == info->rx_irq))
+		unbind_from_irqhandler(info->tx_irq, info);
+	if (info->tx_irq && (info->tx_irq != info->rx_irq)) {
+		unbind_from_irqhandler(info->tx_irq, info);
+		unbind_from_irqhandler(info->rx_irq, info);
+	}
+	info->tx_evtchn = info->rx_evtchn = 0;
+	info->tx_irq = info->rx_irq = 0;
 
 	/* End access and free the pages */
 	xennet_end_access(info->tx_ring_ref, info->tx.sring);
@@ -1503,12 +1528,82 @@  static int xen_net_read_mac(struct xenbus_device *dev, u8 mac[])
 	return 0;
 }
 
+static int setup_netfront_single(struct netfront_info *info)
+{
+	int err;
+
+	err = xenbus_alloc_evtchn(info->xbdev, &info->tx_evtchn);
+	if (err < 0)
+		goto fail;
+
+	err = bind_evtchn_to_irqhandler(info->tx_evtchn,
+					xennet_interrupt,
+					0, info->netdev->name, info);
+	if (err < 0)
+		goto bind_fail;
+	info->rx_evtchn = info->tx_evtchn;
+	info->rx_irq = info->tx_irq = err;
+
+	return 0;
+
+bind_fail:
+	xenbus_free_evtchn(info->xbdev, info->tx_evtchn);
+	info->tx_evtchn = 0;
+fail:
+	return err;
+}
+
+static int setup_netfront_split(struct netfront_info *info)
+{
+	int err;
+
+	err = xenbus_alloc_evtchn(info->xbdev, &info->tx_evtchn);
+	if (err < 0)
+		goto fail;
+	err = xenbus_alloc_evtchn(info->xbdev, &info->rx_evtchn);
+	if (err < 0)
+		goto alloc_rx_evtchn_fail;
+
+	snprintf(info->tx_irq_name, sizeof(info->tx_irq_name),
+		 "%s-tx", info->netdev->name);
+	err = bind_evtchn_to_irqhandler(info->tx_evtchn,
+					xennet_tx_interrupt,
+					0, info->tx_irq_name, info);
+	if (err < 0)
+		goto bind_tx_fail;
+	info->tx_irq = err;
+
+	snprintf(info->rx_irq_name, sizeof(info->rx_irq_name),
+		 "%s-rx", info->netdev->name);
+	err = bind_evtchn_to_irqhandler(info->rx_evtchn,
+					xennet_rx_interrupt,
+					0, info->rx_irq_name, info);
+	if (err < 0)
+		goto bind_rx_fail;
+	info->rx_irq = err;
+
+	return 0;
+
+bind_rx_fail:
+	unbind_from_irqhandler(info->tx_irq, info);
+	info->tx_irq = 0;
+bind_tx_fail:
+	xenbus_free_evtchn(info->xbdev, info->rx_evtchn);
+	info->rx_evtchn = 0;
+alloc_rx_evtchn_fail:
+	xenbus_free_evtchn(info->xbdev, info->tx_evtchn);
+	info->tx_evtchn = 0;
+fail:
+	return err;
+}
+
 static int setup_netfront(struct xenbus_device *dev, struct netfront_info *info)
 {
 	struct xen_netif_tx_sring *txs;
 	struct xen_netif_rx_sring *rxs;
 	int err;
 	struct net_device *netdev = info->netdev;
+	unsigned int feature_split_evtchn;
 
 	info->tx_ring_ref = GRANT_INVALID_REF;
 	info->rx_ring_ref = GRANT_INVALID_REF;
@@ -1516,6 +1611,12 @@  static int setup_netfront(struct xenbus_device *dev, struct netfront_info *info)
 	info->tx.sring = NULL;
 	netdev->irq = 0;
 
+	err = xenbus_scanf(XBT_NIL, info->xbdev->otherend,
+			   "feature-split-event-channels", "%u",
+			   &feature_split_evtchn);
+	if (err < 0)
+		feature_split_evtchn = 0;
+
 	err = xen_net_read_mac(dev, netdev->dev_addr);
 	if (err) {
 		xenbus_dev_fatal(dev, err, "parsing %s/mac", dev->nodename);
@@ -1550,22 +1651,23 @@  static int setup_netfront(struct xenbus_device *dev, struct netfront_info *info)
 		goto grant_rx_ring_fail;
 	info->rx_ring_ref = err;
 
-	err = xenbus_alloc_evtchn(dev, &info->evtchn);
+	if (feature_split_evtchn)
+		err = setup_netfront_split(info);
+	/* setup single event channel if
+	 *  a) feature-split-event-channels == 0
+	 *  b) feature-split-event-channels == 1 but failed to setup
+	 */
+	if (!feature_split_evtchn || (feature_split_evtchn && err))
+		err = setup_netfront_single(info);
+
 	if (err)
 		goto alloc_evtchn_fail;
 
-	err = bind_evtchn_to_irqhandler(info->evtchn, xennet_interrupt,
-					0, netdev->name, netdev);
-	if (err < 0)
-		goto bind_fail;
-	netdev->irq = err;
 	return 0;
 
 	/* If we fail to setup netfront, it is safe to just revoke access to
 	 * granted pages because backend is not accessing it at this point.
 	 */
-bind_fail:
-	xenbus_free_evtchn(dev, info->evtchn);
 alloc_evtchn_fail:
 	gnttab_end_foreign_access_ref(info->rx_ring_ref, 0);
 grant_rx_ring_fail:
@@ -1610,11 +1712,27 @@  again:
 		message = "writing rx ring-ref";
 		goto abort_transaction;
 	}
-	err = xenbus_printf(xbt, dev->nodename,
-			    "event-channel", "%u", info->evtchn);
-	if (err) {
-		message = "writing event-channel";
-		goto abort_transaction;
+
+	if (info->tx_evtchn == info->rx_evtchn) {
+		err = xenbus_printf(xbt, dev->nodename,
+				    "event-channel", "%u", info->tx_evtchn);
+		if (err) {
+			message = "writing event-channel";
+			goto abort_transaction;
+		}
+	} else {
+		err = xenbus_printf(xbt, dev->nodename,
+				    "event-channel-tx", "%u", info->tx_evtchn);
+		if (err) {
+			message = "writing event-channel-tx";
+			goto abort_transaction;
+		}
+		err = xenbus_printf(xbt, dev->nodename,
+				    "event-channel-rx", "%u", info->rx_evtchn);
+		if (err) {
+			message = "writing event-channel-rx";
+			goto abort_transaction;
+		}
 	}
 
 	err = xenbus_printf(xbt, dev->nodename, "request-rx-copy", "%u",
@@ -1727,7 +1845,9 @@  static int xennet_connect(struct net_device *dev)
 	 * packets.
 	 */
 	netif_carrier_on(np->netdev);
-	notify_remote_via_irq(np->netdev->irq);
+	notify_remote_via_irq(np->tx_irq);
+	if (np->tx_irq != np->rx_irq)
+		notify_remote_via_irq(np->rx_irq);
 	xennet_tx_buf_gc(dev);
 	xennet_alloc_rx_buffers(dev);