[dpdk-dev] [PATCH 0/2] Fix crash with vpmd and mbuf debug

[dpdk-dev] [PATCH 0/2] Fix crash with vpmd and mbuf debug

[Bruce Richardson]

When mbuf debug support is turned on in the build time config, crashes will
occur when clearing up the RX/TX rings, if the 10G vector PMD is in use. This
error can be reproduced using testpmd.
This patchset makes the setup/teardown code easier to debug by marking "cold"
code paths as such, and then fixes the errors by checking for already-freed
mbufs when clearing the rings.

Bruce Richardson (2):
  ixgbe: add "cold" attribute to setup/teardown fns
  ixgbe: check mbuf refcnt when clearing RX/TX ring

 drivers/net/ixgbe/ixgbe_rxtx.c     | 62 ++++++++++++++++++++------------------
 drivers/net/ixgbe/ixgbe_rxtx_vec.c | 24 ++++++++++-----
 2 files changed, 48 insertions(+), 38 deletions(-)

-- 
2.4.3

[dpdk-dev] [PATCH 1/2] ixgbe: add "cold" attribute to setup/teardown fns

[Bruce Richardson]

As well as the fast-path functions in the rxtx code, there are also
functions which set up and tear down the descriptor rings. Since these
are not performance critical functions, there is no need to have them
extensively optimized, so we add __attribute__((cold)) to their
definitions. This has the side-effect of making debugging them easier as
the compiler does not optimize them as heavily, so more variables are
accessible by default in gdb.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.c     | 59 +++++++++++++++++++-------------------
 drivers/net/ixgbe/ixgbe_rxtx_vec.c | 16 ++++++-----
 2 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 3ace8a8..41a062e 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -1757,7 +1757,7 @@ ixgbe_recv_pkts_lro_bulk_alloc(void *rx_queue, struct rte_mbuf **rx_pkts,
  * needed. If the memzone is already created, then this function returns a ptr
  * to the old one.
  */
-static const struct rte_memzone *
+static const struct rte_memzone * __attribute__((cold))
 ring_dma_zone_reserve(struct rte_eth_dev *dev, const char *ring_name,
 		      uint16_t queue_id, uint32_t ring_size, int socket_id)
 {
@@ -1781,7 +1781,7 @@ ring_dma_zone_reserve(struct rte_eth_dev *dev, const char *ring_name,
 #endif
 }
 
-static void
+static void __attribute__((cold))
 ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue *txq)
 {
 	unsigned i;
@@ -1796,7 +1796,7 @@ ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue *txq)
 	}
 }
 
