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

  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(-)
  

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};
  

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?
  

> -----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.
  

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.