All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net v2] xen-netback: bookkeep number of active queues in our own module
@ 2014-06-22 13:31 Wei Liu
  2014-06-23  0:21 ` David Miller
  2014-06-23  0:21 ` David Miller
  0 siblings, 2 replies; 13+ messages in thread
From: Wei Liu @ 2014-06-22 13:31 UTC (permalink / raw)
  To: xen-devel, netdev; +Cc: boris.ostrovsky, Wei Liu, Ian Campbell

The original code uses netdev->real_num_tx_queues to bookkeep number of
queues and invokes netif_set_real_num_tx_queues to set the number of
queues. However, netif_set_real_num_tx_queues doesn't allow
real_num_tx_queues to be smaller than 1, which means setting the number
to 0 will not work and real_num_tx_queues is untouched.

This is bogus when xenvif_free is invoked before any number of queues is
allocated. That function needs to iterate through all queues to free
resources. Using the wrong number of queues results in NULL pointer
dereference.

So we bookkeep the number of queues in xen-netback to solve this
problem. This fixes a regression introduced by multiqueue patchset in
3.16-rc1.

Also remove xenvif_select_queue and leave queue selection to core
driver, as suggested by David Miller.

Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/net/xen-netback/common.h    |    1 +
 drivers/net/xen-netback/interface.c |   49 ++++++++---------------------------
 drivers/net/xen-netback/xenbus.c    |   28 ++++++++++----------
 3 files changed, 26 insertions(+), 52 deletions(-)

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 4dd7c4a..2532ce8 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -222,6 +222,7 @@ struct xenvif {
 
 	/* Queues */
 	struct xenvif_queue *queues;
+	unsigned int num_queues; /* active queues, resource allocated */
 
 	/* Miscellaneous private stuff. */
 	struct net_device *dev;
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index 852da34..9e97c7c 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -137,32 +137,11 @@ static void xenvif_wake_queue_callback(unsigned long data)
 	}
 }
 
-static u16 xenvif_select_queue(struct net_device *dev, struct sk_buff *skb,
-			       void *accel_priv, select_queue_fallback_t fallback)
-{
-	unsigned int num_queues = dev->real_num_tx_queues;
-	u32 hash;
-	u16 queue_index;
-
-	/* First, check if there is only one queue to optimise the
-	 * single-queue or old frontend scenario.
-	 */
-	if (num_queues == 1) {
-		queue_index = 0;
-	} else {
-		/* Use skb_get_hash to obtain an L4 hash if available */
-		hash = skb_get_hash(skb);
-		queue_index = hash % num_queues;
-	}
-
-	return queue_index;
-}
-
 static int xenvif_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct xenvif *vif = netdev_priv(dev);
 	struct xenvif_queue *queue = NULL;
-	unsigned int num_queues = dev->real_num_tx_queues;
+	unsigned int num_queues = vif->num_queues;
 	u16 index;
 	int min_slots_needed;
 
