[dpdk-dev] [PATCH v3 3/5] net/tap: fix check for mbuf's nb_segs failure

[dpdk-dev] [PATCH v3 3/5] net/tap: fix check for mbuf's nb_segs failure

[wangyunjian]

From: Yunjian Wang <wangyunjian@huawei.com>

Now the rxq->pool is mbuf concatenation, But its nb_segs is 1.
When do some sanity checks on the mbuf, it fails.

Fixes: 0781f5762cfe ("net/tap: support segmented mbufs")
CC: stable@dpdk.org

Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
 drivers/net/tap/rte_eth_tap.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index a9ba0ca68..703fcceb9 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -339,6 +339,23 @@ tap_rx_offload_get_queue_capa(void)
 	       DEV_RX_OFFLOAD_TCP_CKSUM;
 }
 
+static void
+tap_rxq_pool_free(struct rte_mbuf *pool)
+{
+	struct rte_mbuf *mbuf = pool;
+	uint16_t nb_segs = 1;
+
+	if (mbuf == NULL)
+		return;
+
+	while (mbuf->next) {
+		mbuf = mbuf->next;
+		nb_segs++;
+	}
+	pool->nb_segs = nb_segs;
+	rte_pktmbuf_free(pool);
+}
+
 /* Callback to handle the rx burst of packets to the correct interface and
  * file descriptor(s) in a multi-queue setup.
  */
@@ -389,7 +406,7 @@ pmd_rx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 					goto end;
 
 				seg->next = NULL;
-				rte_pktmbuf_free(mbuf);
+				tap_rxq_pool_free(mbuf);
 
 				goto end;
 			}
@@ -1033,7 +1050,7 @@ tap_dev_close(struct rte_eth_dev *dev)
 			rxq = &internals->rxq[i];
 			close(process_private->rxq_fds[i]);
 			process_private->rxq_fds[i] = -1;
-			rte_pktmbuf_free(rxq->pool);
+			tap_rxq_pool_free(rxq->pool);
 			rte_free(rxq->iovecs);
 			rxq->pool = NULL;
 			rxq->iovecs = NULL;
@@ -1072,7 +1089,7 @@ tap_rx_queue_release(void *queue)
 	if (process_private->rxq_fds[rxq->queue_id] > 0) {
 		close(process_private->rxq_fds[rxq->queue_id]);
 		process_private->rxq_fds[rxq->queue_id] = -1;
-		rte_pktmbuf_free(rxq->pool);
+		tap_rxq_pool_free(rxq->pool);
 		rte_free(rxq->iovecs);
 		rxq->pool = NULL;
 		rxq->iovecs = NULL;
@@ -1480,7 +1497,7 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
 	return 0;
 
 error:
-	rte_pktmbuf_free(rxq->pool);
+	tap_rxq_pool_free(rxq->pool);
 	rxq->pool = NULL;
 	rte_free(rxq->iovecs);
 	rxq->iovecs = NULL;
@@ -2435,7 +2452,7 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
 			rxq = &internals->rxq[i];
 			close(process_private->rxq_fds[i]);
 			process_private->rxq_fds[i] = -1;
-			rte_pktmbuf_free(rxq->pool);
+			tap_rxq_pool_free(rxq->pool);
 			rte_free(rxq->iovecs);
 			rxq->pool = NULL;
 			rxq->iovecs = NULL;
-- 
2.19.1

Re: [dpdk-dev] [dpdk-stable] [PATCH v3 3/5] net/tap: fix check for mbuf's nb_segs failure

[Ferruh Yigit]

On 4/7/2020 5:23 AM, wangyunjian wrote:
> From: Yunjian Wang <wangyunjian@huawei.com>
> 
> Now the rxq->pool is mbuf concatenation, But its nb_segs is 1.
> When do some sanity checks on the mbuf, it fails.

+1, 'rxq->pool' seems Rx ring representation as linked mbufs and empty ones has
'nb_segs' values as 1.

> 
> Fixes: 0781f5762cfe ("net/tap: support segmented mbufs")
> CC: stable@dpdk.org
> 
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index a9ba0ca68..703fcceb9 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -339,6 +339,23 @@ tap_rx_offload_get_queue_capa(void)
>  	       DEV_RX_OFFLOAD_TCP_CKSUM;
>  }
>  
> +static void
> +tap_rxq_pool_free(struct rte_mbuf *pool)
> +{
> +	struct rte_mbuf *mbuf = pool;
> +	uint16_t nb_segs = 1;
> +
> +	if (mbuf == NULL)
> +		return;
> +
> +	while (mbuf->next) {
> +		mbuf = mbuf->next;
> +		nb_segs++;
> +	}
> +	pool->nb_segs = nb_segs;
> +	rte_pktmbuf_free(pool);
> +}

