[PATCH] mbuf: decrease refcnt when detaching

[PATCH] mbuf: decrease refcnt when detaching

[Hiroyuki Mikita]

The rte_pktmbuf_detach() function should decrease refcnt on a direct
buffer.

Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>
---
 lib/librte_mbuf/rte_mbuf.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 529debb..3b117ca 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1468,9 +1468,11 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
  */
 static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
 {
+	struct rte_mbuf *md = rte_mbuf_from_indirect(m);
 	struct rte_mempool *mp = m->pool;
 	uint32_t mbuf_size, buf_len, priv_size;
 
+	rte_mbuf_refcnt_update(md, -1);
 	priv_size = rte_pktmbuf_priv_size(mp);
 	mbuf_size = sizeof(struct rte_mbuf) + priv_size;
 	buf_len = rte_pktmbuf_data_room_size(mp);
@@ -1498,7 +1500,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 		if (RTE_MBUF_INDIRECT(m)) {
 			struct rte_mbuf *md = rte_mbuf_from_indirect(m);
 			rte_pktmbuf_detach(m);
-			if (rte_mbuf_refcnt_update(md, -1) == 0)
+			if (rte_mbuf_refcnt_read(md) == 0)
 				__rte_mbuf_raw_free(md);
 		}
 		return m;
-- 
1.9.1

Re: [PATCH] mbuf: decrease refcnt when detaching

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Hiroyuki Mikita
> Sent: Sunday, May 15, 2016 4:51 PM
> To: olivier.matz@6wind.com
> Cc: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] mbuf: decrease refcnt when detaching
> 
> The rte_pktmbuf_detach() function should decrease refcnt on a direct
> buffer.

Hmm, why is that?
What's wrong with the current approach?
Konstantin

