[dpdk-dev] [PATCH] fm10k: fix a crash bug when quit from testpmd

[dpdk-dev] [PATCH] fm10k: fix a crash bug when quit from testpmd

[Chen Jing D(Mark)]

From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>

When the fm10k port is closed, both func tx_queue_clean() and
fm10k_tx_queue_release_mbufs_vec() will try to release buffer in
SW ring. The latter func won't do sanity check on those pointers
and cause crash.

The fix include 2 parts.
1. Remove Vector TX buffer release func since it can share the
   release functions with regular TX.
2. Add log to print out what actual Rx/Tx func is used.

Signed-off-by: Chen Jing D(Mark) <jing.d.chen@intel.com>
---
 drivers/net/fm10k/fm10k.h          |    1 -
 drivers/net/fm10k/fm10k_ethdev.c   |   17 ++++++++++++-----
 drivers/net/fm10k/fm10k_rxtx_vec.c |   28 ----------------------------
 3 files changed, 12 insertions(+), 34 deletions(-)

diff --git a/drivers/net/fm10k/fm10k.h b/drivers/net/fm10k/fm10k.h
index 754aa6a..38d5489 100644
--- a/drivers/net/fm10k/fm10k.h
+++ b/drivers/net/fm10k/fm10k.h
@@ -237,7 +237,6 @@ struct fm10k_tx_queue {
 };
 
 struct fm10k_txq_ops {
-	void (*release_mbufs)(struct fm10k_tx_queue *txq);
 	void (*reset)(struct fm10k_tx_queue *txq);
 };
 
diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
index cf7ada7..af7b0c2 100644
--- a/drivers/net/fm10k/fm10k_ethdev.c
+++ b/drivers/net/fm10k/fm10k_ethdev.c
@@ -386,7 +386,6 @@ fm10k_check_mq_mode(struct rte_eth_dev *dev)
 }
 
 static const struct fm10k_txq_ops def_txq_ops = {
-	.release_mbufs = tx_queue_free,
 	.reset = tx_queue_reset,
 };
 
@@ -1073,7 +1072,7 @@ fm10k_dev_queue_release(struct rte_eth_dev *dev)
 		for (i = 0; i < dev->data->nb_tx_queues; i++) {
 			struct fm10k_tx_queue *txq = dev->data->tx_queues[i];
 
-			txq->ops->release_mbufs(txq);
+			tx_queue_free(txq);
 		}
 	}
 
@@ -1793,7 +1792,7 @@ fm10k_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_id,
 	if (dev->data->tx_queues[queue_id] != NULL) {
 		struct fm10k_tx_queue *txq = dev->data->tx_queues[queue_id];
 
-		txq->ops->release_mbufs(txq);
+		tx_queue_free(txq);
 		dev->data->tx_queues[queue_id] = NULL;
 	}
 
@@ -1872,7 +1871,7 @@ fm10k_tx_queue_release(void *queue)
 	struct fm10k_tx_queue *q = queue;
 	PMD_INIT_FUNC_TRACE();
 
-	q->ops->release_mbufs(q);
+	tx_queue_free(q);
 }
 
 static int
@@ -2439,13 +2438,16 @@ fm10k_set_tx_function(struct rte_eth_dev *dev)
 	}
 
 	if (use_sse) {
+		PMD_INIT_LOG(ERR, "Use vector Tx func");
 		for (i = 0; i < dev->data->nb_tx_queues; i++) {
 			txq = dev->data->tx_queues[i];
 			fm10k_txq_vec_setup(txq);
 		}
 		dev->tx_pkt_burst = fm10k_xmit_pkts_vec;
-	} else
+	} else {
 		dev->tx_pkt_burst = fm10k_xmit_pkts;
+		PMD_INIT_LOG(ERR, "Use regular Tx func");
+	}
 }
 
 static void __attribute__((cold))
@@ -2469,6 +2471,11 @@ fm10k_set_rx_function(struct rte_eth_dev *dev)
 		(dev->rx_pkt_burst == fm10k_recv_scattered_pkts_vec ||
 		dev->rx_pkt_burst == fm10k_recv_pkts_vec);
 
+	if (rx_using_sse)
+		PMD_INIT_LOG(ERR, "Use vector Rx func");
+	else
+		PMD_INIT_LOG(ERR, "Use regular Rx func");
+
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		struct fm10k_rx_queue *rxq = dev->data->rx_queues[i];
 