Since you are already iterating the chain, why not free immediately instead of
calculating the nb_segs and making API go through the chain again, what about
following:

tap_rxq_pool_free(struct rte_mbuf *pool)
{
    struct rte_mbuf *next;







    while (pool) {



         next = pool->next;



         rte_pktmbuf_free(pool);



         pool = next;



    }
}

> +
>  /* Callback to handle the rx burst of packets to the correct interface and
>   * file descriptor(s) in a multi-queue setup.
>   */
> @@ -389,7 +406,7 @@ pmd_rx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  					goto end;
>  
>  				seg->next = NULL;
> -				rte_pktmbuf_free(mbuf);
> +				tap_rxq_pool_free(mbuf);

As far as I can see 'mbuf' should have correct 'nb_segs' value, and it can
continue to use 'rte_pktmbuf_free()'. If you can observe the problem can you
please try this?

>  
>  				goto end;
>  			}
> @@ -1033,7 +1050,7 @@ tap_dev_close(struct rte_eth_dev *dev)
>  			rxq = &internals->rxq[i];
>  			close(process_private->rxq_fds[i]);
>  			process_private->rxq_fds[i] = -1;
> -			rte_pktmbuf_free(rxq->pool);
> +			tap_rxq_pool_free(rxq->pool);
>  			rte_free(rxq->iovecs);
>  			rxq->pool = NULL;
>  			rxq->iovecs = NULL;
> @@ -1072,7 +1089,7 @@ tap_rx_queue_release(void *queue)
>  	if (process_private->rxq_fds[rxq->queue_id] > 0) {
>  		close(process_private->rxq_fds[rxq->queue_id]);
>  		process_private->rxq_fds[rxq->queue_id] = -1;
> -		rte_pktmbuf_free(rxq->pool);
> +		tap_rxq_pool_free(rxq->pool);
>  		rte_free(rxq->iovecs);
>  		rxq->pool = NULL;
>  		rxq->iovecs = NULL;
> @@ -1480,7 +1497,7 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
>  	return 0;
>  
>  error:
> -	rte_pktmbuf_free(rxq->pool);
> +	tap_rxq_pool_free(rxq->pool);
>  	rxq->pool = NULL;
>  	rte_free(rxq->iovecs);
>  	rxq->iovecs = NULL;
> @@ -2435,7 +2452,7 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
>  			rxq = &internals->rxq[i];
>  			close(process_private->rxq_fds[i]);
>  			process_private->rxq_fds[i] = -1;
> -			rte_pktmbuf_free(rxq->pool);
> +			tap_rxq_pool_free(rxq->pool);
>  			rte_free(rxq->iovecs);
>  			rxq->pool = NULL;
>  			rxq->iovecs = NULL;
> 

Re: [dpdk-dev] [dpdk-stable] [PATCH v3 3/5] net/tap: fix check for mbuf's nb_segs failure

[Stephen Hemminger]

On Tue, 7 Apr 2020 16:15:16 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> > +static void
> > +tap_rxq_pool_free(struct rte_mbuf *pool)
> > +{
> > +	struct rte_mbuf *mbuf = pool;
> > +	uint16_t nb_segs = 1;
> > +
> > +	if (mbuf == NULL)
> > +		return;
> > +
> > +	while (mbuf->next) {
> > +		mbuf = mbuf->next;
> > +		nb_segs++;
> > +	}
> > +	pool->nb_segs = nb_segs;
> > +	rte_pktmbuf_free(pool);
> > +}  

Since mbuf is going to be free, why bother with nb_segs.
Since rte_pktmbuf_free takes NULL as an argument, and frees the m->next chain
I don't see why not just
	rte_pktmbuf_free(pool)

Re: [dpdk-dev] [dpdk-stable] [PATCH v3 3/5] net/tap: fix check for mbuf's nb_segs failure

[Ferruh Yigit]

On 4/7/2020 4:38 PM, Stephen Hemminger wrote:
> On Tue, 7 Apr 2020 16:15:16 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
>>> +static void
>>> +tap_rxq_pool_free(struct rte_mbuf *pool)
>>> +{
>>> +	struct rte_mbuf *mbuf = pool;
>>> +	uint16_t nb_segs = 1;
>>> +
>>> +	if (mbuf == NULL)
>>> +		return;
>>> +
>>> +	while (mbuf->next) {
>>> +		mbuf = mbuf->next;
>>> +		nb_segs++;
>>> +	}
>>> +	pool->nb_segs = nb_segs;
>>> +	rte_pktmbuf_free(pool);
>>> +}  
> 
> Since mbuf is going to be free, why bother with nb_segs.
> Since rte_pktmbuf_free takes NULL as an argument, and frees the m->next chain
> I don't see why not just
> 	rte_pktmbuf_free(pool)
> 

Chain is not constructed properly, 'nb_segs' is wrong, only 'rte_pktmbuf_free()'
call won't free all the chain but first mbuf.

This implementation is fixing 'nb_segs' sot that 'rte_pktmbuf_free()' can work
as you suggested.

Or I suggest iterate the list and fix all mbufs, instead of fixing 'nb_segs',
this may be one iteration less.

Re: [dpdk-dev] [dpdk-stable] [PATCH v3 3/5] net/tap: fix check for mbuf's nb_segs failure

[Ferruh Yigit]

On 4/7/2020 4:45 PM, Ferruh Yigit wrote:
> On 4/7/2020 4:38 PM, Stephen Hemminger wrote:
>> On Tue, 7 Apr 2020 16:15:16 +0100
>> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>>>> +static void
>>>> +tap_rxq_pool_free(struct rte_mbuf *pool)
>>>> +{
>>>> +	struct rte_mbuf *mbuf = pool;
>>>> +	uint16_t nb_segs = 1;
>>>> +
>>>> +	if (mbuf == NULL)
>>>> +		return;
>>>> +
>>>> +	while (mbuf->next) {
>>>> +		mbuf = mbuf->next;
>>>> +		nb_segs++;
>>>> +	}
>>>> +	pool->nb_segs = nb_segs;
>>>> +	rte_pktmbuf_free(pool);
>>>> +}  
>>
>> Since mbuf is going to be free, why bother with nb_segs.
>> Since rte_pktmbuf_free takes NULL as an argument, and frees the m->next chain
>> I don't see why not just
>> 	rte_pktmbuf_free(pool)
>>
> 
> Chain is not constructed properly, 'nb_segs' is wrong, only 'rte_pktmbuf_free()'
> call won't free all the chain but first mbuf.

No, my bad. It will free all and will work as expected. But this is to fix the
mbuf sanity checks when they are enabled.

> 
> This implementation is fixing 'nb_segs' sot that 'rte_pktmbuf_free()' can work
> as you suggested.
> 
> Or I suggest iterate the list and fix all mbufs, instead of fixing 'nb_segs',
> this may be one iteration less.
> 

Re: [dpdk-dev] [dpdk-stable] [PATCH v3 3/5] net/tap: fix check for mbuf's nb_segs failure

[Stephen Hemminger]

On Tue, 7 Apr 2020 16:45:59 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 4/7/2020 4:38 PM, Stephen Hemminger wrote:
> > On Tue, 7 Apr 2020 16:15:16 +0100
> > Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> >   
> >>> +static void
> >>> +tap_rxq_pool_free(struct rte_mbuf *pool)
> >>> +{
> >>> +	struct rte_mbuf *mbuf = pool;
> >>> +	uint16_t nb_segs = 1;
> >>> +
> >>> +	if (mbuf == NULL)
> >>> +		return;
> >>> +
> >>> +	while (mbuf->next) {
> >>> +		mbuf = mbuf->next;
> >>> +		nb_segs++;
> >>> +	}
> >>> +	pool->nb_segs = nb_segs;
> >>> +	rte_pktmbuf_free(pool);
> >>> +}    
> > 
> > Since mbuf is going to be free, why bother with nb_segs.
> > Since rte_pktmbuf_free takes NULL as an argument, and frees the m->next chain
> > I don't see why not just
> > 	rte_pktmbuf_free(pool)
> >   
> 
> Chain is not constructed properly, 'nb_segs' is wrong, only 'rte_pktmbuf_free()'
> call won't free all the chain but first mbuf.
> 
> This implementation is fixing 'nb_segs' sot that 'rte_pktmbuf_free()' can work
> as you suggested.
> 
> Or I suggest iterate the list and fix all mbufs, instead of fixing 'nb_segs',
> this may be one iteration less.

If you look at implementation of rte_pktmbuf_free() in current DPDK version
it does not care what nb_segs is set to.

Re: [dpdk-dev] [dpdk-stable] [PATCH v3 3/5] net/tap: fix check for mbuf's nb_segs failure

[wangyunjian]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, April 8, 2020 12:08 AM
> To: Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org;
> keith.wiles@intel.com; Lilijun (Jerry) <jerry.lilijun@huawei.com>; xudingke
> <xudingke@huawei.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v3 3/5] net/tap: fix check for
> mbuf's nb_segs failure
> 
> On Tue, 7 Apr 2020 16:45:59 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> > On 4/7/2020 4:38 PM, Stephen Hemminger wrote:
> > > On Tue, 7 Apr 2020 16:15:16 +0100
> > > Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> > >
> > >>> +static void
> > >>> +tap_rxq_pool_free(struct rte_mbuf *pool) {
> > >>> +	struct rte_mbuf *mbuf = pool;
> > >>> +	uint16_t nb_segs = 1;
> > >>> +
> > >>> +	if (mbuf == NULL)
> > >>> +		return;
> > >>> +
> > >>> +	while (mbuf->next) {
> > >>> +		mbuf = mbuf->next;
> > >>> +		nb_segs++;
> > >>> +	}
> > >>> +	pool->nb_segs = nb_segs;
> > >>> +	rte_pktmbuf_free(pool);
> > >>> +}
> > >
> > > Since mbuf is going to be free, why bother with nb_segs.
> > > Since rte_pktmbuf_free takes NULL as an argument, and frees the
> > > m->next chain I don't see why not just
> > > 	rte_pktmbuf_free(pool)
> > >
> >
> > Chain is not constructed properly, 'nb_segs' is wrong, only
> 'rte_pktmbuf_free()'
> > call won't free all the chain but first mbuf.
> >
> > This implementation is fixing 'nb_segs' sot that 'rte_pktmbuf_free()'
> > can work as you suggested.
> >
> > Or I suggest iterate the list and fix all mbufs, instead of fixing
> > 'nb_segs', this may be one iteration less.
> 
> If you look at implementation of rte_pktmbuf_free() in current DPDK version it
> does not care what nb_segs is set to.

