From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nk11p03mm-asmtp001.mac.com (nk11p03mm-asmtp001.mac.com [17.158.232.236]) by dpdk.org (Postfix) with ESMTP id 874FDC39C for ; Mon, 3 Aug 2015 10:17:33 +0200 (CEST) Received: from [192.168.31.134] (250.229.126.124.broad.bjtelecom.net [124.126.229.250]) by nk11p03mm-asmtp001.mac.com (Oracle Communications Messaging Server 7.0.5.35.0 64bit (built Mar 31 2015)) with ESMTPSA id <0NSH001ZBZ10LY10@nk11p03mm-asmtp001.mac.com> for dev@dpdk.org; Mon, 03 Aug 2015 08:17:31 +0000 (GMT) MIME-version: 1.0 (Mac OS X Mail 8.2 \(2098\)) From: HePeng In-reply-to: <6A0DE07E22DDAD4C9103DF62FEBC0909D1A630@shsmsx102.ccr.corp.intel.com> Date: Mon, 03 Aug 2015 16:17:21 +0800 Message-id: <9375FD93-28D0-47AD-9CBB-CD099E3CD488@icloud.com> References: <1438392394-19653-1-git-send-email-xnhp0320@icloud.com> <6A0DE07E22DDAD4C9103DF62FEBC0909D1A508@shsmsx102.ccr.corp.intel.com> <179B898C-F03A-4250-8857-236CFC5274BA@icloud.com> <6A0DE07E22DDAD4C9103DF62FEBC0909D1A59D@shsmsx102.ccr.corp.intel.com> <470BDF91-6FAE-4605-885E-78F02F0F0FBF@icloud.com> <6A0DE07E22DDAD4C9103DF62FEBC0909D1A630@shsmsx102.ccr.corp.intel.com> To: "Lu, Wenzhuo" X-Mailer: Apple Mail (2.2098) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:5.14.151,1.0.33,0.0.0000 definitions=2015-08-03_01:2015-08-01,2015-08-02,1970-01-01 signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=2 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=7.0.1-1412110000 definitions=main-1508030139 Content-Type: text/plain; charset=gb2312 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] [new]ixgbe:set txep.mbuf to NULL when calling ixgbe_tx_free_bufs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Aug 2015 08:17:34 -0000 Oh, sorry, my mistakes, it is in the tx_release_mbuf=20 static void ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue *txq) { unsigned i; if (txq->sw_ring !=3D NULL) { for (i =3D 0; i < txq->nb_tx_desc; i++) { if (txq->sw_ring[i].mbuf !=3D NULL) { = rte_pktmbuf_free_seg(txq->sw_ring[i].mbuf); txq->sw_ring[i].mbuf =3D NULL; } } } } So the real patch should be added here. > =D4=DA 2015=C4=EA8=D4=C23=C8=D5=A3=AC=CF=C2=CE=E74:10=A3=ACLu, Wenzhuo = =D0=B4=B5=C0=A3=BA >=20 > Hi Peng, >=20 >> -----Original Message----- >> From: HePeng [mailto:xnhp0320@icloud.com = ] >> Sent: Monday, August 3, 2015 3:09 PM >> To: Lu, Wenzhuo >> Cc: dev@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH] [new]ixgbe:set txep.mbuf to NULL when = calling >> ixgbe_tx_free_bufs >>=20 >> Hi Wenzhuo, >> The issue is in the function *ixgbe_dev_free_queues* called in = the >> *ixgbe_dev_close*. >> The *ixgbe_dev_free_queues* will call = *ixgbe_rx_queue_release_mbuf* >> to recycle all the mbuf on the queues. If some mbufs have already = been recycled >> by the *ixgbe_tx_free_bufs*, their ref cnt is 0. >>=20 >> However, since the pointers are not set to NULL, >> *ixgbe_rx_queue_release_mbuf* will also check the mbufs whose ref cnt = is 0, >> then if one enables *CONFIG_RTE_MBUF_DEBUG*, the sanity checks will = warn >> that the ref cnt is bad, and the program will bail out. >>=20 >> As you said if this is a designed behavior, you need to fix the = code in >> *ixgbe_rx_queue_release_mbuf* to skip the mbuf that already been = recycled. > But I think it's skipped, like this, >=20 > 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); >=20 >>=20 >>=20 >>> =D4=DA 2015=C4=EA8=D4=C23=C8=D5=A3=AC=CF=C2=CE=E71:16=A3=ACLu, = Wenzhuo =D0=B4 >> =B5=C0=A3=BA >>>=20 >>> Hi Peng, >>> Would you like to tell me more details about the panic? >>> I saw there's refcnt check in rte_mbuf_sanity_check. I'm not sure = what sanity >> check you want to add. >>> Thanks. >>>=20 >>>> -----Original Message----- >>>> From: HePeng [mailto:xnhp0320@icloud.com] >>>> Sent: Monday, August 3, 2015 10:54 AM >>>> To: Lu, Wenzhuo >>>> Subject: Re: [dpdk-dev] [PATCH] [new]ixgbe:set txep.mbuf to NULL = when >> calling >>>> ixgbe_tx_free_bufs >>>>=20 >>>> Hi, Wenzhuo >>>> It will cause panic if you enable *CONFIG_RTE_MBUF_DEBUG* in you >>>> config file. So if it is a designed behavior, some code fix may = need for >>>> *mbuf_sanity_check*. >>>> Thanks. >>>>=20 >>>>=20 >>>>> =D4=DA 2015=C4=EA8=D4=C23=C8=D5=A3=AC=C9=CF=CE=E710:46=A3=ACLu, = Wenzhuo >> =D0=B4 >>>> =B5=C0=A3=BA >>>>>=20 >>>>> Hi Peng, >>>>>=20 >>>>>> -----Original Message----- >>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of hepeng >>>>>> Sent: Saturday, August 1, 2015 9:27 AM >>>>>> To: dev@dpdk.org >>>>>> Subject: [dpdk-dev] [PATCH] [new]ixgbe:set txep.mbuf to NULL when >> calling >>>>>> ixgbe_tx_free_bufs >>>>>>=20 >>>>>> In *ixgbe_tx_free_bufs*, after recycling some tx entries, one = should set >> their >>>>>> mbuf pointers to NULL. >>>>>>=20 >>>>>> The first path is not correct, the txep->mbuf should be set to = NULL no >> matter >>>> if >>>>>> it is recycled into mempool >>>>>> Signed-off-by: hepeng >>>>>> --- >>>>>> drivers/net/ixgbe/ixgbe_rxtx_vec.c | 13 ++++++++++++- >>>>>> 1 file changed, 12 insertions(+), 1 deletion(-) >>>>>>=20 >>>>>> diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c >>>>>> b/drivers/net/ixgbe/ixgbe_rxtx_vec.c >>>>>> index 1c16dec..e7ce740 100644 >>>>>> --- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c >>>>>> +++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c >>>>>> @@ -612,6 +612,7 @@ ixgbe_tx_free_bufs(struct ixgbe_tx_queue = *txq) >>>>>> */ >>>>>> txep =3D &txq->sw_ring_v[txq->tx_next_dd - (n - 1)]; >>>>>> m =3D __rte_pktmbuf_prefree_seg(txep[0].mbuf); >>>>>> + txep[0].mbuf =3D NULL; >>>>>> if (likely(m !=3D NULL)) { >>>>>> free[0] =3D m; >>>>>> nb_free =3D 1; >>>>>> @@ -632,11 +633,21 @@ ixgbe_tx_free_bufs(struct ixgbe_tx_queue = *txq) >>>>>> } else { >>>>>> for (i =3D 1; i < n; i++) { >>>>>> m =3D __rte_pktmbuf_prefree_seg(txep[i].mbuf); >>>>>> - if (m !=3D NULL) >>>>>> + if (m !=3D NULL) { >>>>>> rte_mempool_put(m->pool, m); >>>>>> + } >>>>>> } >>>>>> } >>>>>>=20 >>>>>> + /* >>>>>> + * No matter the mbufs have been put back to mempool or not, >>>>>> + * we should set the txep[i].mbuf to NULL >>>>>> + */ >>>>>> + >>>>>> + for( i =3D 1; i < n; i++) { >>>>>> + txep[i].mbuf =3D NULL; >>>>>> + } >>>>>> + >>>>>> /* buffers were freed, update counters */ >>>>>> txq->nb_tx_free =3D (uint16_t)(txq->nb_tx_free + = txq->tx_rs_thresh); >>>>>> txq->tx_next_dd =3D (uint16_t)(txq->tx_next_dd + = txq->tx_rs_thresh); >>>>>> -- >>>>>> 1.9.1 >>>>>=20 >>>>> NACK. >>>>> Thanks for looking into this code. But it's designed behavior, not = an issue. >>>>> BTW, if you want to send a new version, the tittle should be like = this [PATCH >> v2] >>>> ixgbe: ..., and add "--in-reply-to your original mail" when sending = the mail, >> and >>>> add a v2 comments. You can reference the other's v2 patches for = detail.