[dpdk-dev] [PATCH v3] mbuf: cleanup rte_pktmbuf_lastseg(), fix atomic usage

Hanoch Haim (hhaim) hhaim at cisco.com
Wed Nov 15 13:46:15 CET 2017


+Oliver, 
Ilia, I assume there is a reason why Oliver did that, I just consolidate the code. 
He didn't want to *write* the same value to mbuf field. 
In the common case that pointer was already cleared by the driver, it is better to just read it. From cache synchronization perspective it will run faster.

Thanks,
Hanoh


-----Original Message-----
From: Ilya Matveychikov [mailto:matvejchikov at gmail.com] 
Sent: Wednesday, November 15, 2017 1:14 PM
To: Hanoch Haim (hhaim)
Cc: dev at dpdk.org
Subject: Re: [dpdk-dev] [PATCH v3] mbuf: cleanup rte_pktmbuf_lastseg(), fix atomic usage


> On Nov 15, 2017, at 1:14 PM, Hanoh Haim <hhaim at cisco.com> wrote:
> 
> Signed-off-by: Hanoh Haim <hhaim at cisco.com>
> ---
> lib/librte_mbuf/rte_mbuf.h | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h 
> index 7e326bb..ab110f8 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1159,6 +1159,15 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
> 	__rte_mbuf_sanity_check(m, 1);
> }
> 
> +
> +static __rte_always_inline void rte_pktmbuf_reset_before_free(struct 
> +rte_mbuf *m) {
> +	if (m->next != NULL) {
> +		m->next = NULL;
> +		m->nb_segs = 1;
> +	}
> +}
> +

Probably it will be more clean to add something __te_pktmbuf_reset_nb_segs() without check for (m->next != NULL) and use it everywhere in mbuf’s the code, not only in
rte_pktmbuf_prefree_seg() function. And I think it will be better to have separate patch for that.

> /**
>  * Allocate a new mbuf from a mempool.
>  *
> @@ -1323,8 +1332,7 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
> 	m->ol_flags = 0;
> 
> 	if (rte_mbuf_refcnt_update(md, -1) == 0) {
> -		md->next = NULL;
> -		md->nb_segs = 1;

Using rte_pktmbuf_reset_before_free() here adds unnecessary check for m->next in that path.

> +		rte_pktmbuf_reset_before_free(md);
> 		rte_mbuf_refcnt_set(md, 1);
> 		rte_mbuf_raw_free(md);
> 	}
> @@ -1354,25 +1362,16 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> 		if (RTE_MBUF_INDIRECT(m))
> 			rte_pktmbuf_detach(m);
> 
> -		if (m->next != NULL) {
> -			m->next = NULL;
> -			m->nb_segs = 1;
> -		}
> -
> +		rte_pktmbuf_reset_before_free(m);
> 		return m;
> 
> -       } else if (rte_atomic16_add_return(&m->refcnt_atomic, -1) == 0) {
> -
> +	} else if (rte_mbuf_refcnt_update(m, -1) == 0) {
> 
> 		if (RTE_MBUF_INDIRECT(m))
> 			rte_pktmbuf_detach(m);
> 
> -		if (m->next != NULL) {
> -			m->next = NULL;
> -			m->nb_segs = 1;
> -		}
> +		rte_pktmbuf_reset_before_free(m);
> 		rte_mbuf_refcnt_set(m, 1);
> -
> 		return m;
> 	}
> 	return NULL;
> --
> 2.9.3
> 



More information about the dev mailing list