I found this problem with mbuf debug enabled.

Thanks
Yunjian

Re: [dpdk-dev] [dpdk-stable] [PATCH v3 3/5] net/tap: fix check for mbuf's nb_segs failure

[Ferruh Yigit]

On 4/7/2020 4:15 PM, Ferruh Yigit wrote:
> On 4/7/2020 5:23 AM, wangyunjian wrote:
>> From: Yunjian Wang <wangyunjian@huawei.com>
>>
>> Now the rxq->pool is mbuf concatenation, But its nb_segs is 1.
>> When do some sanity checks on the mbuf, it fails.
> 
> +1, 'rxq->pool' seems Rx ring representation as linked mbufs and empty ones has
> 'nb_segs' values as 1.
> 
>>
>> Fixes: 0781f5762cfe ("net/tap: support segmented mbufs")
>> CC: stable@dpdk.org
>>
>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
>> ---
>>  drivers/net/tap/rte_eth_tap.c | 27 ++++++++++++++++++++++-----
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index a9ba0ca68..703fcceb9 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -339,6 +339,23 @@ tap_rx_offload_get_queue_capa(void)
>>  	       DEV_RX_OFFLOAD_TCP_CKSUM;
>>  }
>>  
>> +static void
>> +tap_rxq_pool_free(struct rte_mbuf *pool)
>> +{
>> +	struct rte_mbuf *mbuf = pool;
>> +	uint16_t nb_segs = 1;
>> +
>> +	if (mbuf == NULL)
>> +		return;
>> +
>> +	while (mbuf->next) {
>> +		mbuf = mbuf->next;
>> +		nb_segs++;
>> +	}
>> +	pool->nb_segs = nb_segs;
>> +	rte_pktmbuf_free(pool);
>> +}
> 
> Since you are already iterating the chain, why not free immediately instead of
> calculating the nb_segs and making API go through the chain again, what about
> following:
> 
> tap_rxq_pool_free(struct rte_mbuf *pool)
> {
>     struct rte_mbuf *next;
>     while (pool) {
>          next = pool->next;
>          rte_pktmbuf_free(pool);
>          pool = next;
>     }
> }

