[dpdk-dev] [PATCH 2/2] Remove RTE_MBUF_REFCNT references

[Olivier MATZ]

Hi,

On 02/18/2015 11:22 AM, Bruce Richardson wrote:
> On Wed, Feb 18, 2015 at 11:14:42AM +0100, Olivier MATZ wrote:
>> On 02/18/2015 11:00 AM, Bruce Richardson wrote:
>>> On Wed, Feb 18, 2015 at 09:48:58AM +0000, Ananyev, Konstantin wrote:
>>>> Hi lads,
>>>>
>>>>> -----Original Message-----
>>>>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Bruce Richardson
>>>>> Sent: Wednesday, February 18, 2015 9:36 AM
>>>>> To: Olivier MATZ
>>>>> Cc: dev at dpdk.org
>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] Remove RTE_MBUF_REFCNT references
>>>>>
>>>>> On Wed, Feb 18, 2015 at 10:16:56AM +0100, Olivier MATZ wrote:
>>>>>> Hi Sergio,
>>>>>>
>>>>>> On 02/16/2015 05:08 PM, Sergio Gonzalez Monroy wrote:
>>>>>>> This patch removes all references to RTE_MBUF_REFCNT, setting the refcnt
>>>>>>> field in the mbuf struct permanently.
>>>>>>>
>>>>>>> Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy at intel.com>
>>>>>>
>>>>>> I think removing the refcount compile option goes in the right
>>>>>> direction. However, activating the refcount will break the applications
>>>>>> that reserve a private zone in mbufs. This is due to the macros
>>>>>> RTE_MBUF_TO_BADDR() and RTE_MBUF_FROM_BADDR() that suppose that
>>>>>> the beginning of the mbuf is 128 bytes (sizeof mbuf) before the
>>>>>> data buffer.
>>>>>>
>>>>>
>>>>> While I understand how the macros make certain assumptions, how does activating
>>>>> the refcnt specifically lead to the problems you describe? Could you explain
>>>>> that part in a bit more detail?
>>>>>
>>>>> Thanks,
>>>>> /Bruce
>>>>>
>>>>
>>>> Olivier, I also don't understand your concern here.
>>>> As I can see, that patch has nothing to do with RTE_MBUF_FROM_BADDR/ RTE_MBUF_FROM_BADDR macros.
>>>> They are still there, for example rte_pktmbuf_detach() still uses it to restore mbuf's buf_addr.
>>>> The only principal change here, is that we don't rely more  on RTE_MBUF_FROM_BADDR to determine,
>>>> Is that indirect mbuf or not.
>>>> Instead we use a special falg for that purpose:
>>>>
>>>> -#define RTE_MBUF_INDIRECT(mb)   (RTE_MBUF_FROM_BADDR((mb)->buf_addr) != (mb))
>>>> +#define RTE_MBUF_INDIRECT(mb)   (mb->ol_flags & IND_ATTACHED_MBUF)
>>>>
>>>> BTW, Sergio as I said before, I think it should be:
>>>> #define RTE_MBUF_INDIRECT(mb)   ((mb)->ol_flags & IND_ATTACHED_MBUF)
>>>>
>>>> Konstantin
>>>>
>>>>
>>>>>> For RTE_MBUF_TO_BADDR(), it's relatively easy to replace it. The
>>>>>> mbuf pool could store the size of the private size like it's done
>>>>>> for mbp_priv->mbuf_data_room_size. Using rte_mempool_from_obj(m)
>>>>>> or m->pool, we can retrieve the mbuf pool and this value, then
>>>>>> compute the buffer address.
>>>
>>> Agreed, that makes sense.
>>>
>>>>>>
>>>>>> For RTE_MBUF_FROM_BADDR(), it's more complex. We could ensure that
>>>>>> a backpointer to the mbuf is always located before the data buffer,
>>>>>> but it looks difficult to do.
>>>
>>> On the other hand, with the proposed refcnt change Sergio proposes, we no
>>> longer use this macro in any of the built-in mbuf handling for freeing mbufs.
>>> Does this need to be solved at anything other than the application level?
>>
>> It's still used in __rte_pktmbuf_prefree_seg() to retrieve the
>> parent mbuf (direct) from the indirect mbuf beeing freed.
>>
> Yes, my bad.
> How was this managed before, since refcnt field seems to be necessary in order
> to effectively manage indirect mbufs? Is this just the case that this is something
> that never worked and that needs to be solved, or is it something that was
> working that this patch will now break?

This is something that never worked before: refcounts are not compatible
with reserving private data in mbufs. This patch does not change the
issue, it is still there.

Before the patch, an application that wanted to reserve a private
data could disable refcounts at compile-time.
After the patch, the solution is just to avoid using refcounts.

Regards,
Olivier