[PATCH 3.8-rc] net: mvneta: fix driver operation in SMP context

[PATCH 3.8-rc] net: mvneta: fix driver operation in SMP context

[Thomas Petazzoni]

From: Dmitri Epshtein <dima@marvell.com>

In order for the driver to behave properly in a SMP context, the same
transmit queue should be used by the kernel in dev_queue_xmit() and in
the driver's mvneta_tx() function. To achieve that, the driver now
implements the ->ndo_select_txq() operation.

For now, it always returns the same transmit queue, txq_def, until the
driver is expanded to properly take advantage of the multiqueue
capabilities of the hardware.

Without this patch, the network driver crashes the kernel almost
immediately on Armada XP platforms, if the network load is at least a
little bit parallel (i.e several threads).

[Thomas Petazzoni: reword commit message]
Signed-off-by: Dmitri Epshtein <dima@marvell.com>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
This is 3.8-rc material.
---
 drivers/net/ethernet/marvell/mvneta.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index b6025c3..af2c421 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -1310,6 +1310,17 @@ static u32 mvneta_skb_tx_csum(struct mvneta_port *pp, struct sk_buff *skb)
 	return MVNETA_TX_L4_CSUM_NOT;
 }
 
+static u16 mvneta_tx_policy(struct mvneta_port *pp, struct sk_buff *skb)
+{
+	return (u16)txq_def;
+}
+
+static u16 mvneta_select_txq(struct net_device *dev, struct sk_buff *skb)
+{
+	struct mvneta_port *pp = netdev_priv(dev);
+	return mvneta_tx_policy(pp, skb);
+}
+
 /* Returns rx queue pointer (find last set bit) according to causeRxTx
  * value
  */
