[dpdk-dev] mbuf: decrease refcnt when detaching

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

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

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
>
  

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

> >
  

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
  

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
  

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
>