diff --git a/drivers/net/fm10k/fm10k_rxtx_vec.c b/drivers/net/fm10k/fm10k_rxtx_vec.c
index 06beca9..6042568 100644
--- a/drivers/net/fm10k/fm10k_rxtx_vec.c
+++ b/drivers/net/fm10k/fm10k_rxtx_vec.c
@@ -45,8 +45,6 @@
 #endif
 
 static void
-fm10k_tx_queue_release_mbufs_vec(struct fm10k_tx_queue *txq);
-static void
 fm10k_reset_tx_queue(struct fm10k_tx_queue *txq);
 
 /* Handling the offload flags (olflags) field takes computation
@@ -634,7 +632,6 @@ fm10k_recv_scattered_pkts_vec(void *rx_queue,
 }
 
 static const struct fm10k_txq_ops vec_txq_ops = {
-	.release_mbufs = fm10k_tx_queue_release_mbufs_vec,
 	.reset = fm10k_reset_tx_queue,
 };
 
@@ -795,31 +792,6 @@ fm10k_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
 }
 
 static void __attribute__((cold))
-fm10k_tx_queue_release_mbufs_vec(struct fm10k_tx_queue *txq)
-{
-	unsigned i;
-	const uint16_t max_desc = (uint16_t)(txq->nb_desc - 1);
-
-	if (txq->sw_ring == NULL || txq->nb_free == max_desc)
-		return;
-
-	/* release the used mbufs in sw_ring */
-	for (i = txq->next_dd - (txq->rs_thresh - 1);
-	     i != txq->next_free;
-	     i = (i + 1) & max_desc)
-		rte_pktmbuf_free_seg(txq->sw_ring[i]);
-
-	txq->nb_free = max_desc;
-
-	/* reset tx_entry */
-	for (i = 0; i < txq->nb_desc; i++)
-		txq->sw_ring[i] = NULL;
-
-	rte_free(txq->sw_ring);
-	txq->sw_ring = NULL;
-}
-
-static void __attribute__((cold))
 fm10k_reset_tx_queue(struct fm10k_tx_queue *txq)
 {
 	static const struct fm10k_tx_desc zeroed_desc = {0};
-- 
1.7.7.6

Re: [dpdk-dev] [PATCH] fm10k: fix a crash bug when quit from testpmd

[Qiu, Michael]

On 2015/11/12 12:58, Chen Jing D(Mark) wrote:
> From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>
>
> When the fm10k port is closed, both func tx_queue_clean() and
> fm10k_tx_queue_release_mbufs_vec() will try to release buffer in
> SW ring. The latter func won't do sanity check on those pointers
> and cause crash.
>
> The fix include 2 parts.
> 1. Remove Vector TX buffer release func since it can share the
>    release functions with regular TX.
> 2. Add log to print out what actual Rx/Tx func is used.
>
> Signed-off-by: Chen Jing D(Mark) <jing.d.chen@intel.com>
> ---

Acked-by: Michael Qiu <michael.qiu@intel.com>
>  drivers/net/fm10k/fm10k.h          |    1 -
>  drivers/net/fm10k/fm10k_ethdev.c   |   17 ++++++++++++-----
>  drivers/net/fm10k/fm10k_rxtx_vec.c |   28 ----------------------------
>  3 files changed, 12 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/net/fm10k/fm10k.h b/drivers/net/fm10k/fm10k.h
> index 754aa6a..38d5489 100644
> --- a/drivers/net/fm10k/fm10k.h
> +++ b/drivers/net/fm10k/fm10k.h
> @@ -237,7 +237,6 @@ struct fm10k_tx_queue {
>  };
>  
>  struct fm10k_txq_ops {
> -	void (*release_mbufs)(struct fm10k_tx_queue *txq);
>  	void (*reset)(struct fm10k_tx_queue *txq);
>  };
>  
> diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
> index cf7ada7..af7b0c2 100644
> --- a/drivers/net/fm10k/fm10k_ethdev.c
> +++ b/drivers/net/fm10k/fm10k_ethdev.c
> @@ -386,7 +386,6 @@ fm10k_check_mq_mode(struct rte_eth_dev *dev)
>  }
>  
>  static const struct fm10k_txq_ops def_txq_ops = {
> -	.release_mbufs = tx_queue_free,
>  	.reset = tx_queue_reset,
>  };
>  
> @@ -1073,7 +1072,7 @@ fm10k_dev_queue_release(struct rte_eth_dev *dev)
>  		for (i = 0; i < dev->data->nb_tx_queues; i++) {
>  			struct fm10k_tx_queue *txq = dev->data->tx_queues[i];
>  
> -			txq->ops->release_mbufs(txq);
> +			tx_queue_free(txq);
>  		}
>  	}
>  
> @@ -1793,7 +1792,7 @@ fm10k_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_id,
>  	if (dev->data->tx_queues[queue_id] != NULL) {
>  		struct fm10k_tx_queue *txq = dev->data->tx_queues[queue_id];
>  
> -		txq->ops->release_mbufs(txq);
> +		tx_queue_free(txq);
>  		dev->data->tx_queues[queue_id] = NULL;
>  	}
>  
> @@ -1872,7 +1871,7 @@ fm10k_tx_queue_release(void *queue)
>  	struct fm10k_tx_queue *q = queue;
>  	PMD_INIT_FUNC_TRACE();
>  
> -	q->ops->release_mbufs(q);
> +	tx_queue_free(q);
>  }
>  
>  static int
> @@ -2439,13 +2438,16 @@ fm10k_set_tx_function(struct rte_eth_dev *dev)
>  	}
>  
>  	if (use_sse) {
> +		PMD_INIT_LOG(ERR, "Use vector Tx func");
>  		for (i = 0; i < dev->data->nb_tx_queues; i++) {
>  			txq = dev->data->tx_queues[i];
>  			fm10k_txq_vec_setup(txq);
>  		}
>  		dev->tx_pkt_burst = fm10k_xmit_pkts_vec;
> -	} else
> +	} else {
>  		dev->tx_pkt_burst = fm10k_xmit_pkts;
> +		PMD_INIT_LOG(ERR, "Use regular Tx func");
> +	}
>  }
>  
>  static void __attribute__((cold))
> @@ -2469,6 +2471,11 @@ fm10k_set_rx_function(struct rte_eth_dev *dev)
>  		(dev->rx_pkt_burst == fm10k_recv_scattered_pkts_vec ||
>  		dev->rx_pkt_burst == fm10k_recv_pkts_vec);
>  
> +	if (rx_using_sse)
> +		PMD_INIT_LOG(ERR, "Use vector Rx func");
> +	else
> +		PMD_INIT_LOG(ERR, "Use regular Rx func");
> +
>  	for (i = 0; i < dev->data->nb_rx_queues; i++) {
>  		struct fm10k_rx_queue *rxq = dev->data->rx_queues[i];
>  
> diff --git a/drivers/net/fm10k/fm10k_rxtx_vec.c b/drivers/net/fm10k/fm10k_rxtx_vec.c
> index 06beca9..6042568 100644
> --- a/drivers/net/fm10k/fm10k_rxtx_vec.c
> +++ b/drivers/net/fm10k/fm10k_rxtx_vec.c
> @@ -45,8 +45,6 @@
>  #endif
>  
>  static void
> -fm10k_tx_queue_release_mbufs_vec(struct fm10k_tx_queue *txq);
> -static void
>  fm10k_reset_tx_queue(struct fm10k_tx_queue *txq);
>  
>  /* Handling the offload flags (olflags) field takes computation
> @@ -634,7 +632,6 @@ fm10k_recv_scattered_pkts_vec(void *rx_queue,
>  }
>  
>  static const struct fm10k_txq_ops vec_txq_ops = {
> -	.release_mbufs = fm10k_tx_queue_release_mbufs_vec,
>  	.reset = fm10k_reset_tx_queue,
>  };
>  
> @@ -795,31 +792,6 @@ fm10k_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
>  }
>  
>  static void __attribute__((cold))
> -fm10k_tx_queue_release_mbufs_vec(struct fm10k_tx_queue *txq)
> -{
> -	unsigned i;
> -	const uint16_t max_desc = (uint16_t)(txq->nb_desc - 1);
> -
> -	if (txq->sw_ring == NULL || txq->nb_free == max_desc)
> -		return;
> -
> -	/* release the used mbufs in sw_ring */
> -	for (i = txq->next_dd - (txq->rs_thresh - 1);
> -	     i != txq->next_free;
> -	     i = (i + 1) & max_desc)
> -		rte_pktmbuf_free_seg(txq->sw_ring[i]);
> -
> -	txq->nb_free = max_desc;
> -
> -	/* reset tx_entry */
> -	for (i = 0; i < txq->nb_desc; i++)
> -		txq->sw_ring[i] = NULL;
> -
> -	rte_free(txq->sw_ring);
> -	txq->sw_ring = NULL;
> -}
> -
> -static void __attribute__((cold))
>  fm10k_reset_tx_queue(struct fm10k_tx_queue *txq)
>  {
>  	static const struct fm10k_tx_desc zeroed_desc = {0};

Re: [dpdk-dev] [PATCH] fm10k: fix a crash bug when quit from testpmd

[Thomas Monjalon]

2015-11-12 12:57, Chen Jing D:
> From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>
> 
> When the fm10k port is closed, both func tx_queue_clean() and
> fm10k_tx_queue_release_mbufs_vec() will try to release buffer in
> SW ring. The latter func won't do sanity check on those pointers
> and cause crash.
> 
> The fix include 2 parts.
> 1. Remove Vector TX buffer release func since it can share the
>    release functions with regular TX.
> 2. Add log to print out what actual Rx/Tx func is used.

2 parts mean 2 patches.

[...]
> +	if (rx_using_sse)
> +		PMD_INIT_LOG(ERR, "Use vector Rx func");
> +	else
> +		PMD_INIT_LOG(ERR, "Use regular Rx func");

Why using en error log level?

Re: [dpdk-dev] [PATCH] fm10k: fix a crash bug when quit from testpmd

[Chen, Jing D]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, November 24, 2015 6:55 AM
> To: Chen, Jing D
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] fm10k: fix a crash bug when quit from
> testpmd
> 
> 2015-11-12 12:57, Chen Jing D:
> > From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>
> >
> > When the fm10k port is closed, both func tx_queue_clean() and
> > fm10k_tx_queue_release_mbufs_vec() will try to release buffer in
> > SW ring. The latter func won't do sanity check on those pointers
> > and cause crash.
> >
> > The fix include 2 parts.
> > 1. Remove Vector TX buffer release func since it can share the
> >    release functions with regular TX.
> > 2. Add log to print out what actual Rx/Tx func is used.
> 
> 2 parts mean 2 patches.

