Re: [dpdk-dev] [PATCH] [new]ixgbe:set txep.mbuf to NULL when calling ixgbe_tx_free_bufs

[HePeng <xnhp0320@icloud.com>]

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. 

	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.

	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. 


> 在 2015年8月3日，下午1:16，Lu, Wenzhuo <wenzhuo.lu@intel.com> 写道：
> 
> 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.
> 
>> -----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
>> 
>> 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.
>> 
>> 
>>> 在 2015年8月3日，上午10:46，Lu, Wenzhuo <wenzhuo.lu@intel.com> 写
>> 道：
>>> 
>>> Hi Peng,
>>> 
>>>> -----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
>>>> 
>>>> In *ixgbe_tx_free_bufs*, after recycling some tx entries, one should set their
>>>> mbuf pointers to NULL.
>>>> 
>>>> 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 <xnhp0320@icloud.com>
>>>> ---
>>>> drivers/net/ixgbe/ixgbe_rxtx_vec.c | 13 ++++++++++++-
>>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>>> 
>>>> 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 = &txq->sw_ring_v[txq->tx_next_dd - (n - 1)];
>>>> 	m = __rte_pktmbuf_prefree_seg(txep[0].mbuf);
>>>> +    txep[0].mbuf = NULL;
>>>> 	if (likely(m != NULL)) {
>>>> 		free[0] = m;
>>>> 		nb_free = 1;
>>>> @@ -632,11 +633,21 @@ ixgbe_tx_free_bufs(struct ixgbe_tx_queue *txq)
>>>> 	} else {
>>>> 		for (i = 1; i < n; i++) {
>>>> 			m = __rte_pktmbuf_prefree_seg(txep[i].mbuf);
>>>> -			if (m != NULL)
>>>> +			if (m != NULL) {
>>>> 				rte_mempool_put(m->pool, m);
>>>> +            }
>>>> 		}
>>>> 	}
>>>> 
>>>> +    /*
>>>> +     * No matter the mbufs have been put back to mempool or not,
>>>> +     * we should set the txep[i].mbuf to NULL
>>>> +     */
>>>> +
>>>> +    for( i = 1; i < n; i++) {
>>>> +        txep[i].mbuf = NULL;
>>>> +    }
>>>> +
>>>> 	/* buffers were freed, update counters */
>>>> 	txq->nb_tx_free = (uint16_t)(txq->nb_tx_free + txq->tx_rs_thresh);
>>>> 	txq->tx_next_dd = (uint16_t)(txq->tx_next_dd + txq->tx_rs_thresh);
>>>> --
>>>> 1.9.1
>>> 
>>> 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.
>>> 
>