> 
> Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>
> ---
>  lib/librte_mbuf/rte_mbuf.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 529debb..3b117ca 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1468,9 +1468,11 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
>   */
>  static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>  {
> +	struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>  	struct rte_mempool *mp = m->pool;
>  	uint32_t mbuf_size, buf_len, priv_size;
> 
> +	rte_mbuf_refcnt_update(md, -1);
>  	priv_size = rte_pktmbuf_priv_size(mp);
>  	mbuf_size = sizeof(struct rte_mbuf) + priv_size;
>  	buf_len = rte_pktmbuf_data_room_size(mp);
> @@ -1498,7 +1500,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>  		if (RTE_MBUF_INDIRECT(m)) {
>  			struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>  			rte_pktmbuf_detach(m);
> -			if (rte_mbuf_refcnt_update(md, -1) == 0)
> +			if (rte_mbuf_refcnt_read(md) == 0)
>  				__rte_mbuf_raw_free(md);
>  		}
>  		return m;
> --
> 1.9.1

Re: [PATCH] mbuf: decrease refcnt when detaching

[Hiroyuki MIKITA]

Hi Konstantin,

Now, the attach operation increases refcnt, but the detach does not decrease it.
I think both operations should affect refcnt or not.
Which design is intended?

In "6.7. Direct and Indirect Buffers" of Programmer's Guide,
it is mentioned that "...whenever an indirect buffer is attached to
the direct buffer,
the reference counter on the direct buffer is incremented.
Similarly, whenever the indirect buffer is detached,
the reference counter on the direct buffer is decremented."

Regards,
Hiroyuki

2016-05-16 9:05 GMT+09:00 Ananyev, Konstantin <konstantin.ananyev@intel.com>:
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Hiroyuki Mikita
>> Sent: Sunday, May 15, 2016 4:51 PM
>> To: olivier.matz@6wind.com
>> Cc: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH] mbuf: decrease refcnt when detaching
>>
>> The rte_pktmbuf_detach() function should decrease refcnt on a direct
>> buffer.
>
> Hmm, why is that?
> What's wrong with the current approach?
> Konstantin
>
>>
>> Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>
>> ---
>>  lib/librte_mbuf/rte_mbuf.h | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index 529debb..3b117ca 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -1468,9 +1468,11 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
>>   */
>>  static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>>  {
>> +     struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>>       struct rte_mempool *mp = m->pool;
>>       uint32_t mbuf_size, buf_len, priv_size;
>>
>> +     rte_mbuf_refcnt_update(md, -1);
>>       priv_size = rte_pktmbuf_priv_size(mp);
>>       mbuf_size = sizeof(struct rte_mbuf) + priv_size;
>>       buf_len = rte_pktmbuf_data_room_size(mp);
>> @@ -1498,7 +1500,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>>               if (RTE_MBUF_INDIRECT(m)) {
>>                       struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>>                       rte_pktmbuf_detach(m);
>> -                     if (rte_mbuf_refcnt_update(md, -1) == 0)
>> +                     if (rte_mbuf_refcnt_read(md) == 0)
>>                               __rte_mbuf_raw_free(md);
>>               }
>>               return m;
>> --
>> 1.9.1
>

Re: [PATCH] mbuf: decrease refcnt when detaching

[Ananyev, Konstantin]

Hi Hiroyuki,

> 
> Hi Konstantin,
> 
> Now, the attach operation increases refcnt, but the detach does not decrease it.
> I think both operations should affect refcnt or not.
> Which design is intended?
> 
> In "6.7. Direct and Indirect Buffers" of Programmer's Guide,
> it is mentioned that "...whenever an indirect buffer is attached to
> the direct buffer,
> the reference counter on the direct buffer is incremented.
> Similarly, whenever the indirect buffer is detached,
> the reference counter on the direct buffer is decremented."

Well, right now the rte_pktmbuf_detach(struct rte_mbuf *m) just restores 
the fields of indirect mbufs to the default values, nothing more.
Actual freeing of the mbuf it was attached to is done in the __rte_pktmbuf_prefree_seg().
I suppose the name rte_pktmbuf_detach() is a bit confusing here,
might be, when created, it should be named rte_pktmbuf_restore() or so.
About proposed changes - it would introduce an extra unnecessary read of refcnt (as it is a volatile field).
If we'll decide to go that way, then I think rte_pktmbuf_detach() have to deal with freeing md.
Something like that:

static inline void 
rte_pktmbuf_detach(struct rte_mbuf *m)
{
         struct rte_mbuf *md = rte_mbuf_from_indirect(m);
         
          /* former rte_pktmbuf_detach */
          rte_pktmbuf_restore(m);    
          if (rte_mbuf_refcnt_update(md, -1) == 0)
               __rte_mbuf_raw_free(md);
}

That might be a good thing in terms of API usability and clearness,
but would cause a change in public API behaviour, so I am not sure it is worth it.
Konstantin 

> 
> Regards,
> Hiroyuki
> 
> 2016-05-16 9:05 GMT+09:00 Ananyev, Konstantin <konstantin.ananyev@intel.com>:
> >
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Hiroyuki Mikita
> >> Sent: Sunday, May 15, 2016 4:51 PM
> >> To: olivier.matz@6wind.com
> >> Cc: dev@dpdk.org
> >> Subject: [dpdk-dev] [PATCH] mbuf: decrease refcnt when detaching
> >>
> >> The rte_pktmbuf_detach() function should decrease refcnt on a direct
> >> buffer.
> >
> > Hmm, why is that?
> > What's wrong with the current approach?
> > Konstantin
> >
> >>
> >> Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>
> >> ---
> >>  lib/librte_mbuf/rte_mbuf.h | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> >> index 529debb..3b117ca 100644
> >> --- a/lib/librte_mbuf/rte_mbuf.h
> >> +++ b/lib/librte_mbuf/rte_mbuf.h
> >> @@ -1468,9 +1468,11 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
> >>   */
> >>  static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
> >>  {
> >> +     struct rte_mbuf *md = rte_mbuf_from_indirect(m);
> >>       struct rte_mempool *mp = m->pool;
> >>       uint32_t mbuf_size, buf_len, priv_size;
> >>
> >> +     rte_mbuf_refcnt_update(md, -1);
> >>       priv_size = rte_pktmbuf_priv_size(mp);
> >>       mbuf_size = sizeof(struct rte_mbuf) + priv_size;
> >>       buf_len = rte_pktmbuf_data_room_size(mp);
> >> @@ -1498,7 +1500,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> >>               if (RTE_MBUF_INDIRECT(m)) {
> >>                       struct rte_mbuf *md = rte_mbuf_from_indirect(m);
> >>                       rte_pktmbuf_detach(m);
> >> -                     if (rte_mbuf_refcnt_update(md, -1) == 0)
> >> +                     if (rte_mbuf_refcnt_read(md) == 0)
> >>                               __rte_mbuf_raw_free(md);
> >>               }
> >>               return m;
> >> --
> >> 1.9.1
> >

Re: [PATCH] mbuf: decrease refcnt when detaching

[Olivier Matz]

Hi Hiroyuki,


On 05/15/2016 05:50 PM, Hiroyuki Mikita wrote:
> The rte_pktmbuf_detach() function should decrease refcnt on a direct
> buffer.
> 
> Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>
> ---
>  lib/librte_mbuf/rte_mbuf.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 529debb..3b117ca 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1468,9 +1468,11 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
>   */
>  static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>  {
> +	struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>  	struct rte_mempool *mp = m->pool;
>  	uint32_t mbuf_size, buf_len, priv_size;
>  
> +	rte_mbuf_refcnt_update(md, -1);
>  	priv_size = rte_pktmbuf_priv_size(mp);
>  	mbuf_size = sizeof(struct rte_mbuf) + priv_size;
>  	buf_len = rte_pktmbuf_data_room_size(mp);
> @@ -1498,7 +1500,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>  		if (RTE_MBUF_INDIRECT(m)) {
>  			struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>  			rte_pktmbuf_detach(m);
> -			if (rte_mbuf_refcnt_update(md, -1) == 0)
> +			if (rte_mbuf_refcnt_read(md) == 0)
>  				__rte_mbuf_raw_free(md);
>  		}
>  		return m;
> 

Thanks for submitting this patch. I agree that rte_pktmbuf_attach()
and rte_pktmbuf_detach() are not symmetrical, but I think your patch
could trigger some race conditions.

Example:

- init: m, c1 and c2 are direct mbuf
- rte_pktmbuf_attach(c1, m);  # c1 becomes a clone of m
- rte_pktmbuf_attach(c2, m);  # c2 becomes another clone of m
- rte_pktmbuf_free(m);
- after that, we have:
  - m is a direct mbuf with refcnt = 2
  - c1 is an indirect mbuf pointing to data of m
  - c2 is an indirect mbuf pointing to data of m
- if we call rte_pktmbuf_free(c1) and rte_pktmbuf_free(c2) on 2
  different cores at the same time, m can be freed twice because
  (rte_mbuf_refcnt_read(md) == 0) can be true on both cores.

I think the proper way of doing would be to have rte_pktmbuf_detach()
returning the value of rte_mbuf_refcnt_update(md, -1), ensuring that
only one core will call _rte_mbuf_raw_free().

In the unit tests, in test_attach_from_different_pool(), the mbuf m
is never freed due to this behavior. That shows the current api is
a bit misleading. I think it should also be fixed in the patch.

Another issue is that it will break the API.
To avoid issues in applications relying on the current behavior of
rte_pktmbuf_detach(), I'd say we should keep the function as-is and
mark it as deprecated. Then, introduce a new function
rte_pktmbuf_detach2() (or any better name :) ) that changes the
behavior to what you suggest. An entry in the release note would
also be helpful.

The other option is to let things as-is and just document the behavior
of rte_pktmbuf_detach(), explicitly saying that it is not symmetrical
with the attach. But I'd prefer the first option.


Thoughts ?

Regards,
Olivier

Re: [PATCH] mbuf: decrease refcnt when detaching

[Thomas Monjalon]

2016-05-16 11:46, Hiroyuki MIKITA:
> Now, the attach operation increases refcnt, but the detach does not decrease it.
> I think both operations should affect refcnt or not.
> Which design is intended?
> 
> In "6.7. Direct and Indirect Buffers" of Programmer's Guide,
> it is mentioned that "...whenever an indirect buffer is attached to
> the direct buffer,
> the reference counter on the direct buffer is incremented.
> Similarly, whenever the indirect buffer is detached,
> the reference counter on the direct buffer is decremented."

The doc is the reference. The doxygen comment should explicit every
details of the behaviour.
And the unit tests must validate every parts of the behaviour.
Probably there is a bug which is not (yet) tested.
Please see the function testclone_testupdate_testdetach. Thanks

Re: [PATCH] mbuf: decrease refcnt when detaching

[Hiroyuki MIKITA]

Hi all,

Thanks for suggestions.
I think the Oliver's first option is good.
I introduce the new function and will replace rte_pktmbuf_detach()
with it in a future release.


2016-05-16 18:13 GMT+09:00 Thomas Monjalon <thomas.monjalon@6wind.com>:
> 2016-05-16 11:46, Hiroyuki MIKITA:
>> Now, the attach operation increases refcnt, but the detach does not decrease it.
>> I think both operations should affect refcnt or not.
>> Which design is intended?
>>
>> In "6.7. Direct and Indirect Buffers" of Programmer's Guide,
>> it is mentioned that "...whenever an indirect buffer is attached to
>> the direct buffer,
>> the reference counter on the direct buffer is incremented.
>> Similarly, whenever the indirect buffer is detached,
>> the reference counter on the direct buffer is decremented."
>
> The doc is the reference. The doxygen comment should explicit every
> details of the behaviour.
> And the unit tests must validate every parts of the behaviour.
> Probably there is a bug which is not (yet) tested.
> Please see the function testclone_testupdate_testdetach. Thanks
>

[PATCH v2] mbuf: decrease refcnt when detaching

[Hiroyuki Mikita]

The rte_pktmbuf_detach() function should decrease refcnt on a direct
buffer.

Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>
---
v2:
* introduced a new function rte_pktmbuf_detach2() which decrease refcnt.
* marked rte_pktmbuf_detach() as deprecated.
* added comments about refcnt to rte_pktmbuf_attach() and rte_pktmbuf_detach().
* checked refcnt when detaching in unit tests.
* added this issue to release notes.

 app/test/test_mbuf.c                   |  9 +++++--
 doc/guides/rel_notes/release_16_07.rst | 11 ++++-----
 lib/librte_mbuf/rte_mbuf.h             | 43 +++++++++++++++++++++++++++++++---
 3 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 98ff93a..2bf05eb 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -438,6 +438,7 @@ test_attach_from_different_pool(void)
 	struct rte_mbuf *clone = NULL;
 	struct rte_mbuf *clone2 = NULL;
 	char *data, *c_data, *c_data2;
+	uint16_t refcnt;
 
 	/* alloc a mbuf */
 	m = rte_pktmbuf_alloc(pktmbuf_pool);
@@ -508,13 +509,17 @@ test_attach_from_different_pool(void)
 		GOTO_FAIL("invalid refcnt in m\n");
 
 	/* detach the clones */
-	rte_pktmbuf_detach(clone);
+	refcnt = rte_pktmbuf_detach2(clone);
 	if (c_data != rte_pktmbuf_mtod(clone, char *))
 		GOTO_FAIL("clone was not detached properly\n");
+	if (refcnt != 2 || rte_mbuf_refcnt_read(m) != 2)
+		GOTO_FAIL("invalid refcnt in m\n");
 
-	rte_pktmbuf_detach(clone2);
+	refcnt = rte_pktmbuf_detach2(clone2);
 	if (c_data2 != rte_pktmbuf_mtod(clone2, char *))
 		GOTO_FAIL("clone2 was not detached properly\n");
+	if (refcnt != 1 || rte_mbuf_refcnt_read(m) != 1)
+		GOTO_FAIL("invalid refcnt in m\n");
 
 	/* free the clones and the initial mbuf */
 	rte_pktmbuf_free(clone2);
diff --git a/doc/guides/rel_notes/release_16_07.rst b/doc/guides/rel_notes/release_16_07.rst
index f6d543c..9678c1f 100644
--- a/doc/guides/rel_notes/release_16_07.rst
+++ b/doc/guides/rel_notes/release_16_07.rst
@@ -77,13 +77,10 @@ Other
 Known Issues
 ------------
 
-This section should contain new known issues in this release. Sample format:
-
-* **Add title in present tense with full stop.**
-
-  Add a short 1-2 sentence description of the known issue in the present
-  tense. Add information on any known workarounds.
-
+* The ``rte_pktmbuf_detach()`` function does not decrement the direct
+  mbuf's reference counter. It leads a memory leak of the direct
+  mbuf. The workaround is to explicitly decrement the reference
+  counter or use ``rte_pktmbuf_detach2()``.
 
 API Changes
 -----------
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 529debb..c0a592d 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1408,6 +1408,7 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
  *
  * After attachment we refer the mbuf we attached as 'indirect',
  * while mbuf we attached to as 'direct'.
+ * The direct mbuf's reference counter is incremented.
  * Right now, not supported:
  *  - attachment for already indirect mbuf (e.g. - mi has to be direct).
  *  - mbuf we trying to attach (mi) is used by someone else
@@ -1459,15 +1460,50 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
 /**
  * Detach an indirect packet mbuf.
  *
+ * Note: It is deprecated.
+ * The direct mbuf's reference counter is not decremented.
+ *
+ *  - restore original mbuf address and length values.
+ *  - reset pktmbuf data and data_len to their default values.
+ *  All other fields of the given packet mbuf will be left intact.
+ *
+ * @param m
+ *   The indirect attached packet mbuf.
+ */
+static inline void __rte_deprecated rte_pktmbuf_detach(struct rte_mbuf *m)
+{
+	struct rte_mempool *mp = m->pool;
+	uint32_t mbuf_size, buf_len, priv_size;
+
+	priv_size = rte_pktmbuf_priv_size(mp);
+	mbuf_size = sizeof(struct rte_mbuf) + priv_size;
+	buf_len = rte_pktmbuf_data_room_size(mp);
+
+	m->priv_size = priv_size;
+	m->buf_addr = (char *)m + mbuf_size;
+	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mbuf_size;
+	m->buf_len = (uint16_t)buf_len;
+	m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
+	m->data_len = 0;
+	m->ol_flags = 0;
+}
+
+/**
+ * Detach an indirect packet mbuf.
+ *
  *  - restore original mbuf address and length values.
  *  - reset pktmbuf data and data_len to their default values.
  *  All other fields of the given packet mbuf will be left intact.
+ *  - decrement the direct mbuf's reference counter.
  *
  * @param m
  *   The indirect attached packet mbuf.
+ * @return
+ *   The updated value of the direct mbuf's reference counter.
  */
-static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
+static inline uint16_t rte_pktmbuf_detach2(struct rte_mbuf *m)
 {
+	struct rte_mbuf *md = rte_mbuf_from_indirect(m);
 	struct rte_mempool *mp = m->pool;
 	uint32_t mbuf_size, buf_len, priv_size;
 
@@ -1482,6 +1518,8 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
 	m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
 	m->data_len = 0;
 	m->ol_flags = 0;
+
+	return rte_mbuf_refcnt_update(md, -1);
 }
 
 static inline struct rte_mbuf* __attribute__((always_inline))
@@ -1497,8 +1535,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 		 */
 		if (RTE_MBUF_INDIRECT(m)) {
 			struct rte_mbuf *md = rte_mbuf_from_indirect(m);
-			rte_pktmbuf_detach(m);
-			if (rte_mbuf_refcnt_update(md, -1) == 0)
+			if (rte_pktmbuf_detach2(m) == 0)
 				__rte_mbuf_raw_free(md);
 		}
 		return m;
-- 
1.9.1

Re: [PATCH v2] mbuf: decrease refcnt when detaching

[Bruce Richardson]

On Tue, May 17, 2016 at 01:53:20AM +0900, Hiroyuki Mikita wrote:
> The rte_pktmbuf_detach() function should decrease refcnt on a direct
> buffer.
> 
> Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>
> ---
> v2:
> * introduced a new function rte_pktmbuf_detach2() which decrease refcnt.
> * marked rte_pktmbuf_detach() as deprecated.
> * added comments about refcnt to rte_pktmbuf_attach() and rte_pktmbuf_detach().
> * checked refcnt when detaching in unit tests.
> * added this issue to release notes.
> 
Rather than detach2, would unattach be better?

/Bruce

Re: [PATCH v2] mbuf: decrease refcnt when detaching

[Ananyev, Konstantin]

Hi Hiroyuki,

> 
> The rte_pktmbuf_detach() function should decrease refcnt on a direct
> buffer.
> 
> Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>
> ---
> v2:
> * introduced a new function rte_pktmbuf_detach2() which decrease refcnt.
> * marked rte_pktmbuf_detach() as deprecated.
> * added comments about refcnt to rte_pktmbuf_attach() and rte_pktmbuf_detach().
> * checked refcnt when detaching in unit tests.
> * added this issue to release notes.
> 
>  app/test/test_mbuf.c                   |  9 +++++--
>  doc/guides/rel_notes/release_16_07.rst | 11 ++++-----
>  lib/librte_mbuf/rte_mbuf.h             | 43 +++++++++++++++++++++++++++++++---
>  3 files changed, 51 insertions(+), 12 deletions(-)
> 
> diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> index 98ff93a..2bf05eb 100644
> --- a/app/test/test_mbuf.c
> +++ b/app/test/test_mbuf.c
> @@ -438,6 +438,7 @@ test_attach_from_different_pool(void)
>  	struct rte_mbuf *clone = NULL;
>  	struct rte_mbuf *clone2 = NULL;
>  	char *data, *c_data, *c_data2;
> +	uint16_t refcnt;
> 
>  	/* alloc a mbuf */
>  	m = rte_pktmbuf_alloc(pktmbuf_pool);
> @@ -508,13 +509,17 @@ test_attach_from_different_pool(void)
>  		GOTO_FAIL("invalid refcnt in m\n");
> 
>  	/* detach the clones */
> -	rte_pktmbuf_detach(clone);
> +	refcnt = rte_pktmbuf_detach2(clone);
>  	if (c_data != rte_pktmbuf_mtod(clone, char *))
>  		GOTO_FAIL("clone was not detached properly\n");
> +	if (refcnt != 2 || rte_mbuf_refcnt_read(m) != 2)
> +		GOTO_FAIL("invalid refcnt in m\n");
> 
> -	rte_pktmbuf_detach(clone2);
> +	refcnt = rte_pktmbuf_detach2(clone2);
>  	if (c_data2 != rte_pktmbuf_mtod(clone2, char *))
>  		GOTO_FAIL("clone2 was not detached properly\n");
> +	if (refcnt != 1 || rte_mbuf_refcnt_read(m) != 1)
> +		GOTO_FAIL("invalid refcnt in m\n");
> 
>  	/* free the clones and the initial mbuf */
>  	rte_pktmbuf_free(clone2);
> diff --git a/doc/guides/rel_notes/release_16_07.rst b/doc/guides/rel_notes/release_16_07.rst
> index f6d543c..9678c1f 100644
> --- a/doc/guides/rel_notes/release_16_07.rst
> +++ b/doc/guides/rel_notes/release_16_07.rst
> @@ -77,13 +77,10 @@ Other
>  Known Issues
>  ------------
> 
> -This section should contain new known issues in this release. Sample format:
> -
> -* **Add title in present tense with full stop.**
> -
> -  Add a short 1-2 sentence description of the known issue in the present
> -  tense. Add information on any known workarounds.
> -
> +* The ``rte_pktmbuf_detach()`` function does not decrement the direct
> +  mbuf's reference counter. It leads a memory leak of the direct
> +  mbuf. The workaround is to explicitly decrement the reference
> +  counter or use ``rte_pktmbuf_detach2()``.
> 
>  API Changes
>  -----------
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 529debb..c0a592d 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1408,6 +1408,7 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
>   *
>   * After attachment we refer the mbuf we attached as 'indirect',
>   * while mbuf we attached to as 'direct'.
> + * The direct mbuf's reference counter is incremented.
>   * Right now, not supported:
>   *  - attachment for already indirect mbuf (e.g. - mi has to be direct).
>   *  - mbuf we trying to attach (mi) is used by someone else
> @@ -1459,15 +1460,50 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
>  /**
>   * Detach an indirect packet mbuf.
>   *
> + * Note: It is deprecated.
> + * The direct mbuf's reference counter is not decremented.
> + *
> + *  - restore original mbuf address and length values.
> + *  - reset pktmbuf data and data_len to their default values.
> + *  All other fields of the given packet mbuf will be left intact.
> + *
> + * @param m
> + *   The indirect attached packet mbuf.
> + */
> +static inline void __rte_deprecated rte_pktmbuf_detach(struct rte_mbuf *m)
> +{
> +	struct rte_mempool *mp = m->pool;
> +	uint32_t mbuf_size, buf_len, priv_size;
> +
> +	priv_size = rte_pktmbuf_priv_size(mp);
> +	mbuf_size = sizeof(struct rte_mbuf) + priv_size;
> +	buf_len = rte_pktmbuf_data_room_size(mp);
> +
> +	m->priv_size = priv_size;
> +	m->buf_addr = (char *)m + mbuf_size;
> +	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mbuf_size;
> +	m->buf_len = (uint16_t)buf_len;
> +	m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
> +	m->data_len = 0;
> +	m->ol_flags = 0;
> +}

I still think it would be good to have a separate function for what rte_pktmbuf_detach()
Is doing right now: restore original values of indirect mbuf.
Probably rename it to rte_pktmbuf_restore() or so and make _detach2() (unatach() ?)
to call it internally. 

> +
> +/**
> + * Detach an indirect packet mbuf.
> + *
>   *  - restore original mbuf address and length values.
>   *  - reset pktmbuf data and data_len to their default values.
>   *  All other fields of the given packet mbuf will be left intact.
> + *  - decrement the direct mbuf's reference counter.
>   *
>   * @param m
>   *   The indirect attached packet mbuf.
> + * @return
> + *   The updated value of the direct mbuf's reference counter.
>   */
> -static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
> +static inline uint16_t rte_pktmbuf_detach2(struct rte_mbuf *m)
>  {
> +	struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>  	struct rte_mempool *mp = m->pool;
>  	uint32_t mbuf_size, buf_len, priv_size;
> 
> @@ -1482,6 +1518,8 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>  	m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
>  	m->data_len = 0;
>  	m->ol_flags = 0;
> +
> +	return rte_mbuf_refcnt_update(md, -1);
>  }
> 
>  static inline struct rte_mbuf* __attribute__((always_inline))
> @@ -1497,8 +1535,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>  		 */
>  		if (RTE_MBUF_INDIRECT(m)) {
>  			struct rte_mbuf *md = rte_mbuf_from_indirect(m);

I don't think there is a need to invoke rte_mbuf_from_indirect() twice.
You can either pass md as a second parameter to _detach2(),
or make detach2() to invoke __rte_mbuf_raw_free()
if rte_mbuf_refcnt_update(md, -1) == 0.
Konstantin

> -			rte_pktmbuf_detach(m);
> -			if (rte_mbuf_refcnt_update(md, -1) == 0)
> +			if (rte_pktmbuf_detach2(m) == 0)
>  				__rte_mbuf_raw_free(md);
>  		}
>  		return m;
> --
> 1.9.1

Re: [PATCH v2] mbuf: decrease refcnt when detaching

[Thomas Monjalon]

2016-05-17 01:53, Hiroyuki Mikita:
> The rte_pktmbuf_detach() function should decrease refcnt on a direct
> buffer.
> 
> Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>
> ---
> v2:
> * introduced a new function rte_pktmbuf_detach2() which decrease refcnt.

As you have noticed, "whenever the indirect buffer is detached,
the reference counter on the direct buffer is decremented."
So the current behaviour of rte_pktmbuf_detach() is buggy.
Why not fix it without renaming?
If you consider this behavioral bug is part of the API, we
can fix it in a new function unattach and deprecate detach.
But Konstantin, why do you want to keep a restore function?
What is the need?

Please explicit the function name for the detach operation in
doc/guides/prog_guide/mbuf_lib.rst (whatever detach2 or unattach).

> * marked rte_pktmbuf_detach() as deprecated.
> * added comments about refcnt to rte_pktmbuf_attach() and rte_pktmbuf_detach().
> * checked refcnt when detaching in unit tests.
> * added this issue to release notes.

Re: [PATCH v2] mbuf: decrease refcnt when detaching

[Ananyev, Konstantin]

Hi Thomas,
 
> > The rte_pktmbuf_detach() function should decrease refcnt on a direct
> > buffer.
> >
> > Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>
> > ---
> > v2:
> > * introduced a new function rte_pktmbuf_detach2() which decrease refcnt.
> 
> As you have noticed, "whenever the indirect buffer is detached,
> the reference counter on the direct buffer is decremented."
> So the current behaviour of rte_pktmbuf_detach() is buggy.
> Why not fix it without renaming?
> If you consider this behavioral bug is part of the API, we
> can fix it in a new function unattach and deprecate detach.
> But Konstantin, why do you want to keep a restore function?
> What is the need?

I think it might be a useful functionality in some situations:
some users can attach/detach to external memory buffers (no mbufs)
and similar functionality is required.
Let say right now examples/vhost/main.c has its own pktmbuf_detach_zcp()
which is doing pretty much the same - restore original values, after detaching
mbuf from external (virtio) memory buffer.
Would be good if we'll use a standard API function here.
Konstantin  

> 
> Please explicit the function name for the detach operation in
> doc/guides/prog_guide/mbuf_lib.rst (whatever detach2 or unattach).
> 
> > * marked rte_pktmbuf_detach() as deprecated.
> > * added comments about refcnt to rte_pktmbuf_attach() and rte_pktmbuf_detach().
> > * checked refcnt when detaching in unit tests.
> > * added this issue to release notes.

Re: [PATCH v2] mbuf: decrease refcnt when detaching

[Thomas Monjalon]

2016-05-17 12:59, Ananyev, Konstantin:
> > > The rte_pktmbuf_detach() function should decrease refcnt on a direct
> > > buffer.
> > 
> > As you have noticed, "whenever the indirect buffer is detached,
> > the reference counter on the direct buffer is decremented."
> > So the current behaviour of rte_pktmbuf_detach() is buggy.
> > Why not fix it without renaming?
> > If you consider this behavioral bug is part of the API, we
> > can fix it in a new function unattach and deprecate detach.
> > But Konstantin, why do you want to keep a restore function?
> > What is the need?
> 
> I think it might be a useful functionality in some situations:
> some users can attach/detach to external memory buffers (no mbufs)
> and similar functionality is required.

Attach to external memory buffer (raw buffer) is not currently supported.

> Let say right now examples/vhost/main.c has its own pktmbuf_detach_zcp()

You should look at the commit http://dpdk.org/commit/68363d85
	"examples/vhost: remove the non-working zero copy code"

> which is doing pretty much the same - restore original values, after detaching
> mbuf from external (virtio) memory buffer.
> Would be good if we'll use a standard API function here.

You are welcome to implement mbuf attach to raw buffer.
But it is not a requirement for this fix.

Re: [PATCH v2] mbuf: decrease refcnt when detaching

[Ananyev, Konstantin]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, May 17, 2016 2:40 PM
> To: Ananyev, Konstantin
> Cc: dev@dpdk.org; Hiroyuki Mikita; olivier.matz@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v2] mbuf: decrease refcnt when detaching
> 
> 2016-05-17 12:59, Ananyev, Konstantin:
> > > > The rte_pktmbuf_detach() function should decrease refcnt on a direct
> > > > buffer.
> > >
> > > As you have noticed, "whenever the indirect buffer is detached,
> > > the reference counter on the direct buffer is decremented."
> > > So the current behaviour of rte_pktmbuf_detach() is buggy.
> > > Why not fix it without renaming?
> > > If you consider this behavioral bug is part of the API, we
> > > can fix it in a new function unattach and deprecate detach.
> > > But Konstantin, why do you want to keep a restore function?
> > > What is the need?
> >
> > I think it might be a useful functionality in some situations:
> > some users can attach/detach to external memory buffers (no mbufs)
> > and similar functionality is required.
> 
> Attach to external memory buffer (raw buffer) is not currently supported.
> 
> > Let say right now examples/vhost/main.c has its own pktmbuf_detach_zcp()
> 
> You should look at the commit http://dpdk.org/commit/68363d85
> 	"examples/vhost: remove the non-working zero copy code"
> 
> > which is doing pretty much the same - restore original values, after detaching
> > mbuf from external (virtio) memory buffer.
> > Would be good if we'll use a standard API function here.
> 
> You are welcome to implement mbuf attach to raw buffer.
> But it is not a requirement for this fix.

Hmm, still not sure why we can't keep an existing function?
Obviously it wouldn't cost anything and I still think might be useful.
Konstantin

Re: [PATCH v2] mbuf: decrease refcnt when detaching

[Thomas Monjalon]

2016-05-17 13:44, Ananyev, Konstantin:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2016-05-17 12:59, Ananyev, Konstantin:
> > > > > The rte_pktmbuf_detach() function should decrease refcnt on a direct
> > > > > buffer.
> > > >
> > > > As you have noticed, "whenever the indirect buffer is detached,
> > > > the reference counter on the direct buffer is decremented."
> > > > So the current behaviour of rte_pktmbuf_detach() is buggy.
> > > > Why not fix it without renaming?
> > > > If you consider this behavioral bug is part of the API, we
> > > > can fix it in a new function unattach and deprecate detach.
> > > > But Konstantin, why do you want to keep a restore function?
> > > > What is the need?
> > >
> > > I think it might be a useful functionality in some situations:
> > > some users can attach/detach to external memory buffers (no mbufs)
> > > and similar functionality is required.
> > 
> > Attach to external memory buffer (raw buffer) is not currently supported.
> > 
> > > Let say right now examples/vhost/main.c has its own pktmbuf_detach_zcp()
> > 
> > You should look at the commit http://dpdk.org/commit/68363d85
> > 	"examples/vhost: remove the non-working zero copy code"
> > 
> > > which is doing pretty much the same - restore original values, after detaching
> > > mbuf from external (virtio) memory buffer.
> > > Would be good if we'll use a standard API function here.
> > 
> > You are welcome to implement mbuf attach to raw buffer.
> > But it is not a requirement for this fix.
> 
> Hmm, still not sure why we can't keep an existing function?

Because it does not do what its name (and doc) suggest.

> Obviously it wouldn't cost anything and I still think might be useful.

It costs to overcomplicate API for only a half support.
If you need the feature "attach to raw", please implement it completely.

Re: [PATCH v2] mbuf: decrease refcnt when detaching

[Ananyev, Konstantin]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, May 17, 2016 3:19 PM
> To: Ananyev, Konstantin
> Cc: dev@dpdk.org; Hiroyuki Mikita; olivier.matz@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v2] mbuf: decrease refcnt when detaching
> 
> 2016-05-17 13:44, Ananyev, Konstantin:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2016-05-17 12:59, Ananyev, Konstantin:
> > > > > > The rte_pktmbuf_detach() function should decrease refcnt on a direct
> > > > > > buffer.
> > > > >
> > > > > As you have noticed, "whenever the indirect buffer is detached,
> > > > > the reference counter on the direct buffer is decremented."
> > > > > So the current behaviour of rte_pktmbuf_detach() is buggy.
> > > > > Why not fix it without renaming?
> > > > > If you consider this behavioral bug is part of the API, we
> > > > > can fix it in a new function unattach and deprecate detach.
> > > > > But Konstantin, why do you want to keep a restore function?
> > > > > What is the need?
> > > >
> > > > I think it might be a useful functionality in some situations:
> > > > some users can attach/detach to external memory buffers (no mbufs)
> > > > and similar functionality is required.
> > >
> > > Attach to external memory buffer (raw buffer) is not currently supported.
> > >
> > > > Let say right now examples/vhost/main.c has its own pktmbuf_detach_zcp()
> > >
> > > You should look at the commit http://dpdk.org/commit/68363d85
> > > 	"examples/vhost: remove the non-working zero copy code"
> > >
> > > > which is doing pretty much the same - restore original values, after detaching
> > > > mbuf from external (virtio) memory buffer.
> > > > Would be good if we'll use a standard API function here.
> > >
> > > You are welcome to implement mbuf attach to raw buffer.
> > > But it is not a requirement for this fix.
> >
> > Hmm, still not sure why we can't keep an existing function?
> 
> Because it does not do what its name (and doc) suggest.
> 
> > Obviously it wouldn't cost anything and I still think might be useful.
> 
> It costs to overcomplicate API for only a half support.

