| Message ID | 1447304251-4145-1-git-send-email-jing.d.chen@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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.
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};