@@ -225,7 +204,7 @@ static struct net_device_stats *xenvif_get_stats(struct net_device *dev)
 {
 	struct xenvif *vif = netdev_priv(dev);
 	struct xenvif_queue *queue = NULL;
-	unsigned int num_queues = dev->real_num_tx_queues;
+	unsigned int num_queues = vif->num_queues;
 	unsigned long rx_bytes = 0;
 	unsigned long rx_packets = 0;
 	unsigned long tx_bytes = 0;
@@ -256,7 +235,7 @@ out:
 static void xenvif_up(struct xenvif *vif)
 {
 	struct xenvif_queue *queue = NULL;
-	unsigned int num_queues = vif->dev->real_num_tx_queues;
+	unsigned int num_queues = vif->num_queues;
 	unsigned int queue_index;
 
 	for (queue_index = 0; queue_index < num_queues; ++queue_index) {
@@ -272,7 +251,7 @@ static void xenvif_up(struct xenvif *vif)
 static void xenvif_down(struct xenvif *vif)
 {
 	struct xenvif_queue *queue = NULL;
-	unsigned int num_queues = vif->dev->real_num_tx_queues;
+	unsigned int num_queues = vif->num_queues;
 	unsigned int queue_index;
 
 	for (queue_index = 0; queue_index < num_queues; ++queue_index) {
@@ -379,7 +358,7 @@ static void xenvif_get_ethtool_stats(struct net_device *dev,
 				     struct ethtool_stats *stats, u64 * data)
 {
 	struct xenvif *vif = netdev_priv(dev);
-	unsigned int num_queues = dev->real_num_tx_queues;
+	unsigned int num_queues = vif->num_queues;
 	int i;
 	unsigned int queue_index;
 	struct xenvif_stats *vif_stats;
@@ -424,7 +403,6 @@ static const struct net_device_ops xenvif_netdev_ops = {
 	.ndo_fix_features = xenvif_fix_features,
 	.ndo_set_mac_address = eth_mac_addr,
 	.ndo_validate_addr   = eth_validate_addr,
-	.ndo_select_queue = xenvif_select_queue,
 };
 
 struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
@@ -438,7 +416,7 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
 	snprintf(name, IFNAMSIZ - 1, "vif%u.%u", domid, handle);
 	/* Allocate a netdev with the max. supported number of queues.
 	 * When the guest selects the desired number, it will be updated
-	 * via netif_set_real_num_tx_queues().
+	 * via netif_set_real_num_*_queues().
 	 */
 	dev = alloc_netdev_mq(sizeof(struct xenvif), name, ether_setup,
 			      xenvif_max_queues);
@@ -458,11 +436,9 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
 	vif->dev = dev;
 	vif->disabled = false;
 
-	/* Start out with no queues. The call below does not require
-	 * rtnl_lock() as it happens before register_netdev().
-	 */
+	/* Start out with no queues. */
 	vif->queues = NULL;
-	netif_set_real_num_tx_queues(dev, 0);
+	vif->num_queues = 0;
 
 	dev->netdev_ops	= &xenvif_netdev_ops;
 	dev->hw_features = NETIF_F_SG |
@@ -677,7 +653,7 @@ static void xenvif_wait_unmap_timeout(struct xenvif_queue *queue,
 void xenvif_disconnect(struct xenvif *vif)
 {
 	struct xenvif_queue *queue = NULL;
-	unsigned int num_queues = vif->dev->real_num_tx_queues;
+	unsigned int num_queues = vif->num_queues;
 	unsigned int queue_index;
 
 	if (netif_carrier_ok(vif->dev))
@@ -724,7 +700,7 @@ void xenvif_deinit_queue(struct xenvif_queue *queue)
 void xenvif_free(struct xenvif *vif)
 {
 	struct xenvif_queue *queue = NULL;
-	unsigned int num_queues = vif->dev->real_num_tx_queues;
+	unsigned int num_queues = vif->num_queues;
 	unsigned int queue_index;
 	/* Here we want to avoid timeout messages if an skb can be legitimately
 	 * stuck somewhere else. Realistically this could be an another vif's
@@ -748,12 +724,9 @@ void xenvif_free(struct xenvif *vif)
 		xenvif_deinit_queue(queue);
 	}
 
-	/* Free the array of queues. The call below does not require
-	 * rtnl_lock() because it happens after unregister_netdev().
-	 */
-	netif_set_real_num_tx_queues(vif->dev, 0);
 	vfree(vif->queues);
 	vif->queues = NULL;
+	vif->num_queues = 0;
 
 	free_netdev(vif->dev);
 
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 96c63dc2..3d85acd 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -527,9 +527,7 @@ static void connect(struct backend_info *be)
 	/* Use the number of queues requested by the frontend */
 	be->vif->queues = vzalloc(requested_num_queues *
 				  sizeof(struct xenvif_queue));
-	rtnl_lock();
-	netif_set_real_num_tx_queues(be->vif->dev, requested_num_queues);
-	rtnl_unlock();
+	be->vif->num_queues = requested_num_queues;
 
 	for (queue_index = 0; queue_index < requested_num_queues; ++queue_index) {
 		queue = &be->vif->queues[queue_index];
@@ -546,9 +544,7 @@ static void connect(struct backend_info *be)
 			 * earlier queues can be destroyed using the regular
 			 * disconnect logic.
 			 */
-			rtnl_lock();
-			netif_set_real_num_tx_queues(be->vif->dev, queue_index);
-			rtnl_unlock();
+			be->vif->num_queues = queue_index;
 			goto err;
 		}
 
@@ -561,13 +557,19 @@ static void connect(struct backend_info *be)
 			 * and also clean up any previously initialised queues.
 			 */
 			xenvif_deinit_queue(queue);
-			rtnl_lock();
-			netif_set_real_num_tx_queues(be->vif->dev, queue_index);
-			rtnl_unlock();
+			be->vif->num_queues = queue_index;
 			goto err;
 		}
 	}
 
+	/* Initialisation completed, tell core driver the number of
+	 * active queues.
+	 */
+	rtnl_lock();
+	netif_set_real_num_tx_queues(be->vif->dev, requested_num_queues);
+	netif_set_real_num_rx_queues(be->vif->dev, requested_num_queues);
+	rtnl_unlock();
+
 	xenvif_carrier_on(be->vif);
 
 	unregister_hotplug_status_watch(be);
@@ -582,13 +584,11 @@ static void connect(struct backend_info *be)
 	return;
 
 err:
-	if (be->vif->dev->real_num_tx_queues > 0)
+	if (be->vif->num_queues > 0)
 		xenvif_disconnect(be->vif); /* Clean up existing queues */
 	vfree(be->vif->queues);
 	be->vif->queues = NULL;
-	rtnl_lock();
-	netif_set_real_num_tx_queues(be->vif->dev, 0);
-	rtnl_unlock();
+	be->vif->num_queues = 0;
 	return;
 }
 
@@ -596,7 +596,7 @@ err:
 static int connect_rings(struct backend_info *be, struct xenvif_queue *queue)
 {
 	struct xenbus_device *dev = be->dev;
-	unsigned int num_queues = queue->vif->dev->real_num_tx_queues;
+	unsigned int num_queues = queue->vif->num_queues;
 	unsigned long tx_ring_ref, rx_ring_ref;
 	unsigned int tx_evtchn, rx_evtchn;
 	int err;
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH net v2] xen-netback: bookkeep number of active queues in our own module
  2014-06-22 13:31 [PATCH net v2] xen-netback: bookkeep number of active queues in our own module Wei Liu
  2014-06-23  0:21 ` David Miller
@ 2014-06-23  0:21 ` David Miller
  2014-06-23  8:40   ` Wei Liu
  1 sibling, 1 reply; 13+ messages in thread
From: David Miller @ 2014-06-23  0:21 UTC (permalink / raw)
  To: wei.liu2; +Cc: xen-devel, netdev, boris.ostrovsky, ian.campbell

From: Wei Liu <wei.liu2@citrix.com>
Date: Sun, 22 Jun 2014 14:31:41 +0100

>  
> +	/* Initialisation completed, tell core driver the number of
> +	 * active queues.
> +	 */
> +	rtnl_lock();
> +	netif_set_real_num_tx_queues(be->vif->dev, requested_num_queues);
> +	netif_set_real_num_rx_queues(be->vif->dev, requested_num_queues);
> +	rtnl_unlock();
> +
>  	xenvif_carrier_on(be->vif);

This function _NEVER_ set the number of RX queues beforehand,
therefore why are you adding an RX queue adjustment now?

Regardless of the reason, you must explain such a change, in
detail, in your commit message.

Your previous patch didn't do this, and I really am suspect as to
whether you functionally tested and verified this aspect of your
change at a ll.

Please don't "quietly" make undescribed changes like this. It's very
tiring to think that a patch has been adjusted to my liking and then
during review I find a grenade like this :-/

Thanks.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net v2] xen-netback: bookkeep number of active queues in our own module
  2014-06-22 13:31 [PATCH net v2] xen-netback: bookkeep number of active queues in our own module Wei Liu
@ 2014-06-23  0:21 ` David Miller
  2014-06-23  0:21 ` David Miller
  1 sibling, 0 replies; 13+ messages in thread
From: David Miller @ 2014-06-23  0:21 UTC (permalink / raw)
  To: wei.liu2; +Cc: netdev, boris.ostrovsky, ian.campbell, xen-devel

From: Wei Liu <wei.liu2@citrix.com>
Date: Sun, 22 Jun 2014 14:31:41 +0100

>  
> +	/* Initialisation completed, tell core driver the number of
> +	 * active queues.
> +	 */
> +	rtnl_lock();
> +	netif_set_real_num_tx_queues(be->vif->dev, requested_num_queues);
> +	netif_set_real_num_rx_queues(be->vif->dev, requested_num_queues);
> +	rtnl_unlock();
> +
>  	xenvif_carrier_on(be->vif);

This function _NEVER_ set the number of RX queues beforehand,
therefore why are you adding an RX queue adjustment now?

Regardless of the reason, you must explain such a change, in
detail, in your commit message.

Your previous patch didn't do this, and I really am suspect as to
whether you functionally tested and verified this aspect of your
change at a ll.

Please don't "quietly" make undescribed changes like this. It's very
tiring to think that a patch has been adjusted to my liking and then
during review I find a grenade like this :-/

Thanks.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net v2] xen-netback: bookkeep number of active queues in our own module
  2014-06-23  0:21 ` David Miller
@ 2014-06-23  8:40   ` Wei Liu
  2014-06-23  9:11     ` David Miller
                       ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Wei Liu @ 2014-06-23  8:40 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, boris.ostrovsky, wei.liu2, ian.campbell, xen-devel

On Sun, Jun 22, 2014 at 05:21:05PM -0700, David Miller wrote:
> From: Wei Liu <wei.liu2@citrix.com>
> Date: Sun, 22 Jun 2014 14:31:41 +0100
> 
> >  
> > +	/* Initialisation completed, tell core driver the number of
> > +	 * active queues.
> > +	 */
> > +	rtnl_lock();
> > +	netif_set_real_num_tx_queues(be->vif->dev, requested_num_queues);
> > +	netif_set_real_num_rx_queues(be->vif->dev, requested_num_queues);
> > +	rtnl_unlock();
> > +
> >  	xenvif_carrier_on(be->vif);
> 
> This function _NEVER_ set the number of RX queues beforehand,
> therefore why are you adding an RX queue adjustment now?
> 

As I went through the core driver code I found this to be a missing call
in previous code.

"In current Xen multiqueue design, the number of TX queues and RX queues
are in fact the same. So we need to set the numbers of TX and RX queues
to the same value."

Does the above paragraph answers your question? If so, I will add it to
commit message and resend this patch.

> Regardless of the reason, you must explain such a change, in
> detail, in your commit message.
> 
> Your previous patch didn't do this, and I really am suspect as to
> whether you functionally tested and verified this aspect of your
> change at a ll.
> 

I've done some testing.

> Please don't "quietly" make undescribed changes like this. It's very
> tiring to think that a patch has been adjusted to my liking and then
> during review I find a grenade like this :-/
> 

Sorry, that wasn't my intent.

Wei.

> Thanks.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net v2] xen-netback: bookkeep number of active queues in our own module
  2014-06-23  8:40   ` Wei Liu
  2014-06-23  9:11     ` David Miller
@ 2014-06-23  9:11     ` David Miller
  2014-06-23 15:31     ` Is: automated testing before David M picks them up? Was: Re: [Xen-devel] " Konrad Rzeszutek Wilk
  2014-06-23 15:31     ` Is: automated testing before David M picks them up? Was: " Konrad Rzeszutek Wilk
  3 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2014-06-23  9:11 UTC (permalink / raw)
  To: wei.liu2; +Cc: xen-devel, netdev, boris.ostrovsky, ian.campbell

From: Wei Liu <wei.liu2@citrix.com>
Date: Mon, 23 Jun 2014 09:40:32 +0100

> On Sun, Jun 22, 2014 at 05:21:05PM -0700, David Miller wrote:
>> From: Wei Liu <wei.liu2@citrix.com>
>> Date: Sun, 22 Jun 2014 14:31:41 +0100
>> 
>> >  
>> > +	/* Initialisation completed, tell core driver the number of
>> > +	 * active queues.
>> > +	 */
>> > +	rtnl_lock();
>> > +	netif_set_real_num_tx_queues(be->vif->dev, requested_num_queues);
>> > +	netif_set_real_num_rx_queues(be->vif->dev, requested_num_queues);
>> > +	rtnl_unlock();
>> > +
>> >  	xenvif_carrier_on(be->vif);
>> 
>> This function _NEVER_ set the number of RX queues beforehand,
>> therefore why are you adding an RX queue adjustment now?
>> 
> 
> As I went through the core driver code I found this to be a missing call
> in previous code.
> 
> "In current Xen multiqueue design, the number of TX queues and RX queues
> are in fact the same. So we need to set the numbers of TX and RX queues
> to the same value."
> 
> Does the above paragraph answers your question? If so, I will add it to
> commit message and resend this patch.

Yes.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net v2] xen-netback: bookkeep number of active queues in our own module
  2014-06-23  8:40   ` Wei Liu
@ 2014-06-23  9:11     ` David Miller
  2014-06-23  9:11     ` David Miller
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2014-06-23  9:11 UTC (permalink / raw)
  To: wei.liu2; +Cc: netdev, boris.ostrovsky, ian.campbell, xen-devel

From: Wei Liu <wei.liu2@citrix.com>
Date: Mon, 23 Jun 2014 09:40:32 +0100

> On Sun, Jun 22, 2014 at 05:21:05PM -0700, David Miller wrote:
>> From: Wei Liu <wei.liu2@citrix.com>
>> Date: Sun, 22 Jun 2014 14:31:41 +0100
>> 
>> >  
>> > +	/* Initialisation completed, tell core driver the number of
>> > +	 * active queues.
>> > +	 */
>> > +	rtnl_lock();
>> > +	netif_set_real_num_tx_queues(be->vif->dev, requested_num_queues);
>> > +	netif_set_real_num_rx_queues(be->vif->dev, requested_num_queues);
>> > +	rtnl_unlock();
>> > +
>> >  	xenvif_carrier_on(be->vif);
>> 
>> This function _NEVER_ set the number of RX queues beforehand,
>> therefore why are you adding an RX queue adjustment now?
>> 
> 
> As I went through the core driver code I found this to be a missing call
> in previous code.
> 
> "In current Xen multiqueue design, the number of TX queues and RX queues
> are in fact the same. So we need to set the numbers of TX and RX queues
> to the same value."
> 
> Does the above paragraph answers your question? If so, I will add it to
> commit message and resend this patch.

Yes.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Is: automated testing before David M picks them up? Was: Re: [Xen-devel] [PATCH net v2] xen-netback: bookkeep number of active queues in our own module
  2014-06-23  8:40   ` Wei Liu
  2014-06-23  9:11     ` David Miller
  2014-06-23  9:11     ` David Miller
@ 2014-06-23 15:31     ` Konrad Rzeszutek Wilk
  2014-06-27 19:34       ` Is: automated testing before David M picks them up? Was: " David Miller
  2014-06-27 19:34       ` Is: automated testing before David M picks them up? Was: Re: [Xen-devel] " David Miller
  2014-06-23 15:31     ` Is: automated testing before David M picks them up? Was: " Konrad Rzeszutek Wilk
  3 siblings, 2 replies; 13+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-06-23 15:31 UTC (permalink / raw)
  To: davem
  Cc: David Miller, netdev, boris.ostrovsky, ian.campbell, xen-devel, wei.liu2

> > Your previous patch didn't do this, and I really am suspect as to
> > whether you functionally tested and verified this aspect of your
> > change at a ll.
> > 
> 
> I've done some testing.
> 

Hey David M,

First of sorry for hijacking this thread - but it seemed like the
perfect thread.

David Vrabel raised an interesting point is that we do have an automated
build/test system that finds bugs - and in fact the 3.16-rc1 netfront
and netback bugs were discovered by that (Boris watches them like
hawks and pounces on them immediately).

That being said - all patches that go through the Xen tree (xen/tip.git)
go through that - and also the ones that maintainers do a git pull
on (say for drivers/block/*). But for xen-net* we don't do that since
well.. it hasn't been a problem in the past and we never formulated
a policy for that. (It was easy with Jens' because he likes the
GIT PULL mechanism and we just created a branch in the tree - and I also
managed to cause some embbarasing bugs so to save my face I am now
testing it religiously).

Having said that - how do you handle such situation with sub-maintainers
wanting to do some testing before they are cleared to go your way?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Is: automated testing before David M picks them up? Was: Re: [PATCH net v2] xen-netback: bookkeep number of active queues in our own module
  2014-06-23  8:40   ` Wei Liu
                       ` (2 preceding siblings ...)
  2014-06-23 15:31     ` Is: automated testing before David M picks them up? Was: Re: [Xen-devel] " Konrad Rzeszutek Wilk
@ 2014-06-23 15:31     ` Konrad Rzeszutek Wilk
  3 siblings, 0 replies; 13+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-06-23 15:31 UTC (permalink / raw)
  Cc: wei.liu2, ian.campbell, netdev, xen-devel, boris.ostrovsky, David Miller

> > Your previous patch didn't do this, and I really am suspect as to
> > whether you functionally tested and verified this aspect of your
> > change at a ll.
> > 
> 
> I've done some testing.
> 

Hey David M,

First of sorry for hijacking this thread - but it seemed like the
perfect thread.

David Vrabel raised an interesting point is that we do have an automated
build/test system that finds bugs - and in fact the 3.16-rc1 netfront
and netback bugs were discovered by that (Boris watches them like
hawks and pounces on them immediately).

That being said - all patches that go through the Xen tree (xen/tip.git)
go through that - and also the ones that maintainers do a git pull
on (say for drivers/block/*). But for xen-net* we don't do that since
well.. it hasn't been a problem in the past and we never formulated
a policy for that. (It was easy with Jens' because he likes the
GIT PULL mechanism and we just created a branch in the tree - and I also
managed to cause some embbarasing bugs so to save my face I am now
testing it religiously).

Having said that - how do you handle such situation with sub-maintainers
wanting to do some testing before they are cleared to go your way?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Is: automated testing before David M picks them up? Was: Re: [Xen-devel] [PATCH net v2] xen-netback: bookkeep number of active queues in our own module
  2014-06-23 15:31     ` Is: automated testing before David M picks them up? Was: Re: [Xen-devel] " Konrad Rzeszutek Wilk
  2014-06-27 19:34       ` Is: automated testing before David M picks them up? Was: " David Miller
@ 2014-06-27 19:34       ` David Miller
  2014-06-30  8:39         ` Is: automated testing before David M picks them up? Was: " Ian Campbell
  2014-06-30  8:39         ` Is: automated testing before David M picks them up? Was: Re: [Xen-devel] " Ian Campbell
  1 sibling, 2 replies; 13+ messages in thread
From: David Miller @ 2014-06-27 19:34 UTC (permalink / raw)
  To: konrad.wilk; +Cc: netdev, boris.ostrovsky, ian.campbell, xen-devel, wei.liu2

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Mon, 23 Jun 2014 11:31:51 -0400

> Having said that - how do you handle such situation with sub-maintainers
> wanting to do some testing before they are cleared to go your way?

Who cares, the bug gets found eventually right?

There is no value in my letting changes sit for more than a day or two that
aren't being actively discussed and look fine to me.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Is: automated testing before David M picks them up? Was: Re: [PATCH net v2] xen-netback: bookkeep number of active queues in our own module
  2014-06-23 15:31     ` Is: automated testing before David M picks them up? Was: Re: [Xen-devel] " Konrad Rzeszutek Wilk
@ 2014-06-27 19:34       ` David Miller
  2014-06-27 19:34       ` Is: automated testing before David M picks them up? Was: Re: [Xen-devel] " David Miller
  1 sibling, 0 replies; 13+ messages in thread
From: David Miller @ 2014-06-27 19:34 UTC (permalink / raw)
  To: konrad.wilk; +Cc: netdev, boris.ostrovsky, wei.liu2, ian.campbell, xen-devel

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Mon, 23 Jun 2014 11:31:51 -0400

> Having said that - how do you handle such situation with sub-maintainers
> wanting to do some testing before they are cleared to go your way?

Who cares, the bug gets found eventually right?

There is no value in my letting changes sit for more than a day or two that
aren't being actively discussed and look fine to me.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Is: automated testing before David M picks them up? Was: Re: [Xen-devel] [PATCH net v2] xen-netback: bookkeep number of active queues in our own module
  2014-06-27 19:34       ` Is: automated testing before David M picks them up? Was: Re: [Xen-devel] " David Miller
  2014-06-30  8:39         ` Is: automated testing before David M picks them up? Was: " Ian Campbell
@ 2014-06-30  8:39         ` Ian Campbell
  1 sibling, 0 replies; 13+ messages in thread
From: Ian Campbell @ 2014-06-30  8:39 UTC (permalink / raw)
  To: David Miller; +Cc: konrad.wilk, netdev, boris.ostrovsky, xen-devel, wei.liu2

On Fri, 2014-06-27 at 12:34 -0700, David Miller wrote:
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Mon, 23 Jun 2014 11:31:51 -0400
> 
> > Having said that - how do you handle such situation with sub-maintainers
> > wanting to do some testing before they are cleared to go your way?
> 
> Who cares, the bug gets found eventually right?
> 
> There is no value in my letting changes sit for more than a day or two that
> aren't being actively discussed and look fine to me.

So perhaps Konrad ought to be feeding net{,-next}.git to his testing
framework?

Ian.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Is: automated testing before David M picks them up? Was: Re: [PATCH net v2] xen-netback: bookkeep number of active queues in our own module
  2014-06-27 19:34       ` Is: automated testing before David M picks them up? Was: Re: [Xen-devel] " David Miller
@ 2014-06-30  8:39         ` Ian Campbell
  2014-06-30  8:39         ` Is: automated testing before David M picks them up? Was: Re: [Xen-devel] " Ian Campbell
  1 sibling, 0 replies; 13+ messages in thread
From: Ian Campbell @ 2014-06-30  8:39 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, boris.ostrovsky, xen-devel, wei.liu2

On Fri, 2014-06-27 at 12:34 -0700, David Miller wrote:
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Mon, 23 Jun 2014 11:31:51 -0400
> 
> > Having said that - how do you handle such situation with sub-maintainers
> > wanting to do some testing before they are cleared to go your way?
> 
> Who cares, the bug gets found eventually right?
> 
> There is no value in my letting changes sit for more than a day or two that
> aren't being actively discussed and look fine to me.

So perhaps Konrad ought to be feeding net{,-next}.git to his testing
framework?

Ian.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH net v2] xen-netback: bookkeep number of active queues in our own module
@ 2014-06-22 13:31 Wei Liu
  0 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2014-06-22 13:31 UTC (permalink / raw)
  To: xen-devel, netdev; +Cc: boris.ostrovsky, Wei Liu, Ian Campbell

The original code uses netdev->real_num_tx_queues to bookkeep number of
queues and invokes netif_set_real_num_tx_queues to set the number of
queues. However, netif_set_real_num_tx_queues doesn't allow
real_num_tx_queues to be smaller than 1, which means setting the number
to 0 will not work and real_num_tx_queues is untouched.

This is bogus when xenvif_free is invoked before any number of queues is
allocated. That function needs to iterate through all queues to free
resources. Using the wrong number of queues results in NULL pointer
dereference.

So we bookkeep the number of queues in xen-netback to solve this
problem. This fixes a regression introduced by multiqueue patchset in
3.16-rc1.

Also remove xenvif_select_queue and leave queue selection to core
driver, as suggested by David Miller.

Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/net/xen-netback/common.h    |    1 +
 drivers/net/xen-netback/interface.c |   49 ++++++++---------------------------
 drivers/net/xen-netback/xenbus.c    |   28 ++++++++++----------
 3 files changed, 26 insertions(+), 52 deletions(-)

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 4dd7c4a..2532ce8 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -222,6 +222,7 @@ struct xenvif {
 
 	/* Queues */
 	struct xenvif_queue *queues;
+	unsigned int num_queues; /* active queues, resource allocated */
 
 	/* Miscellaneous private stuff. */
 	struct net_device *dev;
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index 852da34..9e97c7c 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -137,32 +137,11 @@ static void xenvif_wake_queue_callback(unsigned long data)
 	}
 }
 
-static u16 xenvif_select_queue(struct net_device *dev, struct sk_buff *skb,
-			       void *accel_priv, select_queue_fallback_t fallback)
-{
-	unsigned int num_queues = dev->real_num_tx_queues;
-	u32 hash;
-	u16 queue_index;
-
-	/* First, check if there is only one queue to optimise the
-	 * single-queue or old frontend scenario.
-	 */
-	if (num_queues == 1) {
-		queue_index = 0;
-	} else {
-		/* Use skb_get_hash to obtain an L4 hash if available */
-		hash = skb_get_hash(skb);
-		queue_index = hash % num_queues;
-	}
-
-	return queue_index;
-}
-
 static int xenvif_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct xenvif *vif = netdev_priv(dev);
 	struct xenvif_queue *queue = NULL;
-	unsigned int num_queues = dev->real_num_tx_queues;
+	unsigned int num_queues = vif->num_queues;
 	u16 index;
 	int min_slots_needed;
 
@@ -225,7 +204,7 @@ static struct net_device_stats *xenvif_get_stats(struct net_device *dev)
 {
 	struct xenvif *vif = netdev_priv(dev);
 	struct xenvif_queue *queue = NULL;
-	unsigned int num_queues = dev->real_num_tx_queues;
+	unsigned int num_queues = vif->num_queues;
 	unsigned long rx_bytes = 0;
 	unsigned long rx_packets = 0;
 	unsigned long tx_bytes = 0;
@@ -256,7 +235,7 @@ out:
 static void xenvif_up(struct xenvif *vif)
 {
 	struct xenvif_queue *queue = NULL;
-	unsigned int num_queues = vif->dev->real_num_tx_queues;
+	unsigned int num_queues = vif->num_queues;
 	unsigned int queue_index;
 
 	for (queue_index = 0; queue_index < num_queues; ++queue_index) {
@@ -272,7 +251,7 @@ static void xenvif_up(struct xenvif *vif)
 static void xenvif_down(struct xenvif *vif)
 {
 	struct xenvif_queue *queue = NULL;
-	unsigned int num_queues = vif->dev->real_num_tx_queues;
+	unsigned int num_queues = vif->num_queues;
 	unsigned int queue_index;
 
 	for (queue_index = 0; queue_index < num_queues; ++queue_index) {
@@ -379,7 +358,7 @@ static void xenvif_get_ethtool_stats(struct net_device *dev,
 				     struct ethtool_stats *stats, u64 * data)
 {
 	struct xenvif *vif = netdev_priv(dev);
-	unsigned int num_queues = dev->real_num_tx_queues;
+	unsigned int num_queues = vif->num_queues;
 	int i;
 	unsigned int queue_index;
 	struct xenvif_stats *vif_stats;
@@ -424,7 +403,6 @@ static const struct net_device_ops xenvif_netdev_ops = {
 	.ndo_fix_features = xenvif_fix_features,
 	.ndo_set_mac_address = eth_mac_addr,
 	.ndo_validate_addr   = eth_validate_addr,
-	.ndo_select_queue = xenvif_select_queue,
 };
 
 struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
@@ -438,7 +416,7 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
 	snprintf(name, IFNAMSIZ - 1, "vif%u.%u", domid, handle);
 	/* Allocate a netdev with the max. supported number of queues.
 	 * When the guest selects the desired number, it will be updated
-	 * via netif_set_real_num_tx_queues().
+	 * via netif_set_real_num_*_queues().
 	 */
 	dev = alloc_netdev_mq(sizeof(struct xenvif), name, ether_setup,
 			      xenvif_max_queues);
@@ -458,11 +436,9 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
 	vif->dev = dev;
 	vif->disabled = false;
 
-	/* Start out with no queues. The call below does not require
-	 * rtnl_lock() as it happens before register_netdev().
-	 */
+	/* Start out with no queues. */
 	vif->queues = NULL;
-	netif_set_real_num_tx_queues(dev, 0);
+	vif->num_queues = 0;
 
 	dev->netdev_ops	= &xenvif_netdev_ops;
 	dev->hw_features = NETIF_F_SG |
@@ -677,7 +653,7 @@ static void xenvif_wait_unmap_timeout(struct xenvif_queue *queue,
 void xenvif_disconnect(struct xenvif *vif)
 {
 	struct xenvif_queue *queue = NULL;
-	unsigned int num_queues = vif->dev->real_num_tx_queues;
+	unsigned int num_queues = vif->num_queues;
 	unsigned int queue_index;
 
 	if (netif_carrier_ok(vif->dev))
@@ -724,7 +700,7 @@ void xenvif_deinit_queue(struct xenvif_queue *queue)
 void xenvif_free(struct xenvif *vif)
 {
 	struct xenvif_queue *queue = NULL;
-	unsigned int num_queues = vif->dev->real_num_tx_queues;
+	unsigned int num_queues = vif->num_queues;
 	unsigned int queue_index;
 	/* Here we want to avoid timeout messages if an skb can be legitimately
 	 * stuck somewhere else. Realistically this could be an another vif's
@@ -748,12 +724,9 @@ void xenvif_free(struct xenvif *vif)
 		xenvif_deinit_queue(queue);
 	}
 
-	/* Free the array of queues. The call below does not require
-	 * rtnl_lock() because it happens after unregister_netdev().
-	 */
-	netif_set_real_num_tx_queues(vif->dev, 0);
 	vfree(vif->queues);
 	vif->queues = NULL;
+	vif->num_queues = 0;
 
 	free_netdev(vif->dev);
 
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 96c63dc2..3d85acd 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -527,9 +527,7 @@ static void connect(struct backend_info *be)
 	/* Use the number of queues requested by the frontend */
 	be->vif->queues = vzalloc(requested_num_queues *
 				  sizeof(struct xenvif_queue));
-	rtnl_lock();
-	netif_set_real_num_tx_queues(be->vif->dev, requested_num_queues);
-	rtnl_unlock();
+	be->vif->num_queues = requested_num_queues;
 
 	for (queue_index = 0; queue_index < requested_num_queues; ++queue_index) {
 		queue = &be->vif->queues[queue_index];
@@ -546,9 +544,7 @@ static void connect(struct backend_info *be)
 			 * earlier queues can be destroyed using the regular
 			 * disconnect logic.
 			 */
-			rtnl_lock();
-			netif_set_real_num_tx_queues(be->vif->dev, queue_index);
-			rtnl_unlock();
+			be->vif->num_queues = queue_index;
 			goto err;
 		}
 
@@ -561,13 +557,19 @@ static void connect(struct backend_info *be)
 			 * and also clean up any previously initialised queues.
 			 */
 			xenvif_deinit_queue(queue);
-			rtnl_lock();
-			netif_set_real_num_tx_queues(be->vif->dev, queue_index);
-			rtnl_unlock();
+			be->vif->num_queues = queue_index;
 			goto err;
 		}
 	}
 
+	/* Initialisation completed, tell core driver the number of
+	 * active queues.
+	 */
+	rtnl_lock();
+	netif_set_real_num_tx_queues(be->vif->dev, requested_num_queues);
+	netif_set_real_num_rx_queues(be->vif->dev, requested_num_queues);
+	rtnl_unlock();
+
 	xenvif_carrier_on(be->vif);
 
 	unregister_hotplug_status_watch(be);
@@ -582,13 +584,11 @@ static void connect(struct backend_info *be)
 	return;
 
 err:
-	if (be->vif->dev->real_num_tx_queues > 0)
+	if (be->vif->num_queues > 0)
 		xenvif_disconnect(be->vif); /* Clean up existing queues */
 	vfree(be->vif->queues);
 	be->vif->queues = NULL;
-	rtnl_lock();
-	netif_set_real_num_tx_queues(be->vif->dev, 0);
-	rtnl_unlock();
+	be->vif->num_queues = 0;
 	return;
 }
 
@@ -596,7 +596,7 @@ err:
 static int connect_rings(struct backend_info *be, struct xenvif_queue *queue)
 {
 	struct xenbus_device *dev = be->dev;
-	unsigned int num_queues = queue->vif->dev->real_num_tx_queues;
+	unsigned int num_queues = queue->vif->num_queues;
 	unsigned long tx_ring_ref, rx_ring_ref;
 	unsigned int tx_evtchn, rx_evtchn;
 	int err;
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2014-06-30  8:39 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-22 13:31 [PATCH net v2] xen-netback: bookkeep number of active queues in our own module Wei Liu
2014-06-23  0:21 ` David Miller
2014-06-23  0:21 ` David Miller
2014-06-23  8:40   ` Wei Liu
2014-06-23  9:11     ` David Miller
2014-06-23  9:11     ` David Miller
2014-06-23 15:31     ` Is: automated testing before David M picks them up? Was: Re: [Xen-devel] " Konrad Rzeszutek Wilk
2014-06-27 19:34       ` Is: automated testing before David M picks them up? Was: " David Miller
2014-06-27 19:34       ` Is: automated testing before David M picks them up? Was: Re: [Xen-devel] " David Miller
2014-06-30  8:39         ` Is: automated testing before David M picks them up? Was: " Ian Campbell
2014-06-30  8:39         ` Is: automated testing before David M picks them up? Was: Re: [Xen-devel] " Ian Campbell
2014-06-23 15:31     ` Is: automated testing before David M picks them up? Was: " Konrad Rzeszutek Wilk
2014-06-22 13:31 Wei Liu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.