I still think it is better to have it then not, but wouldn't insist here.
Konstantin

> If you need the feature "attach to raw", please implement it completely.

Re: [PATCH v2] mbuf: decrease refcnt when detaching

[Hiroyuki MIKITA]

I think this behavior is not part of the API, it is a bug.

I agree that detach() frees the direct mbuf when refcnt becomes 0,
Konstantin suggests.
It is a right behavior of reference counting.

Regards,
Hiroyuki

2016-05-18 0:45 GMT+09:00 Ananyev, Konstantin <konstantin.ananyev@intel.com>:
>
>
>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>> Sent: Tuesday, May 17, 2016 3:19 PM
>> To: Ananyev, Konstantin
>> Cc: dev@dpdk.org; Hiroyuki Mikita; olivier.matz@6wind.com
>> Subject: Re: [dpdk-dev] [PATCH v2] mbuf: decrease refcnt when detaching
>>
>> 2016-05-17 13:44, Ananyev, Konstantin:
>> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>> > > 2016-05-17 12:59, Ananyev, Konstantin:
>> > > > > > The rte_pktmbuf_detach() function should decrease refcnt on a direct
>> > > > > > buffer.
>> > > > >
>> > > > > As you have noticed, "whenever the indirect buffer is detached,
>> > > > > the reference counter on the direct buffer is decremented."
>> > > > > So the current behaviour of rte_pktmbuf_detach() is buggy.
>> > > > > Why not fix it without renaming?
>> > > > > If you consider this behavioral bug is part of the API, we
>> > > > > can fix it in a new function unattach and deprecate detach.
>> > > > > But Konstantin, why do you want to keep a restore function?
>> > > > > What is the need?
>> > > >
>> > > > I think it might be a useful functionality in some situations:
>> > > > some users can attach/detach to external memory buffers (no mbufs)
>> > > > and similar functionality is required.
>> > >
>> > > Attach to external memory buffer (raw buffer) is not currently supported.
>> > >
>> > > > Let say right now examples/vhost/main.c has its own pktmbuf_detach_zcp()
>> > >
>> > > You should look at the commit http://dpdk.org/commit/68363d85
>> > >   "examples/vhost: remove the non-working zero copy code"
>> > >
>> > > > which is doing pretty much the same - restore original values, after detaching
>> > > > mbuf from external (virtio) memory buffer.
>> > > > Would be good if we'll use a standard API function here.
>> > >
>> > > You are welcome to implement mbuf attach to raw buffer.
>> > > But it is not a requirement for this fix.
>> >
>> > Hmm, still not sure why we can't keep an existing function?
>>
>> Because it does not do what its name (and doc) suggest.
>>
>> > Obviously it wouldn't cost anything and I still think might be useful.
>>
>> It costs to overcomplicate API for only a half support.
>
> I still think it is better to have it then not, but wouldn't insist here.
> Konstantin
>
>> If you need the feature "attach to raw", please implement it completely.
>