-static void
+static void __attribute__((cold))
 ixgbe_tx_free_swring(struct ixgbe_tx_queue *txq)
 {
 	if (txq != NULL &&
@@ -1804,7 +1804,7 @@ ixgbe_tx_free_swring(struct ixgbe_tx_queue *txq)
 		rte_free(txq->sw_ring);
 }
 
-static void
+static void __attribute__((cold))
 ixgbe_tx_queue_release(struct ixgbe_tx_queue *txq)
 {
 	if (txq != NULL && txq->ops != NULL) {
@@ -1814,14 +1814,14 @@ ixgbe_tx_queue_release(struct ixgbe_tx_queue *txq)
 	}
 }
 
-void
+void __attribute__((cold))
 ixgbe_dev_tx_queue_release(void *txq)
 {
 	ixgbe_tx_queue_release(txq);
 }
 
 /* (Re)set dynamic ixgbe_tx_queue fields to defaults */
-static void
+static void __attribute__((cold))
 ixgbe_reset_tx_queue(struct ixgbe_tx_queue *txq)
 {
 	static const union ixgbe_adv_tx_desc zeroed_desc = {{0}};
@@ -1870,7 +1870,7 @@ static const struct ixgbe_txq_ops def_txq_ops = {
  * the queue parameters. Used in tx_queue_setup by primary process and then
  * in dev_init by secondary process when attaching to an existing ethdev.
  */
-void
+void __attribute__((cold))
 ixgbe_set_tx_function(struct rte_eth_dev *dev, struct ixgbe_tx_queue *txq)
 {
 	/* Use a simple Tx queue (no offloads, no multi segs) if possible */
@@ -1900,7 +1900,7 @@ ixgbe_set_tx_function(struct rte_eth_dev *dev, struct ixgbe_tx_queue *txq)
 	}
 }
 
-int
+int __attribute__((cold))
 ixgbe_dev_tx_queue_setup(struct rte_eth_dev *dev,
 			 uint16_t queue_idx,
 			 uint16_t nb_desc,
@@ -2088,7 +2088,7 @@ ixgbe_dev_tx_queue_setup(struct rte_eth_dev *dev,
  *
  * @m scattered cluster head
  */
-static void
+static void __attribute__((cold))
 ixgbe_free_sc_cluster(struct rte_mbuf *m)
 {
 	uint8_t i, nb_segs = m->nb_segs;
@@ -2101,7 +2101,7 @@ ixgbe_free_sc_cluster(struct rte_mbuf *m)
 	}
 }
 
-static void
+static void __attribute__((cold))
 ixgbe_rx_queue_release_mbufs(struct ixgbe_rx_queue *rxq)
 {
 	unsigned i;
@@ -2133,7 +2133,7 @@ ixgbe_rx_queue_release_mbufs(struct ixgbe_rx_queue *rxq)
 			}
 }
 
-static void
+static void __attribute__((cold))
 ixgbe_rx_queue_release(struct ixgbe_rx_queue *rxq)
 {
 	if (rxq != NULL) {
@@ -2144,7 +2144,7 @@ ixgbe_rx_queue_release(struct ixgbe_rx_queue *rxq)
 	}
 }
 
-void
+void __attribute__((cold))
 ixgbe_dev_rx_queue_release(void *rxq)
 {
 	ixgbe_rx_queue_release(rxq);
@@ -2158,7 +2158,7 @@ ixgbe_dev_rx_queue_release(void *rxq)
  *  -EINVAL: the preconditions are NOT satisfied and the default Rx burst
  *           function must be used.
  */
-static inline int
+static inline int __attribute__((cold))
 #ifdef RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC
 check_rx_burst_bulk_alloc_preconditions(struct ixgbe_rx_queue *rxq)
 #else
@@ -2213,7 +2213,7 @@ check_rx_burst_bulk_alloc_preconditions(__rte_unused struct ixgbe_rx_queue *rxq)
 }
 
 /* Reset dynamic ixgbe_rx_queue fields back to defaults */
-static void
+static void __attribute__((cold))
 ixgbe_reset_rx_queue(struct ixgbe_adapter *adapter, struct ixgbe_rx_queue *rxq)
 {
 	static const union ixgbe_adv_rx_desc zeroed_desc = {{0}};
@@ -2263,7 +2263,7 @@ ixgbe_reset_rx_queue(struct ixgbe_adapter *adapter, struct ixgbe_rx_queue *rxq)
 	rxq->pkt_last_seg = NULL;
 }
 
-int
+int __attribute__((cold))
 ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
 			 uint16_t queue_idx,
 			 uint16_t nb_desc,
@@ -2470,7 +2470,7 @@ ixgbe_dev_rx_descriptor_done(void *rx_queue, uint16_t offset)
 	return !!(rxdp->wb.upper.status_error & IXGBE_RXDADV_STAT_DD);
 }
 
-void
+void __attribute__((cold))
 ixgbe_dev_clear_queues(struct rte_eth_dev *dev)
 {
 	unsigned i;
@@ -3443,7 +3443,7 @@ ixgbe_vmdq_tx_hw_configure(struct ixgbe_hw *hw)
 	return;
 }
 
-static int
+static int __attribute__((cold))
 ixgbe_alloc_rx_queue_mbufs(struct ixgbe_rx_queue *rxq)
 {
 	struct ixgbe_rx_entry *rxe = rxq->sw_ring;
@@ -3738,7 +3738,8 @@ ixgbe_set_ivar(struct rte_eth_dev *dev, u8 entry, u8 vector, s8 type)
 	}
 }
 
