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

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

[Hanoh Haim]

Signed-off-by: Hanoh Haim <hhaim@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;
+	}
+}
+
 /**
  * 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;
+		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

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

[Ilya Matveychikov]

> On Nov 15, 2017, at 1:14 PM, Hanoh Haim <hhaim@cisco.com> wrote:
> 
> Signed-off-by: Hanoh Haim <hhaim@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
> 

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

[Hanoch Haim (hhaim)]

+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@gmail.com] 
Sent: Wednesday, November 15, 2017 1:14 PM
To: Hanoch Haim (hhaim)
Cc: dev@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@cisco.com> wrote:
> 
> Signed-off-by: Hanoh Haim <hhaim@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
> 

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

[Olivier MATZ]

Hi,

On Wed, Nov 15, 2017 at 12:46:15PM +0000, Hanoch Haim (hhaim) wrote:
> +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@gmail.com] 
> Sent: Wednesday, November 15, 2017 1:14 PM
> To: Hanoch Haim (hhaim)
> Cc: dev@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@cisco.com> wrote:
> > 

I think the patch should be renamed in something like:

  mbuf: fix mbuf free performance with non atomic refcnt

A description of the problem in the commit log would also be welcome.

It looks it is a regression introduced by commit 8f094a9ac5d7.
In that case, we should also have:

Fixes: 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool")


> > Signed-off-by: Hanoh Haim <hhaim@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.

I'm not convinced that:

    __rte_pktmbuf_reset_nb_segs(m);

is clearer than:

   m->next = NULL;
   m->nb_segs = 1;

Anyway, I agree this should not be part of this patch. We should only keep
the fix.


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

Yes, agree with Ilya.


> 
> > +		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) {

I agree with Konstantin's comment done in another thread [1]:

  '''
  That would cause extra read; cmp (and possible slowdown) for atomic refcnt.
  If that really need to be fixed - probably we need to introduce a new function
  that would do update without trying to read refctn first - rte_mbuf_refcnt_update_blind() or so.
  '''

However I'm not sure a new function is really needed: the name is not ideal,
and it would only be used once. What about the patch below?

==============================
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1361,8 +1361,18 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 
                return m;
 
-       } else if (rte_atomic16_add_return(&m->refcnt_atomic, -1) == 0) {
+       } else {
 
+               /* We don't use rte_mbuf_refcnt_update() because we already
+                * tested that refcnt != 1.
+                */
+#ifdef RTE_MBUF_REFCNT_ATOMIC
+               ret = rte_atomic16_add_return(&m->refcnt_atomic, -1);
+#else
+               ret = --m->refcnt;
+#endif
+               if (ret != 0)
+                       return NULL;
 
                if (RTE_MBUF_INDIRECT(m))
                        rte_pktmbuf_detach(m);
@@ -1375,7 +1385,6 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 
                return m;
        }
-       return NULL;
 }
 
 /* deprecated, replaced by rte_pktmbuf_prefree_seg() */
==============================

[1] http://dpdk.org/dev/patchwork/patch/31378/


Regards,
Olivier

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

[Hanoch Haim (hhaim)]

Hi Oliver, 

It's hard for me to follow this thread. 

1)  It is not about clear/not-clear, it is error prone to *replicate* code that has the same logic.

"I'm not convinced that:

    __rte_pktmbuf_reset_nb_segs(m);

is clearer than:

   m->next = NULL;
   m->nb_segs = 1;

Anyway, I agree this should not be part of this patch. We should only keep the fix.
"
2) This definitely does not look good. 
All the point in my patch is to move the ref-cnt operations to set of API that already taking care of RTE_MBUF_REFCNT_ATOMIC

+               /* We don't use rte_mbuf_refcnt_update() because we already
+                * tested that refcnt != 1.
+                */
+#ifdef RTE_MBUF_REFCNT_ATOMIC
+               ret = rte_atomic16_add_return(&m->refcnt_atomic, -1); 
+#else
+               ret = --m->refcnt;
+#endif
+               if (ret != 0)
+                       return NULL;


Hanoh


-----Original Message-----
From: Olivier MATZ [mailto:olivier.matz@6wind.com] 
Sent: Wednesday, November 15, 2017 7:31 PM
To: Hanoch Haim (hhaim)
Cc: Ilya Matveychikov; dev@dpdk.org; Konstantin Ananyev
Subject: Re: [dpdk-dev] [PATCH v3] mbuf: cleanup rte_pktmbuf_lastseg(), fix atomic usage

Hi,