[PATCH v3] mbuf: decrease refcnt when detaching

[Hiroyuki Mikita]

The rte_pktmbuf_detach() function should decrease refcnt on a direct
buffer.

Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>
---
v3:
* fixed rte_pktmbuf_detach() to decrease refcnt.
* free the direct mbuf when refcnt becomes 0.
* added this issue to Resolved Issues in release notes.

v2:
* introduced a new function rte_pktmbuf_detach2() which decrease refcnt.
* marked rte_pktmbuf_detach() as deprecated.
* added comments about refcnt to rte_pktmbuf_attach() and rte_pktmbuf_detach().
* checked refcnt when detaching in unit tests.
* added this issue to release notes.

 app/test/test_mbuf.c                   |  4 ++++
 doc/guides/rel_notes/release_16_07.rst |  6 ++++++
 lib/librte_mbuf/rte_mbuf.h             | 17 ++++++++++-------
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 98ff93a..8460db7 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -511,10 +511,14 @@ test_attach_from_different_pool(void)
 	rte_pktmbuf_detach(clone);
 	if (c_data != rte_pktmbuf_mtod(clone, char *))
 		GOTO_FAIL("clone was not detached properly\n");
+	if (rte_mbuf_refcnt_read(m) != 2)
+		GOTO_FAIL("invalid refcnt in m\n");
 
 	rte_pktmbuf_detach(clone2);
 	if (c_data2 != rte_pktmbuf_mtod(clone2, char *))
 		GOTO_FAIL("clone2 was not detached properly\n");