@@ -1476,7 +1487,8 @@ error:
 static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
 {
 	struct mvneta_port *pp = netdev_priv(dev);
-	struct mvneta_tx_queue *txq = &pp->txqs[txq_def];
+	u16 txq_id = mvneta_tx_policy(pp, skb);
+	struct mvneta_tx_queue *txq = &pp->txqs[txq_id];
 	struct mvneta_tx_desc *tx_desc;
 	struct netdev_queue *nq;
 	int frags = 0;
@@ -1486,7 +1498,7 @@ static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
 		goto out;
 
 	frags = skb_shinfo(skb)->nr_frags + 1;
-	nq    = netdev_get_tx_queue(dev, txq_def);
+	nq    = netdev_get_tx_queue(dev, txq_id);
 
 	/* Get a descriptor for the first part of the packet */
 	tx_desc = mvneta_txq_next_desc_get(txq);
@@ -2550,6 +2562,7 @@ static const struct net_device_ops mvneta_netdev_ops = {
 	.ndo_change_mtu      = mvneta_change_mtu,
 	.ndo_tx_timeout      = mvneta_tx_timeout,
 	.ndo_get_stats64     = mvneta_get_stats64,
+	.ndo_select_queue    = mvneta_select_txq,
 };
 
 const struct ethtool_ops mvneta_eth_tool_ops = {
-- 
1.7.9.5

[PATCH 3.8-rc] net: mvneta: fix driver operation in SMP context

[Thomas Petazzoni]

From: Dmitri Epshtein <dima@marvell.com>

In order for the driver to behave properly in a SMP context, the same
transmit queue should be used by the kernel in dev_queue_xmit() and in
the driver's mvneta_tx() function. To achieve that, the driver now
implements the ->ndo_select_txq() operation.

For now, it always returns the same transmit queue, txq_def, until the
driver is expanded to properly take advantage of the multiqueue
capabilities of the hardware.

Without this patch, the network driver crashes the kernel almost
immediately on Armada XP platforms, if the network load is at least a
little bit parallel (i.e several threads).

[Thomas Petazzoni: reword commit message]
Signed-off-by: Dmitri Epshtein <dima@marvell.com>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
This is 3.8-rc material.
---
 drivers/net/ethernet/marvell/mvneta.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index b6025c3..af2c421 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -1310,6 +1310,17 @@ static u32 mvneta_skb_tx_csum(struct mvneta_port *pp, struct sk_buff *skb)
 	return MVNETA_TX_L4_CSUM_NOT;
 }
 
+static u16 mvneta_tx_policy(struct mvneta_port *pp, struct sk_buff *skb)
+{
+	return (u16)txq_def;
+}
+
+static u16 mvneta_select_txq(struct net_device *dev, struct sk_buff *skb)
+{
+	struct mvneta_port *pp = netdev_priv(dev);
+	return mvneta_tx_policy(pp, skb);
+}
+
 /* Returns rx queue pointer (find last set bit) according to causeRxTx
  * value
  */
@@ -1476,7 +1487,8 @@ error:
 static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
 {
 	struct mvneta_port *pp = netdev_priv(dev);
-	struct mvneta_tx_queue *txq = &pp->txqs[txq_def];
+	u16 txq_id = mvneta_tx_policy(pp, skb);
+	struct mvneta_tx_queue *txq = &pp->txqs[txq_id];
 	struct mvneta_tx_desc *tx_desc;
 	struct netdev_queue *nq;
 	int frags = 0;
@@ -1486,7 +1498,7 @@ static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
 		goto out;
 
 	frags = skb_shinfo(skb)->nr_frags + 1;
-	nq    = netdev_get_tx_queue(dev, txq_def);
+	nq    = netdev_get_tx_queue(dev, txq_id);
 
 	/* Get a descriptor for the first part of the packet */
 	tx_desc = mvneta_txq_next_desc_get(txq);
@@ -2550,6 +2562,7 @@ static const struct net_device_ops mvneta_netdev_ops = {
 	.ndo_change_mtu      = mvneta_change_mtu,
 	.ndo_tx_timeout      = mvneta_tx_timeout,
 	.ndo_get_stats64     = mvneta_get_stats64,
+	.ndo_select_queue    = mvneta_select_txq,
 };
 
 const struct ethtool_ops mvneta_eth_tool_ops = {
-- 
1.7.9.5

Re: [PATCH 3.8-rc] net: mvneta: fix driver operation in SMP context

[Jason Cooper]

On Mon, Jan 07, 2013 at 05:27:54PM +0100, Thomas Petazzoni wrote:
> From: Dmitri Epshtein <dima@marvell.com>
> 
> In order for the driver to behave properly in a SMP context, the same
> transmit queue should be used by the kernel in dev_queue_xmit() and in
> the driver's mvneta_tx() function. To achieve that, the driver now
> implements the ->ndo_select_txq() operation.
> 
> For now, it always returns the same transmit queue, txq_def, until the
> driver is expanded to properly take advantage of the multiqueue
> capabilities of the hardware.
> 
> Without this patch, the network driver crashes the kernel almost
> immediately on Armada XP platforms, if the network load is at least a
> little bit parallel (i.e several threads).
> 
> [Thomas Petazzoni: reword commit message]
> Signed-off-by: Dmitri Epshtein <dima@marvell.com>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Acked-by: Jason Cooper <jason@lakedaemon.net>

I'll assume this is going through -net, let me know if it should be
otherwise.

thx,

Jason.

[PATCH 3.8-rc] net: mvneta: fix driver operation in SMP context

[Jason Cooper]

On Mon, Jan 07, 2013 at 05:27:54PM +0100, Thomas Petazzoni wrote:
> From: Dmitri Epshtein <dima@marvell.com>
> 
> In order for the driver to behave properly in a SMP context, the same
> transmit queue should be used by the kernel in dev_queue_xmit() and in
> the driver's mvneta_tx() function. To achieve that, the driver now
> implements the ->ndo_select_txq() operation.
> 
> For now, it always returns the same transmit queue, txq_def, until the
> driver is expanded to properly take advantage of the multiqueue
> capabilities of the hardware.
> 
> Without this patch, the network driver crashes the kernel almost
> immediately on Armada XP platforms, if the network load is at least a
> little bit parallel (i.e several threads).
> 
> [Thomas Petazzoni: reword commit message]
> Signed-off-by: Dmitri Epshtein <dima@marvell.com>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Acked-by: Jason Cooper <jason@lakedaemon.net>

I'll assume this is going through -net, let me know if it should be
otherwise.

thx,

Jason.

Re: [PATCH 3.8-rc] net: mvneta: fix driver operation in SMP context

[Ben Hutchings]

On Mon, 2013-01-07 at 17:27 +0100, Thomas Petazzoni wrote:
> From: Dmitri Epshtein <dima@marvell.com>
> 
> In order for the driver to behave properly in a SMP context, the same
> transmit queue should be used by the kernel in dev_queue_xmit() and in
> the driver's mvneta_tx() function. To achieve that, the driver now
> implements the ->ndo_select_txq() operation.
>
> For now, it always returns the same transmit queue, txq_def, until the
> driver is expanded to properly take advantage of the multiqueue
> capabilities of the hardware.
[...]

Well it looks like the driver already sets up multiple TX queues, and
you just need to call skb_get_queue_mapping(skb) to look up the TX queue
in mvneta_tx().

Also since the numbers of queues are variable you should be calling
alloc_etherdev_mqs(sizeof(struct mvneta_port), txq_number, rxq_number)
instead of alloc_etherdev_mq(..., 8) in mvneta_probe().

If for some reason the current hardware initialisation doesn't actually
work for more than 1 TX queue then change the number you pass to
alloc_etherdev_mqs().

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH 3.8-rc] net: mvneta: fix driver operation in SMP context

[Ben Hutchings]

On Mon, 2013-01-07 at 17:27 +0100, Thomas Petazzoni wrote:
> From: Dmitri Epshtein <dima@marvell.com>
> 
> In order for the driver to behave properly in a SMP context, the same
> transmit queue should be used by the kernel in dev_queue_xmit() and in
> the driver's mvneta_tx() function. To achieve that, the driver now
> implements the ->ndo_select_txq() operation.
>
> For now, it always returns the same transmit queue, txq_def, until the
> driver is expanded to properly take advantage of the multiqueue
> capabilities of the hardware.
[...]

Well it looks like the driver already sets up multiple TX queues, and
you just need to call skb_get_queue_mapping(skb) to look up the TX queue
in mvneta_tx().

Also since the numbers of queues are variable you should be calling
alloc_etherdev_mqs(sizeof(struct mvneta_port), txq_number, rxq_number)
instead of alloc_etherdev_mq(..., 8) in mvneta_probe().

If for some reason the current hardware initialisation doesn't actually
work for more than 1 TX queue then change the number you pass to
alloc_etherdev_mqs().

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 3.8-rc] net: mvneta: fix driver operation in SMP context

[Gregory CLEMENT]

On 01/07/2013 05:27 PM, Thomas Petazzoni wrote:
> From: Dmitri Epshtein <dima@marvell.com>
> 
> In order for the driver to behave properly in a SMP context, the same
> transmit queue should be used by the kernel in dev_queue_xmit() and in
> the driver's mvneta_tx() function. To achieve that, the driver now
> implements the ->ndo_select_txq() operation.
> 
> For now, it always returns the same transmit queue, txq_def, until the
> driver is expanded to properly take advantage of the multiqueue
> capabilities of the hardware.
> 
> Without this patch, the network driver crashes the kernel almost
> immediately on Armada XP platforms, if the network load is at least a
> little bit parallel (i.e several threads).
> 
> [Thomas Petazzoni: reword commit message]
> Signed-off-by: Dmitri Epshtein <dima@marvell.com>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

I have tested this patch on my Armada XP DB board and on the
OpenBlocks AX3 board. I confirmed that it fixes the problem when
multiple thread are used.

You can add my
Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
and also my
Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Or a combination of the two if you prefer!

> ---
> This is 3.8-rc material.
> ---
>  drivers/net/ethernet/marvell/mvneta.c |   17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index b6025c3..af2c421 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -1310,6 +1310,17 @@ static u32 mvneta_skb_tx_csum(struct mvneta_port *pp, struct sk_buff *skb)
>  	return MVNETA_TX_L4_CSUM_NOT;
>  }
>  
> +static u16 mvneta_tx_policy(struct mvneta_port *pp, struct sk_buff *skb)
> +{
> +	return (u16)txq_def;
> +}
> +
> +static u16 mvneta_select_txq(struct net_device *dev, struct sk_buff *skb)
> +{
> +	struct mvneta_port *pp = netdev_priv(dev);
> +	return mvneta_tx_policy(pp, skb);
> +}
> +
>  /* Returns rx queue pointer (find last set bit) according to causeRxTx
>   * value
>   */
> @@ -1476,7 +1487,8 @@ error:
>  static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
>  {
>  	struct mvneta_port *pp = netdev_priv(dev);
> -	struct mvneta_tx_queue *txq = &pp->txqs[txq_def];
> +	u16 txq_id = mvneta_tx_policy(pp, skb);
> +	struct mvneta_tx_queue *txq = &pp->txqs[txq_id];
>  	struct mvneta_tx_desc *tx_desc;
>  	struct netdev_queue *nq;
>  	int frags = 0;
> @@ -1486,7 +1498,7 @@ static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
>  		goto out;
>  
>  	frags = skb_shinfo(skb)->nr_frags + 1;
> -	nq    = netdev_get_tx_queue(dev, txq_def);
> +	nq    = netdev_get_tx_queue(dev, txq_id);
>  
>  	/* Get a descriptor for the first part of the packet */
>  	tx_desc = mvneta_txq_next_desc_get(txq);
> @@ -2550,6 +2562,7 @@ static const struct net_device_ops mvneta_netdev_ops = {
>  	.ndo_change_mtu      = mvneta_change_mtu,
>  	.ndo_tx_timeout      = mvneta_tx_timeout,
>  	.ndo_get_stats64     = mvneta_get_stats64,
> +	.ndo_select_queue    = mvneta_select_txq,
>  };
>  
>  const struct ethtool_ops mvneta_eth_tool_ops = {
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 3.8-rc] net: mvneta: fix driver operation in SMP context

[Gregory CLEMENT]

On 01/07/2013 05:27 PM, Thomas Petazzoni wrote:
> From: Dmitri Epshtein <dima@marvell.com>
> 
> In order for the driver to behave properly in a SMP context, the same
> transmit queue should be used by the kernel in dev_queue_xmit() and in
> the driver's mvneta_tx() function. To achieve that, the driver now
> implements the ->ndo_select_txq() operation.
> 
> For now, it always returns the same transmit queue, txq_def, until the
> driver is expanded to properly take advantage of the multiqueue
> capabilities of the hardware.
> 
> Without this patch, the network driver crashes the kernel almost
> immediately on Armada XP platforms, if the network load is at least a
> little bit parallel (i.e several threads).
> 
> [Thomas Petazzoni: reword commit message]
> Signed-off-by: Dmitri Epshtein <dima@marvell.com>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

I have tested this patch on my Armada XP DB board and on the
OpenBlocks AX3 board. I confirmed that it fixes the problem when
multiple thread are used.

You can add my
Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
and also my
Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Or a combination of the two if you prefer!

> ---
> This is 3.8-rc material.
> ---
>  drivers/net/ethernet/marvell/mvneta.c |   17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index b6025c3..af2c421 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -1310,6 +1310,17 @@ static u32 mvneta_skb_tx_csum(struct mvneta_port *pp, struct sk_buff *skb)
>  	return MVNETA_TX_L4_CSUM_NOT;
>  }
>  
> +static u16 mvneta_tx_policy(struct mvneta_port *pp, struct sk_buff *skb)
> +{
> +	return (u16)txq_def;
> +}
> +
> +static u16 mvneta_select_txq(struct net_device *dev, struct sk_buff *skb)
> +{
> +	struct mvneta_port *pp = netdev_priv(dev);
> +	return mvneta_tx_policy(pp, skb);
> +}
> +
>  /* Returns rx queue pointer (find last set bit) according to causeRxTx
>   * value
>   */
> @@ -1476,7 +1487,8 @@ error:
>  static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
>  {
>  	struct mvneta_port *pp = netdev_priv(dev);
> -	struct mvneta_tx_queue *txq = &pp->txqs[txq_def];
> +	u16 txq_id = mvneta_tx_policy(pp, skb);
> +	struct mvneta_tx_queue *txq = &pp->txqs[txq_id];
>  	struct mvneta_tx_desc *tx_desc;
>  	struct netdev_queue *nq;
>  	int frags = 0;
> @@ -1486,7 +1498,7 @@ static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
>  		goto out;
>  
>  	frags = skb_shinfo(skb)->nr_frags + 1;
> -	nq    = netdev_get_tx_queue(dev, txq_def);
> +	nq    = netdev_get_tx_queue(dev, txq_id);
>  
>  	/* Get a descriptor for the first part of the packet */
>  	tx_desc = mvneta_txq_next_desc_get(txq);
> @@ -2550,6 +2562,7 @@ static const struct net_device_ops mvneta_netdev_ops = {
>  	.ndo_change_mtu      = mvneta_change_mtu,
>  	.ndo_tx_timeout      = mvneta_tx_timeout,
>  	.ndo_get_stats64     = mvneta_get_stats64,
> +	.ndo_select_queue    = mvneta_select_txq,
>  };
>  
>  const struct ethtool_ops mvneta_eth_tool_ops = {
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 3.8-rc] net: mvneta: fix driver operation in SMP context

[David Miller]

From: Jason Cooper <jason@lakedaemon.net>
Date: Mon, 7 Jan 2013 11:44:15 -0500

> I'll assume this is going through -net, let me know if it should be
> otherwise.

It needs to be fixed up before it can be applied anywhere, see
Ben Hutchings's feedback.

[PATCH 3.8-rc] net: mvneta: fix driver operation in SMP context

[David Miller]

From: Jason Cooper <jason@lakedaemon.net>
Date: Mon, 7 Jan 2013 11:44:15 -0500

> I'll assume this is going through -net, let me know if it should be
> otherwise.

It needs to be fixed up before it can be applied anywhere, see
Ben Hutchings's feedback.