On Wed, Nov 15, 2017 at 12:46:15PM +0000, Hanoch Haim (hhaim) wrote:
> +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@gmail.com]
> Sent: Wednesday, November 15, 2017 1:14 PM
> To: Hanoch Haim (hhaim)
> Cc: dev@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@cisco.com> wrote:
> > 

I think the patch should be renamed in something like:

  mbuf: fix mbuf free performance with non atomic refcnt

A description of the problem in the commit log would also be welcome.

It looks it is a regression introduced by commit 8f094a9ac5d7.
In that case, we should also have:

Fixes: 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool")


> > Signed-off-by: Hanoh Haim <hhaim@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.

I'm not convinced that:

    __rte_pktmbuf_reset_nb_segs(m);

is clearer than:

   m->next = NULL;
   m->nb_segs = 1;

Anyway, I agree this should not be part of this patch. We should only keep the fix.


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

Yes, agree with Ilya.


> 
> > +		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) {

I agree with Konstantin's comment done in another thread [1]:

  '''
  That would cause extra read; cmp (and possible slowdown) for atomic refcnt.
  If that really need to be fixed - probably we need to introduce a new function
  that would do update without trying to read refctn first - rte_mbuf_refcnt_update_blind() or so.
  '''

However I'm not sure a new function is really needed: the name is not ideal, and it would only be used once. What about the patch below?

==============================
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1361,8 +1361,18 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 
                return m;
 
-       } else if (rte_atomic16_add_return(&m->refcnt_atomic, -1) == 0) {
+       } else {
 
+               /* We don't use rte_mbuf_refcnt_update() because we already
+                * tested that refcnt != 1.
+                */
+#ifdef RTE_MBUF_REFCNT_ATOMIC
+               ret = rte_atomic16_add_return(&m->refcnt_atomic, -1); 
+#else
+               ret = --m->refcnt;
+#endif
+               if (ret != 0)
+                       return NULL;
 
                if (RTE_MBUF_INDIRECT(m))
                        rte_pktmbuf_detach(m); @@ -1375,7 +1385,6 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 
                return m;
        }
-       return NULL;
 }
 
 /* deprecated, replaced by rte_pktmbuf_prefree_seg() */ ==============================

[1] http://dpdk.org/dev/patchwork/patch/31378/


Regards,
Olivier

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

[Ilya Matveychikov]

> On Nov 16, 2017, at 11:16 AM, Hanoch Haim (hhaim) <hhaim@cisco.com> wrote:
> 
> Hi Oliver, 
> 
> It's hard for me to follow this thread. 
> 
> 1)  It is not about clear/not-clear, it is error prone to *replicate* code that has the same logic.
> 
> "I'm not convinced that:
> 
>    __rte_pktmbuf_reset_nb_segs(m);
> 
> is clearer than:
> 
>   m->next = NULL;
>   m->nb_segs = 1;
> 
> Anyway, I agree this should not be part of this patch. We should only keep the fix.
> "
> 2) This definitely does not look good. 
> All the point in my patch is to move the ref-cnt operations to set of API that already taking care of RTE_MBUF_REFCNT_ATOMIC
> 
> +               /* We don't use rte_mbuf_refcnt_update() because we already
> +                * tested that refcnt != 1.
> +                */
> +#ifdef RTE_MBUF_REFCNT_ATOMIC
> +               ret = rte_atomic16_add_return(&m->refcnt_atomic, -1); 
> +#else
> +               ret = --m->refcnt;
> +#endif
> +               if (ret != 0)
> +                       return NULL;
> 

Looks ugly, agreed.

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

[Olivier MATZ]

Hi Hanoh,

On Thu, Nov 16, 2017 at 07:16:31AM +0000, Hanoch Haim (hhaim) wrote:
> Hi Oliver, 
> 
> It's hard for me to follow this thread. 

Yes, here are some few tips to make it easier to follow:
- avoid top-posting
- prefix quoted lines with "> "
- describe the problem and how you solve it in the commit log
- one problem = one patch

> 1)  It is not about clear/not-clear, it is error prone to *replicate* code that has the same logic.
> 
> "I'm not convinced that:
> 
>     __rte_pktmbuf_reset_nb_segs(m);
> 
> is clearer than:
> 
>    m->next = NULL;
>    m->nb_segs = 1;
> 
> Anyway, I agree this should not be part of this patch. We should only keep the fix.
> "

rte_mbuf_refcnt_update() was not used in rte_pktmbuf_prefree_seg() to
avoid reading the refcount twice.