+	if (rte_mbuf_refcnt_read(m) != 1)
+		GOTO_FAIL("invalid refcnt in m\n");
 
 	/* free the clones and the initial mbuf */
 	rte_pktmbuf_free(clone2);
diff --git a/doc/guides/rel_notes/release_16_07.rst b/doc/guides/rel_notes/release_16_07.rst
index f6d543c..8283373 100644
--- a/doc/guides/rel_notes/release_16_07.rst
+++ b/doc/guides/rel_notes/release_16_07.rst
@@ -65,6 +65,12 @@ Drivers
 Libraries
 ~~~~~~~~~
 
+* **mbuf: Fixed refcnt update when detaching.**
+
+  Fix the ``rte_pktmbuf_detach()`` function to decrement the direct
+  mbuf's reference counter. The previous behavior was not to affect
+  the reference counter. It lead a memory leak of the direct mbuf.
+
 
 Examples
 ~~~~~~~~
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 529debb..299b60e 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1408,6 +1408,8 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
  *
  * After attachment we refer the mbuf we attached as 'indirect',
  * while mbuf we attached to as 'direct'.
+ * The direct mbuf's reference counter is incremented.
+ * 
  * Right now, not supported:
  *  - attachment for already indirect mbuf (e.g. - mi has to be direct).
  *  - mbuf we trying to attach (mi) is used by someone else
