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

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

[hepeng]

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

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

[Lu, Wenzhuo]

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.

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

[HePeng]

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

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

[Lu, Wenzhuo]

Hi Peng,

> -----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
> 
> 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.
But I think it's skipped, like this,

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

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

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

[HePeng]

Oh, sorry, my mistakes, it is in the tx_release_mbuf 

static void
ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue *txq)
{
	unsigned i;

	if (txq->sw_ring != NULL) {
		for (i = 0; i < txq->nb_tx_desc; i++) {
			if (txq->sw_ring[i].mbuf != NULL) {
				rte_pktmbuf_free_seg(txq->sw_ring[i].mbuf);
				txq->sw_ring[i].mbuf = NULL;
			}
		}
	}
}

So the real patch should be added here.


> 在 2015年8月3日，下午4:10，Lu, Wenzhuo <wenzhuo.lu@intel.com> 写道：
> 
> Hi Peng,
> 
>> -----Original Message-----
>> From: HePeng [mailto:xnhp0320@icloud.com <mailto:xnhp0320@icloud.com>]
>> Sent: Monday, August 3, 2015 3:09 PM
>> To: Lu, Wenzhuo
>> Cc: dev@dpdk.org <mailto:dev@dpdk.org>
>> Subject: Re: [dpdk-dev] [PATCH] [new]ixgbe:set txep.mbuf to NULL when calling
>> ixgbe_tx_free_bufs
>> 
>> 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.
> But I think it's skipped, like this,
> 
> 			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);
> 
>> 
>> 
>>> 在 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.

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

[Lu, Wenzhuo]

Hi  Peng,
There're 2 versions of ixgbe_tx_queue_release_mbufs. One in ixgbe_rxtx.c, the other is in ixgbe_rxtx_vec.c.
What you shown is the one in ixgbe_rxtx.c. You can find the one in ixgbe_rxtx_vec.c already has the sanity check for reference count.
Thanks.

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

[HePeng]

 Hi, wenzhuo,
	Have to check that because I really had the panic when compile my code with dpdk 2.0.0. 

	Also I checked the code in the dpdk git repo. It does not check the reference count, it just only calculate 
the begin positions of mbufs that really needs to be recycled. 
	my code is retrieved from git://dpdk.org/dpdk <git://dpdk.org/dpdk>. 

	Thanks for reviewing.

> 在 2015年8月3日，下午4:42，Lu, Wenzhuo <wenzhuo.lu@intel.com> 写道：
> 
> Hi  Peng,
> There’re 2 versions of ixgbe_tx_queue_release_mbufs. One in ixgbe_rxtx.c, the other is in ixgbe_rxtx_vec.c.
> What you shown is the one in ixgbe_rxtx.c. You can find the one in ixgbe_rxtx_vec.c already has the sanity check for reference count.
> Thanks.

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

[Ananyev, Konstantin]

Hi Peng,

As Wenzhuo pointed, we have a special version of tx_release_mbufs fo vector TX:
ixgbe_tx_queue_release_mbufs_vec().
It only frees sw_ring[] entries that contains valid mbufs.
So we don't need to set sw_ring[].mbuf = NULL at  ixgbe_tx_free_bufs().
About the panic you mentioned: I think that was fixed just few days ago.
Please have a look at:
http://dpdk.org/ml/archives/dev/2015-July/022146.html
Konstantin

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of HePeng
> Sent: Monday, August 03, 2015 10:12 AM
> 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
> 
>  Hi, wenzhuo,
> 	Have to check that because I really had the panic when compile my code with dpdk 2.0.0.
> 
> 	Also I checked the code in the dpdk git repo. It does not check the reference count, it just only calculate
> the begin positions of mbufs that really needs to be recycled.
> 	my code is retrieved from git://dpdk.org/dpdk <git://dpdk.org/dpdk>.
> 
> 	Thanks for reviewing.
> 
> > 在 2015年8月3日，下午4:42，Lu, Wenzhuo <wenzhuo.lu@intel.com> 写道：
> >
> > Hi  Peng,
> > There’re 2 versions of ixgbe_tx_queue_release_mbufs. One in ixgbe_rxtx.c, the other is in ixgbe_rxtx_vec.c.
> > What you shown is the one in ixgbe_rxtx.c. You can find the one in ixgbe_rxtx_vec.c already has the sanity check for reference
> count.
> > Thanks.

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

[Bruce Richardson]

On Sat, Aug 01, 2015 at 09:26:34AM +0800, hepeng wrote:
> 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>

Why is this necessary? Setting the mbuf pointer to null introduces an extra write
to the descriptor ring for every packet. Right now the free entries are simply
tracking using the indexes in the tx_queue structure, there is no need to set
the mbufs to NULL as well, as far as I can see.

/Bruce

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