OK, I'll send 2.

> 
> [...]
> > +	if (rx_using_sse)
> > +		PMD_INIT_LOG(ERR, "Use vector Rx func");
> > +	else
> > +		PMD_INIT_LOG(ERR, "Use regular Rx func");
> 
> Why using en error log level?

Because fm10k will decide best rx/tx func in running time, some users 
complain they can't find which rx/tx func they are using. the error level log
will help them.

[dpdk-dev] [PATCH v2] fm10k: fix a crash bug when quit from testpmd

[Chen Jing D(Mark)]

From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>

When the fm10k port is closed, both func tx_queue_clean() and
fm10k_tx_queue_release_mbufs_vec() will try to release buffer in
SW ring. The latter func won't do sanity check on those pointers
and cause crash.

The fix removed Vector TX buffer release func since it can share
the release functions with regular TX.

fixes: fb9066e479a6(fm10k: reset and release mbuf)

Signed-off-by: Chen Jing D(Mark) <jing.d.chen@intel.com>
Acked-by: Michael Qiu <michael.qiu@intel.com>
---
 drivers/net/fm10k/fm10k.h          |    1 -
 drivers/net/fm10k/fm10k_ethdev.c   |    7 +++----
 drivers/net/fm10k/fm10k_rxtx_vec.c |   28 ----------------------------
 3 files changed, 3 insertions(+), 33 deletions(-)