The problem of having clear or unclear is fundamental. I don't see the point of
having a function __rte_pktmbuf_reset_nb_segs(). Keeping the two affectations
makes things explicit.

> 2) This definitely does not look good. 
> All the point in my patch is to move the ref-cnt operations to set of API that already taking care of RTE_MBUF_REFCNT_ATOMIC
> 
> +               /* We don't use rte_mbuf_refcnt_update() because we already
> +                * tested that refcnt != 1.
> +                */
> +#ifdef RTE_MBUF_REFCNT_ATOMIC
> +               ret = rte_atomic16_add_return(&m->refcnt_atomic, -1); 
> +#else
> +               ret = --m->refcnt;
> +#endif
> +               if (ret != 0)
> +                       return NULL;
> 

We cannot use the existing API taking care of atomic refcount
rte_mbuf_refcnt_update() because it would read the refcount twice.

We cannot change the behavior of rte_mbuf_refcnt_update() because it's a
public API.

An option proposed by Konstantin is to introduce a new helper
rte_mbuf_refcnt_update_blind() that does the same than
rte_mbuf_refcnt_update() but without the first test.  It think it is a
bit overkill to have this function for one caller.

That's why I end up with this patch. I don't see why it would be an
issue to have a mbuf ifdef inside the mbuf code.

Olivier

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

[Hanoch Haim (hhaim)]

Understood 

rte_mbuf_refcnt_update_blind() 

should be good., it will take care the RTE_MBUF_REFCNT_ATOMIC 

Hanoh


-----Original Message-----
From: Olivier MATZ [mailto:olivier.matz@6wind.com] 
Sent: Thursday, November 16, 2017 10:42 AM
To: Hanoch Haim (hhaim)
Cc: Konstantin Ananyev; Ilya Matveychikov; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v3] mbuf: cleanup rte_pktmbuf_lastseg(), fix atomic usage

Hi Hanoh,

On Thu, Nov 16, 2017 at 07:16:31AM +0000, Hanoch Haim (hhaim) wrote:
> Hi Oliver,
> 
> It's hard for me to follow this thread. 

Yes, here are some few tips to make it easier to follow:
- avoid top-posting
- prefix quoted lines with "> "
- describe the problem and how you solve it in the commit log
- one problem = one patch

> 1)  It is not about clear/not-clear, it is error prone to *replicate* code that has the same logic.
> 
> "I'm not convinced that:
> 
>     __rte_pktmbuf_reset_nb_segs(m);
> 
> is clearer than:
> 
>    m->next = NULL;
>    m->nb_segs = 1;
> 
> Anyway, I agree this should not be part of this patch. We should only keep the fix.
> "

rte_mbuf_refcnt_update() was not used in rte_pktmbuf_prefree_seg() to avoid reading the refcount twice.

The problem of having clear or unclear is fundamental. I don't see the point of having a function __rte_pktmbuf_reset_nb_segs(). Keeping the two affectations makes things explicit.

> 2) This definitely does not look good. 
> All the point in my patch is to move the ref-cnt operations to set of 
> API that already taking care of RTE_MBUF_REFCNT_ATOMIC
> 
> +               /* We don't use rte_mbuf_refcnt_update() because we already
> +                * tested that refcnt != 1.
> +                */
> +#ifdef RTE_MBUF_REFCNT_ATOMIC
> +               ret = rte_atomic16_add_return(&m->refcnt_atomic, -1); 
> +#else
> +               ret = --m->refcnt;
> +#endif
> +               if (ret != 0)
> +                       return NULL;
> 

We cannot use the existing API taking care of atomic refcount
rte_mbuf_refcnt_update() because it would read the refcount twice.

We cannot change the behavior of rte_mbuf_refcnt_update() because it's a public API.

An option proposed by Konstantin is to introduce a new helper
rte_mbuf_refcnt_update_blind() that does the same than
rte_mbuf_refcnt_update() but without the first test.  It think it is a bit overkill to have this function for one caller.

That's why I end up with this patch. I don't see why it would be an issue to have a mbuf ifdef inside the mbuf code.

Olivier

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

[Ilya Matveychikov]

> On Nov 16, 2017, at 1:06 PM, Hanoch Haim (hhaim) <hhaim@cisco.com> wrote:
> 
> Understood 
> 
> rte_mbuf_refcnt_update_blind() 
> 
> should be good., it will take care the RTE_MBUF_REFCNT_ATOMIC 
> 