Ignore this please, this may be still complaining in mbuf sanity check, so OK to
your usage.

> 
>> +
>>  /* Callback to handle the rx burst of packets to the correct interface and
>>   * file descriptor(s) in a multi-queue setup.
>>   */
>> @@ -389,7 +406,7 @@ pmd_rx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>>  					goto end;
>>  
>>  				seg->next = NULL;
>> -				rte_pktmbuf_free(mbuf);
>> +				tap_rxq_pool_free(mbuf);
> 
> As far as I can see 'mbuf' should have correct 'nb_segs' value, and it can
> continue to use 'rte_pktmbuf_free()'. If you can observe the problem can you
> please try this?
> 
>>  
>>  				goto end;
>>  			}
>> @@ -1033,7 +1050,7 @@ tap_dev_close(struct rte_eth_dev *dev)
>>  			rxq = &internals->rxq[i];
>>  			close(process_private->rxq_fds[i]);
>>  			process_private->rxq_fds[i] = -1;
>> -			rte_pktmbuf_free(rxq->pool);
>> +			tap_rxq_pool_free(rxq->pool);
>>  			rte_free(rxq->iovecs);
>>  			rxq->pool = NULL;
>>  			rxq->iovecs = NULL;
>> @@ -1072,7 +1089,7 @@ tap_rx_queue_release(void *queue)
>>  	if (process_private->rxq_fds[rxq->queue_id] > 0) {
>>  		close(process_private->rxq_fds[rxq->queue_id]);
>>  		process_private->rxq_fds[rxq->queue_id] = -1;
>> -		rte_pktmbuf_free(rxq->pool);
>> +		tap_rxq_pool_free(rxq->pool);
>>  		rte_free(rxq->iovecs);
>>  		rxq->pool = NULL;
>>  		rxq->iovecs = NULL;
>> @@ -1480,7 +1497,7 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
>>  	return 0;
>>  
>>  error:
>> -	rte_pktmbuf_free(rxq->pool);
>> +	tap_rxq_pool_free(rxq->pool);
>>  	rxq->pool = NULL;
>>  	rte_free(rxq->iovecs);
>>  	rxq->iovecs = NULL;
>> @@ -2435,7 +2452,7 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
>>  			rxq = &internals->rxq[i];
>>  			close(process_private->rxq_fds[i]);
>>  			process_private->rxq_fds[i] = -1;
>> -			rte_pktmbuf_free(rxq->pool);
>> +			tap_rxq_pool_free(rxq->pool);
>>  			rte_free(rxq->iovecs);
>>  			rxq->pool = NULL;
>>  			rxq->iovecs = NULL;
>>
>