v2 changes:
 - remove debug info for actual rx/tx func.

diff --git a/drivers/net/fm10k/fm10k.h b/drivers/net/fm10k/fm10k.h
index 754aa6a..38d5489 100644
--- a/drivers/net/fm10k/fm10k.h
+++ b/drivers/net/fm10k/fm10k.h
@@ -237,7 +237,6 @@ struct fm10k_tx_queue {
 };
 
 struct fm10k_txq_ops {
-	void (*release_mbufs)(struct fm10k_tx_queue *txq);
 	void (*reset)(struct fm10k_tx_queue *txq);
 };
 
diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
index 441f713..7f5c852 100644
--- a/drivers/net/fm10k/fm10k_ethdev.c
+++ b/drivers/net/fm10k/fm10k_ethdev.c
@@ -386,7 +386,6 @@ fm10k_check_mq_mode(struct rte_eth_dev *dev)
 }
 
 static const struct fm10k_txq_ops def_txq_ops = {
-	.release_mbufs = tx_queue_free,
 	.reset = tx_queue_reset,
 };
 
@@ -1073,7 +1072,7 @@ fm10k_dev_queue_release(struct rte_eth_dev *dev)
 		for (i = 0; i < dev->data->nb_tx_queues; i++) {
 			struct fm10k_tx_queue *txq = dev->data->tx_queues[i];
 
-			txq->ops->release_mbufs(txq);
+			tx_queue_free(txq);
 		}
 	}
 