-void ixgbe_set_rx_function(struct rte_eth_dev *dev)
+void __attribute__((cold))
+ixgbe_set_rx_function(struct rte_eth_dev *dev)
 {
 	struct ixgbe_adapter *adapter =
 		(struct ixgbe_adapter *)dev->data->dev_private;
@@ -3974,7 +3975,7 @@ ixgbe_set_rsc(struct rte_eth_dev *dev)
 /*
  * Initializes Receive Unit.
  */
-int
+int __attribute__((cold))
 ixgbe_dev_rx_init(struct rte_eth_dev *dev)
 {
 	struct ixgbe_hw     *hw;
@@ -4156,7 +4157,7 @@ ixgbe_dev_rx_init(struct rte_eth_dev *dev)
 /*
  * Initializes Transmit Unit.
  */
-void
+void __attribute__((cold))
 ixgbe_dev_tx_init(struct rte_eth_dev *dev)
 {
 	struct ixgbe_hw     *hw;
@@ -4224,7 +4225,7 @@ ixgbe_dev_tx_init(struct rte_eth_dev *dev)
 /*
  * Set up link for 82599 loopback mode Tx->Rx.
  */
-static inline void
+static inline void __attribute__((cold))
 ixgbe_setup_loopback_link_82599(struct ixgbe_hw *hw)
 {
 	PMD_INIT_FUNC_TRACE();
@@ -4252,7 +4253,7 @@ ixgbe_setup_loopback_link_82599(struct ixgbe_hw *hw)
 /*
  * Start Transmit and Receive Units.
  */
-int
+int __attribute__((cold))
 ixgbe_dev_rxtx_start(struct rte_eth_dev *dev)
 {
 	struct ixgbe_hw     *hw;
@@ -4319,7 +4320,7 @@ ixgbe_dev_rxtx_start(struct rte_eth_dev *dev)
 /*
  * Start Receive Units for specified queue.
  */
-int
+int __attribute__((cold))
 ixgbe_dev_rx_queue_start(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 {
 	struct ixgbe_hw     *hw;
@@ -4364,7 +4365,7 @@ ixgbe_dev_rx_queue_start(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 /*
  * Stop Receive Units for specified queue.
  */
-int
+int __attribute__((cold))
 ixgbe_dev_rx_queue_stop(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 {
 	struct ixgbe_hw     *hw;
@@ -4408,7 +4409,7 @@ ixgbe_dev_rx_queue_stop(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 /*
  * Start Transmit Units for specified queue.
  */
-int
+int __attribute__((cold))
 ixgbe_dev_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)
 {
 	struct ixgbe_hw     *hw;
@@ -4449,7 +4450,7 @@ ixgbe_dev_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)
 /*
  * Stop Transmit Units for specified queue.
  */
-int
+int __attribute__((cold))
 ixgbe_dev_tx_queue_stop(struct rte_eth_dev *dev, uint16_t tx_queue_id)
 {
 	struct ixgbe_hw     *hw;
@@ -4509,7 +4510,7 @@ ixgbe_dev_tx_queue_stop(struct rte_eth_dev *dev, uint16_t tx_queue_id)
 /*
  * [VF] Initializes Receive Unit.
  */
-int
+int __attribute__((cold))
 ixgbevf_dev_rx_init(struct rte_eth_dev *dev)
 {
 	struct ixgbe_hw     *hw;
@@ -4644,7 +4645,7 @@ ixgbevf_dev_rx_init(struct rte_eth_dev *dev)
 /*
  * [VF] Initializes Transmit Unit.
  */
-void
+void __attribute__((cold))
 ixgbevf_dev_tx_init(struct rte_eth_dev *dev)
 {
 	struct ixgbe_hw     *hw;
@@ -4685,7 +4686,7 @@ ixgbevf_dev_tx_init(struct rte_eth_dev *dev)
 /*
  * [VF] Start Transmit and Receive Units.
  */
-void
+void __attribute__((cold))
 ixgbevf_dev_rxtx_start(struct rte_eth_dev *dev)
 {
 	struct ixgbe_hw     *hw;
diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
index abd10f6..0edac82 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
@@ -1,7 +1,7 @@
 /*-
  *   BSD LICENSE
  *
- *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
+ *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
  *   All rights reserved.
  *
  *   Redistribution and use in source and binary forms, with or without
@@ -650,7 +650,7 @@ ixgbe_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
 	return nb_pkts;
 }
 
-static void
+static void __attribute__((cold))
 ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue *txq)
 {
 	unsigned i;
@@ -676,7 +676,7 @@ ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue *txq)
 	}
 }
 
-static void
+static void __attribute__((cold))
 ixgbe_tx_free_swring(struct ixgbe_tx_queue *txq)
 {
 	if (txq == NULL)
@@ -688,7 +688,7 @@ ixgbe_tx_free_swring(struct ixgbe_tx_queue *txq)
 	}
 }
 
-static void
+static void __attribute__((cold))
 ixgbe_reset_tx_queue(struct ixgbe_tx_queue *txq)
 {
 	static const union ixgbe_adv_tx_desc zeroed_desc = {{0}};
@@ -728,7 +728,7 @@ static const struct ixgbe_txq_ops vec_txq_ops = {
 	.reset = ixgbe_reset_tx_queue,
 };
 
-int
+int __attribute__((cold))
 ixgbe_rxq_vec_setup(struct ixgbe_rx_queue *rxq)
 {
 	uintptr_t p;
@@ -746,7 +746,8 @@ ixgbe_rxq_vec_setup(struct ixgbe_rx_queue *rxq)
 	return 0;
 }
 
-int ixgbe_txq_vec_setup(struct ixgbe_tx_queue *txq)
+int __attribute__((cold))
+ixgbe_txq_vec_setup(struct ixgbe_tx_queue *txq)
 {
 	if (txq->sw_ring == NULL)
 		return -1;
@@ -759,7 +760,8 @@ int ixgbe_txq_vec_setup(struct ixgbe_tx_queue *txq)
 	return 0;
 }
 
-int ixgbe_rx_vec_dev_conf_condition_check(struct rte_eth_dev *dev)
+int __attribute__((cold))
+ixgbe_rx_vec_dev_conf_condition_check(struct rte_eth_dev *dev)
 {
 #ifndef RTE_LIBRTE_IEEE1588
 	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
-- 
2.4.3

[dpdk-dev] [PATCH 2/2] ixgbe: check mbuf refcnt when clearing RX/TX ring

[Bruce Richardson]

The function to clear the TX ring when a port was being closed, e.g. on
exit in testpmd, was not checking the mbuf refcnt before freeing it.
Since the function in the vector driver to clear the ring after TX does
not set the pointer to NULL post-free, this caused crashes if mbuf
debugging was turned on.

To reproduce the issue, ensure the follow config variables are set:
RTE_IXGBE_INC_VECTOR
RTE_LIBRTE_MBUF_DEBUG
Then compile up and run testpmd using 10G ports with the vector driver.
Start traffic and let some flow through, then type "stop" and "quit" at
the testpmd prompt, and crash will occur. Output below:

	testpmd> quit
	Stopping port 0...done
	Stopping port 1...PANIC in rte_mbuf_sanity_check():
	bad ref cnt
	[New Thread 0x7fffabfff700 (LWP 145312)]
	[New Thread 0x7fffb47fe700 (LWP 145311)]
	[New Thread 0x7fffb4fff700 (LWP 145310)]
	[New Thread 0x7ffff6cd5700 (LWP 145309)]
	18: [/home/bruce/dpdk.org/x86_64-native-linuxapp-gcc/app/testpmd(_start+0x29)
	<....snip for brevity...>
	Program received signal SIGABRT, Aborted.
	0x00007ffff7120a98 in raise () from /lib64/libc.so.6

A similar error occurs when clearing the RX ring, which is also fixed by
this patch.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.c     | 3 ++-
 drivers/net/ixgbe/ixgbe_rxtx_vec.c | 8 +++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 41a062e..12e25b7 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -2108,7 +2108,8 @@ ixgbe_rx_queue_release_mbufs(struct ixgbe_rx_queue *rxq)
 
 	if (rxq->sw_ring != NULL) {
 		for (i = 0; i < rxq->nb_rx_desc; i++) {
-			if (rxq->sw_ring[i].mbuf != NULL) {
+			if (rxq->sw_ring[i].mbuf != NULL &&
+					rte_mbuf_refcnt_read(rxq->sw_ring[i].mbuf)) {
 				rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
 				rxq->sw_ring[i].mbuf = NULL;
 			}
diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
index 0edac82..7e633d3 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
@@ -665,7 +665,13 @@ ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue *txq)
 		     nb_free < max_desc && i != txq->tx_tail;
 		     i = (i + 1) & max_desc) {
 			txe = (struct ixgbe_tx_entry_v *)&txq->sw_ring[i];
-			if (txe->mbuf != NULL)
+			/*
+			 *check for already freed packets.
+			 * Note: ixgbe_tx_free_bufs does not NULL after free,
+			 * so we actually have to check the reference count.
+			 */
+			if (txe->mbuf != NULL &&
+					rte_mbuf_refcnt_read(txe->mbuf) != 0)
 				rte_pktmbuf_free_seg(txe->mbuf);
 		}
 		/* reset tx_entry */
-- 
2.4.3

Re: [dpdk-dev] [PATCH 1/2] ixgbe: add "cold" attribute to setup/teardown fns

[Thomas Monjalon]

Hi Bruce,

2015-07-03 16:40, Bruce Richardson:
> As well as the fast-path functions in the rxtx code, there are also
> functions which set up and tear down the descriptor rings. Since these
> are not performance critical functions, there is no need to have them
> extensively optimized, so we add __attribute__((cold)) to their
> definitions. This has the side-effect of making debugging them easier as
> the compiler does not optimize them as heavily, so more variables are
> accessible by default in gdb.

What is the benefit, compared to -O0?

Re: [dpdk-dev] [PATCH 2/2] ixgbe: check mbuf refcnt when clearing RX/TX ring

[Thomas Monjalon]

2015-07-03 16:40, Bruce Richardson:
> The function to clear the TX ring when a port was being closed, e.g. on
> exit in testpmd, was not checking the mbuf refcnt before freeing it.
> Since the function in the vector driver to clear the ring after TX does
> not set the pointer to NULL post-free, this caused crashes if mbuf
> debugging was turned on.

Please, could you add a Fixes: line to reference the origin of the bug?
Thanks

Re: [dpdk-dev] [PATCH 1/2] ixgbe: add "cold" attribute to setup/teardown fns

[Bruce Richardson]

On Fri, Jul 03, 2015 at 05:45:34PM +0200, Thomas Monjalon wrote:
> Hi Bruce,
> 
> 2015-07-03 16:40, Bruce Richardson:
> > As well as the fast-path functions in the rxtx code, there are also
> > functions which set up and tear down the descriptor rings. Since these
> > are not performance critical functions, there is no need to have them
> > extensively optimized, so we add __attribute__((cold)) to their
> > definitions. This has the side-effect of making debugging them easier as
> > the compiler does not optimize them as heavily, so more variables are
> > accessible by default in gdb.
> 
> What is the benefit, compared to -O0?

First off, it's per function, rather than having to use -O0 globally. Secondly,
it doesn't disable optimization, it just tells the compiler that the code is
not on the hotpath - whether or not the compiler optimizes it is up to the 
compiler itself. From GCC documentation: 
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes

"The cold attribute on functions is used to inform the compiler that the 
function is unlikely to be executed. The function is optimized for size rather 
than speed and on many targets it is placed into a special subsection of the 
text section so all cold functions appear close together, improving code 
locality of non-cold parts of program. The paths leading to calls of cold
functions within code are marked as unlikely by the branch prediction mechanism.
It is thus useful to mark functions used to handle unlikely conditions, such as
perror, as cold to improve optimization of hot functions that do call marked
functions in rare occasions."

/Bruce

Re: [dpdk-dev] [PATCH 2/2] ixgbe: check mbuf refcnt when clearing RX/TX ring

[Bruce Richardson]

On Fri, Jul 03, 2015 at 05:46:55PM +0200, Thomas Monjalon wrote:
> 2015-07-03 16:40, Bruce Richardson:
> > The function to clear the TX ring when a port was being closed, e.g. on
> > exit in testpmd, was not checking the mbuf refcnt before freeing it.
> > Since the function in the vector driver to clear the ring after TX does
> > not set the pointer to NULL post-free, this caused crashes if mbuf
> > debugging was turned on.
> 
> Please, could you add a Fixes: line to reference the origin of the bug?
> Thanks

What benefit does that information provide? Given that this bug occurs in
two places in the code, and has been there for some time, I'm not sure that a
straight lookup of the commit that last changed the line(s) would identify the
true source of the bug. Because of that, I'd like to know the information is
really going to be useful before making an effort to track it down. :-)

/Bruce

Re: [dpdk-dev] [PATCH 2/2] ixgbe: check mbuf refcnt when clearing RX/TX ring

[Thomas Monjalon]

2015-07-03 17:04, Bruce Richardson:
> On Fri, Jul 03, 2015 at 05:46:55PM +0200, Thomas Monjalon wrote:
> > 2015-07-03 16:40, Bruce Richardson:
> > > The function to clear the TX ring when a port was being closed, e.g. on
> > > exit in testpmd, was not checking the mbuf refcnt before freeing it.
> > > Since the function in the vector driver to clear the ring after TX does
> > > not set the pointer to NULL post-free, this caused crashes if mbuf
> > > debugging was turned on.
> > 
> > Please, could you add a Fixes: line to reference the origin of the bug?
> > Thanks
> 
> What benefit does that information provide? Given that this bug occurs in
> two places in the code, and has been there for some time, I'm not sure that a
> straight lookup of the commit that last changed the line(s) would identify the
> true source of the bug. Because of that, I'd like to know the information is
> really going to be useful before making an effort to track it down. :-)

If it fixes a bug which was in previous release, it may deserve to be in the
release notes. It also helps people wanting to fix the old version they are
stuck with.
Given it fixes only a crash in debug mode, it's not so important here.

Re: [dpdk-dev] [PATCH 1/2] ixgbe: add "cold" attribute to setup/teardown fns

[Thomas Monjalon]

2015-07-03 16:56, Bruce Richardson:
> On Fri, Jul 03, 2015 at 05:45:34PM +0200, Thomas Monjalon wrote:
> > Hi Bruce,
> > 
> > 2015-07-03 16:40, Bruce Richardson:
> > > As well as the fast-path functions in the rxtx code, there are also
> > > functions which set up and tear down the descriptor rings. Since these
> > > are not performance critical functions, there is no need to have them
> > > extensively optimized, so we add __attribute__((cold)) to their
> > > definitions. This has the side-effect of making debugging them easier as
> > > the compiler does not optimize them as heavily, so more variables are
> > > accessible by default in gdb.
> > 
> > What is the benefit, compared to -O0?
> 
> First off, it's per function, rather than having to use -O0 globally. Secondly,
> it doesn't disable optimization, it just tells the compiler that the code is
> not on the hotpath - whether or not the compiler optimizes it is up to the 
> compiler itself. From GCC documentation: 
> https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
> 
> "The cold attribute on functions is used to inform the compiler that the 
> function is unlikely to be executed. The function is optimized for size rather 
> than speed and on many targets it is placed into a special subsection of the 
> text section so all cold functions appear close together, improving code 
> locality of non-cold parts of program. The paths leading to calls of cold
> functions within code are marked as unlikely by the branch prediction mechanism.
> It is thus useful to mark functions used to handle unlikely conditions, such as
> perror, as cold to improve optimization of hot functions that do call marked
> functions in rare occasions."

I know it may provide some optimization of the hot path.
I was asking compared to -O0 because you were justifying this change for debug.
In other words, for debugging, -O0 is probably better. So the reason of this
change should be the optimization. And it would be interesting to know if you
have seen some performance improvement.

Re: [dpdk-dev] [PATCH 1/2] ixgbe: add "cold" attribute to setup/teardown fns

[Stephen Hemminger]

On Fri,  3 Jul 2015 16:40:05 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -1757,7 +1757,7 @@ ixgbe_recv_pkts_lro_bulk_alloc(void *rx_queue, struct rte_mbuf **rx_pkts,
>   * needed. If the memzone is already created, then this function returns a ptr
>   * to the old one.
>   */
> -static const struct rte_memzone *
> +static const struct rte_memzone * __attribute__((cold))
>  ring_dma_zone_reserve(struct rte_eth_dev *dev, const char *ring_name,
>  		      uint16_t queue_id, uint32_t ring_size, int socket_id)
>  {

This function is used in multiple drivers and should really be in common code.

Re: [dpdk-dev] [PATCH 1/2] ixgbe: add "cold" attribute to setup/teardown fns

[Bruce Richardson]

On Fri, Jul 03, 2015 at 09:57:26PM +0200, Thomas Monjalon wrote:
> 2015-07-03 16:56, Bruce Richardson:
> > On Fri, Jul 03, 2015 at 05:45:34PM +0200, Thomas Monjalon wrote:
> > > Hi Bruce,
> > > 
> > > 2015-07-03 16:40, Bruce Richardson:
> > > > As well as the fast-path functions in the rxtx code, there are also
> > > > functions which set up and tear down the descriptor rings. Since these
> > > > are not performance critical functions, there is no need to have them
> > > > extensively optimized, so we add __attribute__((cold)) to their
> > > > definitions. This has the side-effect of making debugging them easier as
> > > > the compiler does not optimize them as heavily, so more variables are
> > > > accessible by default in gdb.
> > > 
> > > What is the benefit, compared to -O0?
> > 
> > First off, it's per function, rather than having to use -O0 globally. Secondly,
> > it doesn't disable optimization, it just tells the compiler that the code is
> > not on the hotpath - whether or not the compiler optimizes it is up to the 
> > compiler itself. From GCC documentation: 
> > https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
> > 
> > "The cold attribute on functions is used to inform the compiler that the 
> > function is unlikely to be executed. The function is optimized for size rather 
> > than speed and on many targets it is placed into a special subsection of the 
> > text section so all cold functions appear close together, improving code 
> > locality of non-cold parts of program. The paths leading to calls of cold
> > functions within code are marked as unlikely by the branch prediction mechanism.
> > It is thus useful to mark functions used to handle unlikely conditions, such as
> > perror, as cold to improve optimization of hot functions that do call marked
> > functions in rare occasions."
> 
> I know it may provide some optimization of the hot path.
> I was asking compared to -O0 because you were justifying this change for debug.
> In other words, for debugging, -O0 is probably better. So the reason of this
> change should be the optimization. And it would be interesting to know if you
> have seen some performance improvement.

For some cases, O0 will be necessary, but the advantage of this change is that
for debugging of code that is not in the fast-path, the use of -O0 may be 
unnecessary - which is useful, since you don't always need to do a special debug
build.

As for performance impact: no, I have not seen any performance impact from this
change. Personally, I view this as a low impact change that doesn't really have
any negatives. Is there some concern in particular you have about it? It's really
just providing some extra hints to the compiler.

/Bruce

Re: [dpdk-dev] [PATCH 1/2] ixgbe: add "cold" attribute to setup/teardown fns

[Thomas Monjalon]

2015-07-06 10:20, Bruce Richardson:
> On Fri, Jul 03, 2015 at 09:57:26PM +0200, Thomas Monjalon wrote:
> > 2015-07-03 16:56, Bruce Richardson:
> > > On Fri, Jul 03, 2015 at 05:45:34PM +0200, Thomas Monjalon wrote:
> > > > Hi Bruce,
> > > > 
> > > > 2015-07-03 16:40, Bruce Richardson:
> > > > > As well as the fast-path functions in the rxtx code, there are also
> > > > > functions which set up and tear down the descriptor rings. Since these
> > > > > are not performance critical functions, there is no need to have them
> > > > > extensively optimized, so we add __attribute__((cold)) to their
> > > > > definitions. This has the side-effect of making debugging them easier as
> > > > > the compiler does not optimize them as heavily, so more variables are
> > > > > accessible by default in gdb.
> > > > 
> > > > What is the benefit, compared to -O0?
> > > 
> > > First off, it's per function, rather than having to use -O0 globally. Secondly,
> > > it doesn't disable optimization, it just tells the compiler that the code is
> > > not on the hotpath - whether or not the compiler optimizes it is up to the 
> > > compiler itself. From GCC documentation: 
> > > https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
> > > 
> > > "The cold attribute on functions is used to inform the compiler that the 
> > > function is unlikely to be executed. The function is optimized for size rather 
> > > than speed and on many targets it is placed into a special subsection of the 
> > > text section so all cold functions appear close together, improving code 
> > > locality of non-cold parts of program. The paths leading to calls of cold
> > > functions within code are marked as unlikely by the branch prediction mechanism.
> > > It is thus useful to mark functions used to handle unlikely conditions, such as
> > > perror, as cold to improve optimization of hot functions that do call marked
> > > functions in rare occasions."
> > 
> > I know it may provide some optimization of the hot path.
> > I was asking compared to -O0 because you were justifying this change for debug.
> > In other words, for debugging, -O0 is probably better. So the reason of this
> > change should be the optimization. And it would be interesting to know if you
> > have seen some performance improvement.
> 
> For some cases, O0 will be necessary, but the advantage of this change is that
> for debugging of code that is not in the fast-path, the use of -O0 may be 
> unnecessary - which is useful, since you don't always need to do a special debug
> build.
> 
> As for performance impact: no, I have not seen any performance impact from this
> change. Personally, I view this as a low impact change that doesn't really have
> any negatives. Is there some concern in particular you have about it? It's really
> just providing some extra hints to the compiler.

No concern. I was only interested to fully understand why you made this change.
Thanks

Re: [dpdk-dev] [PATCH 0/2] Fix crash with vpmd and mbuf debug

[Thomas Monjalon]

2015-07-03 16:40, Bruce Richardson:
> When mbuf debug support is turned on in the build time config, crashes will
> occur when clearing up the RX/TX rings, if the 10G vector PMD is in use. This
> error can be reproduced using testpmd.
> This patchset makes the setup/teardown code easier to debug by marking "cold"
> code paths as such, and then fixes the errors by checking for already-freed
> mbufs when clearing the rings.

Applied, thanks

Re: [dpdk-dev] [PATCH 2/2] ixgbe: check mbuf refcnt when clearing RX/TX ring

[Ananyev, Konstantin]

Hi Bruce,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> Sent: Friday, July 03, 2015 4:40 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 2/2] ixgbe: check mbuf refcnt when clearing RX/TX ring
> 
> The function to clear the TX ring when a port was being closed, e.g. on
> exit in testpmd, was not checking the mbuf refcnt before freeing it.
> Since the function in the vector driver to clear the ring after TX does
> not set the pointer to NULL post-free, this caused crashes if mbuf
> debugging was turned on.
> 
> To reproduce the issue, ensure the follow config variables are set:
> RTE_IXGBE_INC_VECTOR
> RTE_LIBRTE_MBUF_DEBUG
> Then compile up and run testpmd using 10G ports with the vector driver.
> Start traffic and let some flow through, then type "stop" and "quit" at
> the testpmd prompt, and crash will occur. Output below:
> 
> 	testpmd> quit
> 	Stopping port 0...done
> 	Stopping port 1...PANIC in rte_mbuf_sanity_check():
> 	bad ref cnt
> 	[New Thread 0x7fffabfff700 (LWP 145312)]
> 	[New Thread 0x7fffb47fe700 (LWP 145311)]
> 	[New Thread 0x7fffb4fff700 (LWP 145310)]
> 	[New Thread 0x7ffff6cd5700 (LWP 145309)]
> 	18: [/home/bruce/dpdk.org/x86_64-native-linuxapp-gcc/app/testpmd(_start+0x29)
> 	<....snip for brevity...>
> 	Program received signal SIGABRT, Aborted.
> 	0x00007ffff7120a98 in raise () from /lib64/libc.so.6
> 
> A similar error occurs when clearing the RX ring, which is also fixed by
> this patch.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_rxtx.c     | 3 ++-
>  drivers/net/ixgbe/ixgbe_rxtx_vec.c | 8 +++++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index 41a062e..12e25b7 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -2108,7 +2108,8 @@ ixgbe_rx_queue_release_mbufs(struct ixgbe_rx_queue *rxq)
> 
>  	if (rxq->sw_ring != NULL) {
>  		for (i = 0; i < rxq->nb_rx_desc; i++) {
> -			if (rxq->sw_ring[i].mbuf != NULL) {
> +			if (rxq->sw_ring[i].mbuf != NULL &&
> +					rte_mbuf_refcnt_read(rxq->sw_ring[i].mbuf)) {
>  				rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
>  				rxq->sw_ring[i].mbuf = NULL;
>  			}


Sorry for late review, but I am afraid your changes don't fix the problem.
After sw_ring[].mbuf was freed by RX path, entity that manages that RX queue shouldn't touch that mbuf.
(unless it was allocated  by the same RX queue again).
As same mbuf could be already allocated by something else.
As an example by another RX/TX queue and is in active use. 
Same story for TX below.

As I can see the proper fix could be one of 2:
1. Make RX/TX vector functions to reset sw_ring[].mbuf to 0.
2. At queue_release_mbufs(), don't go through all sw_ring[] entries, but only though ones
which might contain valid mbufs.
For RX: entries between rx_tail and rxrearm_start only
(which implies a special queue_release_mbufs() for vector rx).
For TX: from tx_next_dd - (tx_rs_thresh - 1) and no more then nb_tx_desc - nb_tx_free
  
Konstantin

> diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> index 0edac82..7e633d3 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> @@ -665,7 +665,13 @@ ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue *txq)
>  		     nb_free < max_desc && i != txq->tx_tail;
>  		     i = (i + 1) & max_desc) {
>  			txe = (struct ixgbe_tx_entry_v *)&txq->sw_ring[i];
> -			if (txe->mbuf != NULL)
> +			/*
> +			 *check for already freed packets.
> +			 * Note: ixgbe_tx_free_bufs does not NULL after free,
> +			 * so we actually have to check the reference count.
> +			 */
> +			if (txe->mbuf != NULL &&
> +					rte_mbuf_refcnt_read(txe->mbuf) != 0)
>  				rte_pktmbuf_free_seg(txe->mbuf);
>  		}
>  		/* reset tx_entry */
> --
> 2.4.3

Re: [dpdk-dev] [PATCH 2/2] ixgbe: check mbuf refcnt when clearing RX/TX ring

[Richardson, Bruce]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, July 20, 2015 10:37 AM
> To: Richardson, Bruce; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 2/2] ixgbe: check mbuf refcnt when clearing
> RX/TX ring
> 
> Hi Bruce,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> > Sent: Friday, July 03, 2015 4:40 PM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH 2/2] ixgbe: check mbuf refcnt when clearing
> > RX/TX ring
> >
> > The function to clear the TX ring when a port was being closed, e.g.
> > on exit in testpmd, was not checking the mbuf refcnt before freeing it.
> > Since the function in the vector driver to clear the ring after TX
> > does not set the pointer to NULL post-free, this caused crashes if
> > mbuf debugging was turned on.
> >
> > To reproduce the issue, ensure the follow config variables are set:
> > RTE_IXGBE_INC_VECTOR
> > RTE_LIBRTE_MBUF_DEBUG
> > Then compile up and run testpmd using 10G ports with the vector driver.
> > Start traffic and let some flow through, then type "stop" and "quit"
> > at the testpmd prompt, and crash will occur. Output below:
> >
> > 	testpmd> quit
> > 	Stopping port 0...done
> > 	Stopping port 1...PANIC in rte_mbuf_sanity_check():
> > 	bad ref cnt
> > 	[New Thread 0x7fffabfff700 (LWP 145312)]
> > 	[New Thread 0x7fffb47fe700 (LWP 145311)]
> > 	[New Thread 0x7fffb4fff700 (LWP 145310)]
> > 	[New Thread 0x7ffff6cd5700 (LWP 145309)]
> > 	18: [/home/bruce/dpdk.org/x86_64-native-linuxapp-
> gcc/app/testpmd(_start+0x29)
> > 	<....snip for brevity...>
> > 	Program received signal SIGABRT, Aborted.
> > 	0x00007ffff7120a98 in raise () from /lib64/libc.so.6
> >
> > A similar error occurs when clearing the RX ring, which is also fixed
> > by this patch.
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  drivers/net/ixgbe/ixgbe_rxtx.c     | 3 ++-
> >  drivers/net/ixgbe/ixgbe_rxtx_vec.c | 8 +++++++-
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c
> > b/drivers/net/ixgbe/ixgbe_rxtx.c index 41a062e..12e25b7 100644
> > --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> > @@ -2108,7 +2108,8 @@ ixgbe_rx_queue_release_mbufs(struct
> > ixgbe_rx_queue *rxq)
> >
> >  	if (rxq->sw_ring != NULL) {
> >  		for (i = 0; i < rxq->nb_rx_desc; i++) {
> > -			if (rxq->sw_ring[i].mbuf != NULL) {
> > +			if (rxq->sw_ring[i].mbuf != NULL &&
> > +					rte_mbuf_refcnt_read(rxq->sw_ring[i].mbuf))
> {
> >  				rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
> >  				rxq->sw_ring[i].mbuf = NULL;
> >  			}
> 
> 
> Sorry for late review, but I am afraid your changes don't fix the problem.
> After sw_ring[].mbuf was freed by RX path, entity that manages that RX
> queue shouldn't touch that mbuf.
> (unless it was allocated  by the same RX queue again).
> As same mbuf could be already allocated by something else.
> As an example by another RX/TX queue and is in active use.
> Same story for TX below.

Good point, I'd forgotten about that scenario.

> 
> As I can see the proper fix could be one of 2:
> 1. Make RX/TX vector functions to reset sw_ring[].mbuf to 0.
> 2. At queue_release_mbufs(), don't go through all sw_ring[] entries, but
> only though ones which might contain valid mbufs.
> For RX: entries between rx_tail and rxrearm_start only (which implies a
> special queue_release_mbufs() for vector rx).
> For TX: from tx_next_dd - (tx_rs_thresh - 1) and no more then nb_tx_desc -
> nb_tx_free

Option 2 seems a better choice for a fix. I'll look at it when I get a chance.

/Bruce

> 
> Konstantin
> 
> > diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> > b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> > index 0edac82..7e633d3 100644
> > --- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> > +++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> > @@ -665,7 +665,13 @@ ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue
> *txq)
> >  		     nb_free < max_desc && i != txq->tx_tail;
> >  		     i = (i + 1) & max_desc) {
> >  			txe = (struct ixgbe_tx_entry_v *)&txq->sw_ring[i];
> > -			if (txe->mbuf != NULL)
> > +			/*
> > +			 *check for already freed packets.
> > +			 * Note: ixgbe_tx_free_bufs does not NULL after free,
> > +			 * so we actually have to check the reference count.
> > +			 */
> > +			if (txe->mbuf != NULL &&
> > +					rte_mbuf_refcnt_read(txe->mbuf) != 0)
> >  				rte_pktmbuf_free_seg(txe->mbuf);
> >  		}
> >  		/* reset tx_entry */
> > --
> > 2.4.3