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

["Richardson, Bruce" <bruce.richardson@intel.com>]

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