Why guys not to add just __rte_mbuf_refcnt_update() as a wrapper over
rte_atomic16_add_return() and use it in inside rte_mbuf_refcnt_update() and
rte_pktmbuf_prefree_seg() as well?

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

[Olivier MATZ]

On Thu, Nov 16, 2017 at 01:32:13PM +0400, Ilya Matveychikov wrote:
> 
> > On Nov 16, 2017, at 1:06 PM, Hanoch Haim (hhaim) <hhaim@cisco.com> wrote:
> > 
> > Understood 
> > 
> > rte_mbuf_refcnt_update_blind() 
> > 
> > should be good., it will take care the RTE_MBUF_REFCNT_ATOMIC 
> > 
> 
> 
> Why guys not to add just __rte_mbuf_refcnt_update() as a wrapper over
> rte_atomic16_add_return() and use it in inside rte_mbuf_refcnt_update() and
> rte_pktmbuf_prefree_seg() as well?
> 

Is there any other difference with rte_mbuf_refcnt_update_blind() except
the function name?

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

[Ilya Matveychikov]

> On Nov 16, 2017, at 1:37 PM, Olivier MATZ <olivier.matz@6wind.com> wrote:
> 
> On Thu, Nov 16, 2017 at 01:32:13PM +0400, Ilya Matveychikov wrote:
>> 
>>> On Nov 16, 2017, at 1:06 PM, Hanoch Haim (hhaim) <hhaim@cisco.com> wrote:
>>> 
>>> Understood 
>>> 
>>> rte_mbuf_refcnt_update_blind() 
>>> 
>>> should be good., it will take care the RTE_MBUF_REFCNT_ATOMIC 
>>> 
>> 
>> 
>> Why guys not to add just __rte_mbuf_refcnt_update() as a wrapper over
>> rte_atomic16_add_return() and use it in inside rte_mbuf_refcnt_update() and
>> rte_pktmbuf_prefree_seg() as well?
>> 
> 
> Is there any other difference with rte_mbuf_refcnt_update_blind() except
> the function name?

No really, but my suggestion was not only about the name but to use such a
function inside rte_mbuf_refcnt_update() too. Also, that is common naming
scheme in Linux kernel — to add “__” prefix for for “lightweight” functions.

Anyway, IMO having a function will be better than having ifdef/else/endif
block. 

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

[Ananyev, Konstantin]

Hi Olivier,

> I agree with Konstantin's comment done in another thread [1]:
> 
>   '''
>   That would cause extra read; cmp (and possible slowdown) for atomic refcnt.
>   If that really need to be fixed - probably we need to introduce a new function
>   that would do update without trying to read refctn first - rte_mbuf_refcnt_update_blind() or so.
>   '''
> 
> However I'm not sure a new function is really needed: the name is not ideal,

That was just the first one from top of my head  :)
If the issue in naming only - I suppose we can find something better.
Personally I like Ilya's one: __rte_mbuf_refcnt_update().

> and it would only be used once. What about the patch below?

It would do the job too, but looks a bit clumsy to me.
Konstantin

> 
> ==============================
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1361,8 +1361,18 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> 
>                 return m;
> 
> -       } else if (rte_atomic16_add_return(&m->refcnt_atomic, -1) == 0) {
> +       } else {
> 
> +               /* We don't use rte_mbuf_refcnt_update() because we already
> +                * tested that refcnt != 1.
> +                */
> +#ifdef RTE_MBUF_REFCNT_ATOMIC
> +               ret = rte_atomic16_add_return(&m->refcnt_atomic, -1);
> +#else
> +               ret = --m->refcnt;
> +#endif
> +               if (ret != 0)
> +                       return NULL;
> 
>                 if (RTE_MBUF_INDIRECT(m))
>                         rte_pktmbuf_detach(m);
> @@ -1375,7 +1385,6 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> 
>                 return m;
>         }
> -       return NULL;
>  }
> 
>  /* deprecated, replaced by rte_pktmbuf_prefree_seg() */
> ==============================
> 
> [1] http://dpdk.org/dev/patchwork/patch/31378/
> 
> 
> Regards,
> Olivier

[dpdk-dev] [PATCH v4] mbuf: fix mbuf free performance with non atomic refcnt

[Olivier Matz]

When RTE_MBUF_REFCNT_ATOMIC=n, the decrement of the mbuf reference
counter uses an atomic operation. This is not necessary and impacts
the performance (seen with TRex traffic generator).

We cannot replace rte_atomic16_add_return() by rte_mbuf_refcnt_update()
because it would add an additional check.