@@ -1761,7 +1760,7 @@ fm10k_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_id,
 	if (dev->data->tx_queues[queue_id] != NULL) {
 		struct fm10k_tx_queue *txq = dev->data->tx_queues[queue_id];
 
-		txq->ops->release_mbufs(txq);
+		tx_queue_free(txq);
 		dev->data->tx_queues[queue_id] = NULL;
 	}
 
@@ -1836,7 +1835,7 @@ fm10k_tx_queue_release(void *queue)
 	struct fm10k_tx_queue *q = queue;
 	PMD_INIT_FUNC_TRACE();
 
-	q->ops->release_mbufs(q);
+	tx_queue_free(q);
 }
 
 static int
diff --git a/drivers/net/fm10k/fm10k_rxtx_vec.c b/drivers/net/fm10k/fm10k_rxtx_vec.c
index 06beca9..6042568 100644
--- a/drivers/net/fm10k/fm10k_rxtx_vec.c
+++ b/drivers/net/fm10k/fm10k_rxtx_vec.c
@@ -45,8 +45,6 @@
 #endif
 
 static void
-fm10k_tx_queue_release_mbufs_vec(struct fm10k_tx_queue *txq);
-static void
 fm10k_reset_tx_queue(struct fm10k_tx_queue *txq);
 
 /* Handling the offload flags (olflags) field takes computation
@@ -634,7 +632,6 @@ fm10k_recv_scattered_pkts_vec(void *rx_queue,
 }
 
 static const struct fm10k_txq_ops vec_txq_ops = {
-	.release_mbufs = fm10k_tx_queue_release_mbufs_vec,
 	.reset = fm10k_reset_tx_queue,
 };
 
@@ -795,31 +792,6 @@ fm10k_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
 }
 
 static void __attribute__((cold))
-fm10k_tx_queue_release_mbufs_vec(struct fm10k_tx_queue *txq)
-{
-	unsigned i;
-	const uint16_t max_desc = (uint16_t)(txq->nb_desc - 1);
-
-	if (txq->sw_ring == NULL || txq->nb_free == max_desc)
-		return;
-
-	/* release the used mbufs in sw_ring */
-	for (i = txq->next_dd - (txq->rs_thresh - 1);
-	     i != txq->next_free;
-	     i = (i + 1) & max_desc)
-		rte_pktmbuf_free_seg(txq->sw_ring[i]);
-
-	txq->nb_free = max_desc;
-
-	/* reset tx_entry */
-	for (i = 0; i < txq->nb_desc; i++)
-		txq->sw_ring[i] = NULL;
-
-	rte_free(txq->sw_ring);
-	txq->sw_ring = NULL;
-}
-
-static void __attribute__((cold))
 fm10k_reset_tx_queue(struct fm10k_tx_queue *txq)
 {
 	static const struct fm10k_tx_desc zeroed_desc = {0};
-- 
1.7.7.6

Re: [dpdk-dev] [PATCH] fm10k: fix a crash bug when quit from testpmd

[Thomas Monjalon]

2015-11-24 02:20, Chen, Jing D:
> From: Thomas Monjalon
> > 2015-11-12 12:57, Chen Jing D:
> > > +	if (rx_using_sse)
> > > +		PMD_INIT_LOG(ERR, "Use vector Rx func");
> > > +	else
> > > +		PMD_INIT_LOG(ERR, "Use regular Rx func");
> > 
> > Why using en error log level?
> 
> Because fm10k will decide best rx/tx func in running time, some users 
> complain they can't find which rx/tx func they are using. the error level log
> will help them.

But this is not an error.
Please check how other drivers advertise the chosen implementation.

Re: [dpdk-dev] [PATCH v2] fm10k: fix a crash bug when quit from testpmd

[Thomas Monjalon]

2015-11-24 11:10, Chen Jing D:
> When the fm10k port is closed, both func tx_queue_clean() and
> fm10k_tx_queue_release_mbufs_vec() will try to release buffer in
> SW ring. The latter func won't do sanity check on those pointers
> and cause crash.
> 
> The fix removed Vector TX buffer release func since it can share
> the release functions with regular TX.
> 
> fixes: fb9066e479a6(fm10k: reset and release mbuf)
> 
> Signed-off-by: Chen Jing D(Mark) <jing.d.chen@intel.com>
> Acked-by: Michael Qiu <michael.qiu@intel.com>

Applied, thanks