From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <konstantin.ananyev@intel.com>
Received: from mga01.intel.com (mga01.intel.com [192.55.52.88])
 by dpdk.org (Postfix) with ESMTP id B8322C3DC
 for <dev@dpdk.org>; Mon, 20 Jul 2015 11:37:05 +0200 (CEST)
Received: from fmsmga003.fm.intel.com ([10.253.24.29])
 by fmsmga101.fm.intel.com with ESMTP; 20 Jul 2015 02:37:01 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.15,507,1432623600"; d="scan'208";a="527050573"
Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157])
 by FMSMGA003.fm.intel.com with ESMTP; 20 Jul 2015 02:37:02 -0700
Received: from irsmsx111.ger.corp.intel.com (10.108.20.4) by
 IRSMSX103.ger.corp.intel.com (163.33.3.157) with Microsoft SMTP Server (TLS)
 id 14.3.224.2; Mon, 20 Jul 2015 10:36:59 +0100
Received: from irsmsx105.ger.corp.intel.com ([169.254.7.245]) by
 irsmsx111.ger.corp.intel.com ([169.254.2.232]) with mapi id 14.03.0224.002;
 Mon, 20 Jul 2015 10:36:59 +0100
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: "Richardson, Bruce" <bruce.richardson@intel.com>, "dev@dpdk.org"
 <dev@dpdk.org>
Thread-Topic: [dpdk-dev] [PATCH 2/2] ixgbe: check mbuf refcnt when clearing
 RX/TX	ring
Thread-Index: AQHQtaan6yh1Khc5FU+D7/RMgGEJ6Z3kLNfw
Date: Mon, 20 Jul 2015 09:36:59 +0000
Message-ID: <2601191342CEEE43887BDE71AB97725836A26FB2@irsmsx105.ger.corp.intel.com>
References: <1435938006-22254-1-git-send-email-bruce.richardson@intel.com>
 <1435938006-22254-3-git-send-email-bruce.richardson@intel.com>
In-Reply-To: <1435938006-22254-3-git-send-email-bruce.richardson@intel.com>
Accept-Language: en-IE, en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
x-originating-ip: [163.33.239.181]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH 2/2] ixgbe: check mbuf refcnt when clearing
 RX/TX	ring
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Mon, 20 Jul 2015 09:37:06 -0000

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
>=20
> 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.
>=20
> 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:
>=20
> 	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
>=20
> A similar error occurs when clearing the RX ring, which is also fixed by
> this patch.
>=20
> 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(-)
>=20
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxt=
x.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)
>=20
>  	if (rxq->sw_ring !=3D NULL) {
>  		for (i =3D 0; i < rxq->nb_rx_desc; i++) {
> -			if (rxq->sw_ring[i].mbuf !=3D NULL) {
> +			if (rxq->sw_ring[i].mbuf !=3D NULL &&
> +					rte_mbuf_refcnt_read(rxq->sw_ring[i].mbuf)) {
>  				rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf);
>  				rxq->sw_ring[i].mbuf =3D 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 queu=
e 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.=20
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 on=
ly 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
 =20
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 !=3D txq->tx_tail;
>  		     i =3D (i + 1) & max_desc) {
>  			txe =3D (struct ixgbe_tx_entry_v *)&txq->sw_ring[i];
> -			if (txe->mbuf !=3D 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 !=3D NULL &&
> +					rte_mbuf_refcnt_read(txe->mbuf) !=3D 0)
>  				rte_pktmbuf_free_seg(txe->mbuf);
>  		}
>  		/* reset tx_entry */
> --
> 2.4.3