@@ -1462,12 +1464,15 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
  *  - restore original mbuf address and length values.
  *  - reset pktmbuf data and data_len to their default values.
  *  All other fields of the given packet mbuf will be left intact.
+ *  - decrement the direct mbuf's reference counter.
+ *  When the reference counter becomes 0, the direct mbuf is freed.
  *
  * @param m
  *   The indirect attached packet mbuf.
  */
 static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
 {
+	struct rte_mbuf *md = rte_mbuf_from_indirect(m);
 	struct rte_mempool *mp = m->pool;
 	uint32_t mbuf_size, buf_len, priv_size;
 
@@ -1482,6 +1487,10 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
 	m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
 	m->data_len = 0;
 	m->ol_flags = 0;
+
+	if (rte_mbuf_refcnt_update(md, -1) == 0) {
+		__rte_mbuf_raw_free(md);
+	}
 }
 
 static inline struct rte_mbuf* __attribute__((always_inline))
@@ -1491,15 +1500,9 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 
 	if (likely(rte_mbuf_refcnt_update(m, -1) == 0)) {
 
-		/* if this is an indirect mbuf, then
-		 *  - detach mbuf
-		 *  - free attached mbuf segment
-		 */
+		/* if this is an indirect mbuf, it is detached. */
 		if (RTE_MBUF_INDIRECT(m)) {
-			struct rte_mbuf *md = rte_mbuf_from_indirect(m);
 			rte_pktmbuf_detach(m);
-			if (rte_mbuf_refcnt_update(md, -1) == 0)
-				__rte_mbuf_raw_free(md);
 		}
 		return m;
 	}