Solves this by introducing __rte_mbuf_refcnt_update(), which
updates the reference counter without doing anything else.

Fixes: 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool")
Suggested-by: Hanoch Haim <hhaim@cisco.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---

Hanoh,

The following patch implements what was discussed in the thread.
Are you ok with it?

Thanks,
Olivier


 lib/librte_mbuf/rte_mbuf.h | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index ce8a05ddf..dd08cb72b 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -764,6 +764,13 @@ rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
 	rte_atomic16_set(&m->refcnt_atomic, new_value);
 }
 
+/* internal */
+static inline uint16_t
+__rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
+{
+	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value));
+}
+
 /**
  * Adds given value to an mbuf's refcnt and returns its new value.
  * @param m
@@ -788,19 +795,26 @@ rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
 		return 1 + value;
 	}
 
-	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value));
+	return __rte_mbuf_refcnt_update(m, value);
 }
 
 #else /* ! RTE_MBUF_REFCNT_ATOMIC */
 
+/* internal */
+static inline uint16_t
+__rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
+{
+	m->refcnt = (uint16_t)(m->refcnt + value);
+	return m->refcnt;
+}
+
 /**
  * Adds given value to an mbuf's refcnt and returns its new value.
  */
 static inline uint16_t
 rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
 {
-	m->refcnt = (uint16_t)(m->refcnt + value);
-	return m->refcnt;
+	return __rte_mbuf_refcnt_update(m, value);
 }
 
 /**
@@ -1364,8 +1378,7 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *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);
-- 
2.11.0

Re: [dpdk-dev] [PATCH v4] mbuf: fix mbuf free performance with non atomic refcnt

[Ilya Matveychikov]

Olivier,

> On Dec 8, 2017, at 7:46 PM, Olivier Matz <olivier.matz@6wind.com> wrote:
> 

> 
> lib/librte_mbuf/rte_mbuf.h | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index ce8a05ddf..dd08cb72b 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -764,6 +764,13 @@ rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
> 	rte_atomic16_set(&m->refcnt_atomic, new_value);
> }
> 
> +/* internal */
> +static inline uint16_t
> +__rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
> +{
> +	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value));

What’s the purpose of using direct cast to uint16_t here and in other places?

> +}
> +
> /**
>  * Adds given value to an mbuf's refcnt and returns its new value.
>  * @param m
> @@ -788,19 +795,26 @@ rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
> 		return 1 + value;
> 	}
> 
> -	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value));
> +	return __rte_mbuf_refcnt_update(m, value);
> }
> 
> #else /* ! RTE_MBUF_REFCNT_ATOMIC */
> 
> +/* internal */
> +static inline uint16_t
> +__rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
> +{
> +	m->refcnt = (uint16_t)(m->refcnt + value);
> +	return m->refcnt;
> +}
> +
> /**
>  * Adds given value to an mbuf's refcnt and returns its new value.
>  */
> static inline uint16_t
> rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
> {
> -	m->refcnt = (uint16_t)(m->refcnt + value);
> -	return m->refcnt;
> +	return __rte_mbuf_refcnt_update(m, value);
> }
> 
> /**
> @@ -1364,8 +1378,7 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *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);
> -- 
> 2.11.0
> 

Re: [dpdk-dev] [PATCH v4] mbuf: fix mbuf free performance with non atomic refcnt

[Olivier MATZ]

On Fri, Dec 08, 2017 at 08:04:50PM +0400, Ilya Matveychikov wrote:
> Olivier,
> 
> > On Dec 8, 2017, at 7:46 PM, Olivier Matz <olivier.matz@6wind.com> wrote:
> > 
> 
> > 
> > lib/librte_mbuf/rte_mbuf.h | 23 ++++++++++++++++++-----
> > 1 file changed, 18 insertions(+), 5 deletions(-)
> > 
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index ce8a05ddf..dd08cb72b 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -764,6 +764,13 @@ rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
> > 	rte_atomic16_set(&m->refcnt_atomic, new_value);
> > }
> > 
> > +/* internal */
> > +static inline uint16_t
> > +__rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
> > +{
> > +	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value));
> 
> What’s the purpose of using direct cast to uint16_t here and in other places?

This is just a code move.

Few years ago, I remember that icc was quite quick to trigger warnings when
doing implicit casts. I don't know it it's still true, but that may be the
reason why this was done like this initially, or not.

I agree we could remove this explicit cast, but I think it should go in
another patch, because there are several of them.

Olivier

Re: [dpdk-dev] [PATCH v4] mbuf: fix mbuf free performance with non atomic refcnt

[Stephen Hemminger]

On Fri,  8 Dec 2017 16:46:51 +0100
Olivier Matz <olivier.matz@6wind.com> wrote:

> +/* internal */
> +static inline uint16_t
> +__rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
> +{
> +	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value));
> +}

You don't need the cast (or paren's around rte_atomic16_addr_return)
because C has implicit cast to return value.

Re: [dpdk-dev] [PATCH v4] mbuf: fix mbuf free performance with non atomic refcnt

[Hanoch Haim (hhaim)]

Oliver,  Looks great. 

Thanks,
Hanoh

-----Original Message-----
From: Olivier Matz [mailto:olivier.matz@6wind.com] 
Sent: Friday, December 08, 2017 5:47 PM
To: dev@dpdk.org; Hanoch Haim (hhaim)
Cc: matvejchikov@gmail.com; konstantin.ananyev@intel.com
Subject: [PATCH v4] mbuf: fix mbuf free performance with non atomic refcnt

When RTE_MBUF_REFCNT_ATOMIC=n, the decrement of the mbuf reference counter uses an atomic operation. This is not necessary and impacts the performance (seen with TRex traffic generator).

We cannot replace rte_atomic16_add_return() by rte_mbuf_refcnt_update() because it would add an additional check.

Solves this by introducing __rte_mbuf_refcnt_update(), which updates the reference counter without doing anything else.

Fixes: 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool")
Suggested-by: Hanoch Haim <hhaim@cisco.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---

Hanoh,

The following patch implements what was discussed in the thread.
Are you ok with it?

Thanks,
Olivier


 lib/librte_mbuf/rte_mbuf.h | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h index ce8a05ddf..dd08cb72b 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -764,6 +764,13 @@ rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
 	rte_atomic16_set(&m->refcnt_atomic, new_value);  }
 
+/* internal */
+static inline uint16_t
+__rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value) {
+	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value)); 
+}
+
 /**
  * Adds given value to an mbuf's refcnt and returns its new value.
  * @param m
@@ -788,19 +795,26 @@ rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
 		return 1 + value;
 	}
 
-	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value));
+	return __rte_mbuf_refcnt_update(m, value);
 }
 
 #else /* ! RTE_MBUF_REFCNT_ATOMIC */
 
+/* internal */
+static inline uint16_t
+__rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value) {
+	m->refcnt = (uint16_t)(m->refcnt + value);
+	return m->refcnt;
+}
+
 /**
  * Adds given value to an mbuf's refcnt and returns its new value.
  */
 static inline uint16_t
 rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)  {
-	m->refcnt = (uint16_t)(m->refcnt + value);
-	return m->refcnt;
+	return __rte_mbuf_refcnt_update(m, value);
 }
 
 /**
@@ -1364,8 +1378,7 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *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);
--
2.11.0

Re: [dpdk-dev] [PATCH v4] mbuf: fix mbuf free performance with non atomic refcnt

[Olivier MATZ]

On Fri, Dec 08, 2017 at 04:46:51PM +0100, Olivier Matz wrote:
> When RTE_MBUF_REFCNT_ATOMIC=n, the decrement of the mbuf reference
> counter uses an atomic operation. This is not necessary and impacts
> the performance (seen with TRex traffic generator).
> 
> We cannot replace rte_atomic16_add_return() by rte_mbuf_refcnt_update()
> because it would add an additional check.
> 
> Solves this by introducing __rte_mbuf_refcnt_update(), which
> updates the reference counter without doing anything else.
> 
> Fixes: 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool")
> Suggested-by: Hanoch Haim <hhaim@cisco.com>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

Forgot to Cc stable@dpdk.org

Re: [dpdk-dev] [PATCH v4] mbuf: fix mbuf free performance with non atomic refcnt

[Thomas Monjalon]

08/12/2017 16:46, Olivier Matz:
> When RTE_MBUF_REFCNT_ATOMIC=n, the decrement of the mbuf reference
> counter uses an atomic operation. This is not necessary and impacts
> the performance (seen with TRex traffic generator).
> 
> We cannot replace rte_atomic16_add_return() by rte_mbuf_refcnt_update()
> because it would add an additional check.
> 
> Solves this by introducing __rte_mbuf_refcnt_update(), which
> updates the reference counter without doing anything else.
> 
> Fixes: 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool")
> Suggested-by: Hanoch Haim <hhaim@cisco.com>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

Applied, thanks