-- 
1.9.1

Re: [PATCH v3] mbuf: decrease refcnt when detaching

[Olivier Matz]

Hi Hiroyuki,

Thanks for submitting a new version.

There are some styling issues in the patch, I highlighted them below
but you can check them by using checkpatch:

  DPDK_CHECKPATCH_PATH=/path/to/linux/checkpatch.pl \
    scripts/checkpatches.sh file.patch


On 05/17/2016 06:35 PM, Hiroyuki Mikita wrote:
> The rte_pktmbuf_detach() function should decrease refcnt on a direct
> buffer.
> 
> Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>
> 
> [...]
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 529debb..299b60e 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1408,6 +1408,8 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
>   *
>   * After attachment we refer the mbuf we attached as 'indirect',
>   * while mbuf we attached to as 'direct'.
> + * The direct mbuf's reference counter is incremented.
> + * 

ERROR:TRAILING_WHITESPACE: trailing whitespace
#82: FILE: lib/librte_mbuf/rte_mbuf.h:1412:
+ * $


>   * Right now, not supported:
>   *  - attachment for already indirect mbuf (e.g. - mi has to be direct).
>   *  - mbuf we trying to attach (mi) is used by someone else
> @@ -1462,12 +1464,15 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
>   *  - restore original mbuf address and length values.
>   *  - reset pktmbuf data and data_len to their default values.
>   *  All other fields of the given packet mbuf will be left intact.
> + *  - decrement the direct mbuf's reference counter.
> + *  When the reference counter becomes 0, the direct mbuf is freed.

Minor comment here: I think something like that would be clearer:

  *  - restore original mbuf address and length values.
  *  - reset pktmbuf data and data_len to their default values.
  *  - decrement the direct mbuf's reference counter. When the
  *    reference counter becomes 0, the direct mbuf is freed.
  *
  * All other fields of the given packet mbuf will be left intact.


>   *
>   * @param m
>   *   The indirect attached packet mbuf.
>   */
>  static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>  {
> +	struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>  	struct rte_mempool *mp = m->pool;
>  	uint32_t mbuf_size, buf_len, priv_size;
>  
> @@ -1482,6 +1487,10 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>  	m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
>  	m->data_len = 0;
>  	m->ol_flags = 0;
> +
> +	if (rte_mbuf_refcnt_update(md, -1) == 0) {
> +		__rte_mbuf_raw_free(md);
> +	}
>  }

WARNING:BRACES: braces {} are not necessary for single statement blocks
#107: FILE: lib/librte_mbuf/rte_mbuf.h:1491:
+       if (rte_mbuf_refcnt_update(md, -1) == 0) {
+               __rte_mbuf_raw_free(md);
+       }


>  
>  static inline struct rte_mbuf* __attribute__((always_inline))
> @@ -1491,15 +1500,9 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>  
>  	if (likely(rte_mbuf_refcnt_update(m, -1) == 0)) {
>  
> -		/* if this is an indirect mbuf, then
> -		 *  - detach mbuf
> -		 *  - free attached mbuf segment
> -		 */
> +		/* if this is an indirect mbuf, it is detached. */
>  		if (RTE_MBUF_INDIRECT(m)) {
> -			struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>  			rte_pktmbuf_detach(m);
> -			if (rte_mbuf_refcnt_update(md, -1) == 0)
> -				__rte_mbuf_raw_free(md);
>  		}
>  		return m;
>  	}
> 

It's not seen by checkpatch, but the braces could also be removed
here.

Apart from this, it looks good to me. I'm ok with not providing a
compat function as we could consider it was a bug.

Thanks for working on this.

Olivier

Re: [PATCH v3] mbuf: decrease refcnt when detaching

[Hiroyuki Mikita]

Hi Olivier,

Thanks for reviewing.

I am fixing the patch to follow your comments.

Regards,
Hiroyuki

2016-05-18 20:58 GMT+09:00 Olivier Matz <olivier.matz@6wind.com>:
> Hi Hiroyuki,
>
> Thanks for submitting a new version.
>
> There are some styling issues in the patch, I highlighted them below
> but you can check them by using checkpatch:
>
>   DPDK_CHECKPATCH_PATH=/path/to/linux/checkpatch.pl \
>     scripts/checkpatches.sh file.patch
>
>
> On 05/17/2016 06:35 PM, Hiroyuki Mikita wrote:
>> The rte_pktmbuf_detach() function should decrease refcnt on a direct
>> buffer.
>>
>> Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>
>>
>> [...]
>>
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index 529debb..299b60e 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -1408,6 +1408,8 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
>>   *
>>   * After attachment we refer the mbuf we attached as 'indirect',
>>   * while mbuf we attached to as 'direct'.
>> + * The direct mbuf's reference counter is incremented.
>> + *
>
> ERROR:TRAILING_WHITESPACE: trailing whitespace
> #82: FILE: lib/librte_mbuf/rte_mbuf.h:1412:
> + * $
>
>
>>   * Right now, not supported:
>>   *  - attachment for already indirect mbuf (e.g. - mi has to be direct).
>>   *  - mbuf we trying to attach (mi) is used by someone else
>> @@ -1462,12 +1464,15 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
>>   *  - restore original mbuf address and length values.
>>   *  - reset pktmbuf data and data_len to their default values.
>>   *  All other fields of the given packet mbuf will be left intact.
>> + *  - decrement the direct mbuf's reference counter.
>> + *  When the reference counter becomes 0, the direct mbuf is freed.
>
> Minor comment here: I think something like that would be clearer:
>
>   *  - restore original mbuf address and length values.
>   *  - reset pktmbuf data and data_len to their default values.
>   *  - decrement the direct mbuf's reference counter. When the
>   *    reference counter becomes 0, the direct mbuf is freed.
>   *
>   * All other fields of the given packet mbuf will be left intact.
>
>
>>   *
>>   * @param m
>>   *   The indirect attached packet mbuf.
>>   */
>>  static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>>  {
>> +     struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>>       struct rte_mempool *mp = m->pool;
>>       uint32_t mbuf_size, buf_len, priv_size;
>>
>> @@ -1482,6 +1487,10 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>>       m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
>>       m->data_len = 0;
>>       m->ol_flags = 0;
>> +
>> +     if (rte_mbuf_refcnt_update(md, -1) == 0) {
>> +             __rte_mbuf_raw_free(md);
>> +     }
>>  }
>
> WARNING:BRACES: braces {} are not necessary for single statement blocks
> #107: FILE: lib/librte_mbuf/rte_mbuf.h:1491:
> +       if (rte_mbuf_refcnt_update(md, -1) == 0) {
> +               __rte_mbuf_raw_free(md);
> +       }
>
>
>>
>>  static inline struct rte_mbuf* __attribute__((always_inline))
>> @@ -1491,15 +1500,9 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>>
>>       if (likely(rte_mbuf_refcnt_update(m, -1) == 0)) {
>>
>> -             /* if this is an indirect mbuf, then
>> -              *  - detach mbuf
>> -              *  - free attached mbuf segment
>> -              */
>> +             /* if this is an indirect mbuf, it is detached. */
>>               if (RTE_MBUF_INDIRECT(m)) {
>> -                     struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>>                       rte_pktmbuf_detach(m);
>> -                     if (rte_mbuf_refcnt_update(md, -1) == 0)
>> -                             __rte_mbuf_raw_free(md);
>>               }
>>               return m;
>>       }
>>
>
> It's not seen by checkpatch, but the braces could also be removed
> here.
>
> Apart from this, it looks good to me. I'm ok with not providing a
> compat function as we could consider it was a bug.
>
> Thanks for working on this.
>
> Olivier
>

[PATCH v4] mbuf: decrease refcnt when detaching

[Hiroyuki Mikita]

The rte_pktmbuf_detach() function should decrease refcnt on a direct
buffer.

Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>
---
v4:
* fixed formatting.
* fixed doxygen comments of rte_pktmbuf_detach() to be clearer.

v3:
* fixed rte_pktmbuf_detach() to decrease refcnt.
* free the direct mbuf when refcnt becomes 0.
* added this issue to Resolved Issues in release notes.

v2:
* introduced a new function rte_pktmbuf_detach2() which decrease refcnt.
* marked rte_pktmbuf_detach() as deprecated.
* added comments about refcnt to rte_pktmbuf_attach() and rte_pktmbuf_detach().
* checked refcnt when detaching in unit tests.
* added this issue to release notes.

 app/test/test_mbuf.c                   |  4 ++++
 doc/guides/rel_notes/release_16_07.rst |  6 ++++++
 lib/librte_mbuf/rte_mbuf.h             | 23 ++++++++++++-----------
 3 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 98ff93a..8460db7 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -511,10 +511,14 @@ test_attach_from_different_pool(void)
 	rte_pktmbuf_detach(clone);
 	if (c_data != rte_pktmbuf_mtod(clone, char *))
 		GOTO_FAIL("clone was not detached properly\n");
+	if (rte_mbuf_refcnt_read(m) != 2)
+		GOTO_FAIL("invalid refcnt in m\n");
 
 	rte_pktmbuf_detach(clone2);
 	if (c_data2 != rte_pktmbuf_mtod(clone2, char *))
 		GOTO_FAIL("clone2 was not detached properly\n");
+	if (rte_mbuf_refcnt_read(m) != 1)
+		GOTO_FAIL("invalid refcnt in m\n");
 
 	/* free the clones and the initial mbuf */
 	rte_pktmbuf_free(clone2);
diff --git a/doc/guides/rel_notes/release_16_07.rst b/doc/guides/rel_notes/release_16_07.rst
index 58c8ef9..a8c50c8 100644
--- a/doc/guides/rel_notes/release_16_07.rst
+++ b/doc/guides/rel_notes/release_16_07.rst
@@ -69,6 +69,12 @@ Drivers
 Libraries
 ~~~~~~~~~
 
+* **mbuf: Fixed refcnt update when detaching.**
+
+  Fix the ``rte_pktmbuf_detach()`` function to decrement the direct
+  mbuf's reference counter. The previous behavior was not to affect
+  the reference counter. It lead a memory leak of the direct mbuf.
+
 
 Examples
 ~~~~~~~~
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 7b92b88..48911a6 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1412,6 +1412,8 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
  *
  * After attachment we refer the mbuf we attached as 'indirect',
  * while mbuf we attached to as 'direct'.
+ * The direct mbuf's reference counter is incremented.
+ *
  * Right now, not supported:
  *  - attachment for already indirect mbuf (e.g. - mi has to be direct).
  *  - mbuf we trying to attach (mi) is used by someone else
@@ -1465,13 +1467,17 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
  *
  *  - restore original mbuf address and length values.
  *  - reset pktmbuf data and data_len to their default values.
- *  All other fields of the given packet mbuf will be left intact.
+ *  - decrement the direct mbuf's reference counter. When the
+ *  reference counter becomes 0, the direct mbuf is freed.
+ *
+ * All other fields of the given packet mbuf will be left intact.
  *
  * @param m
  *   The indirect attached packet mbuf.
  */
 static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
 {
+	struct rte_mbuf *md = rte_mbuf_from_indirect(m);
 	struct rte_mempool *mp = m->pool;
 	uint32_t mbuf_size, buf_len, priv_size;
 
@@ -1486,6 +1492,9 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
 	m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
 	m->data_len = 0;
 	m->ol_flags = 0;
+
+	if (rte_mbuf_refcnt_update(md, -1) == 0)
+		__rte_mbuf_raw_free(md);
 }
 
 static inline struct rte_mbuf* __attribute__((always_inline))
@@ -1494,17 +1503,9 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 	__rte_mbuf_sanity_check(m, 0);
 
 	if (likely(rte_mbuf_refcnt_update(m, -1) == 0)) {
-
-		/* if this is an indirect mbuf, then
-		 *  - detach mbuf
-		 *  - free attached mbuf segment
-		 */
-		if (RTE_MBUF_INDIRECT(m)) {
-			struct rte_mbuf *md = rte_mbuf_from_indirect(m);
+		/* if this is an indirect mbuf, it is detached. */
+		if (RTE_MBUF_INDIRECT(m))
 			rte_pktmbuf_detach(m);
-			if (rte_mbuf_refcnt_update(md, -1) == 0)
-				__rte_mbuf_raw_free(md);
-		}
 		return m;
 	}
 	return NULL;
-- 
1.9.1

Re: [PATCH v4] mbuf: decrease refcnt when detaching

[Olivier Matz]

Hi Hiroyuki,

On 05/18/2016 04:41 PM, Hiroyuki Mikita wrote:
> The rte_pktmbuf_detach() function should decrease refcnt on a direct
> buffer.
> 
> Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>


Thanks

Re: [PATCH v4] mbuf: decrease refcnt when detaching

[Thomas Monjalon]

> > The rte_pktmbuf_detach() function should decrease refcnt on a direct
> > buffer.
> > 
> > Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied with the doc reference in the commit message, thanks.