[dpdk-dev] [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest
Olivier MATZ
olivier.matz at 6wind.com
Thu Mar 19 11:54:55 CET 2015
Hi Konstantin,
On 03/19/2015 11:47 AM, Ananyev, Konstantin wrote:
>>>> Hi, Konstantin,
>>>>
>>>> Got it. To make the same, nulling the next should be inside of the block as you said.
>>>> One question raises here: If a segment in the chain has refcnt > 1 (so its next is not assigned NULL), and the next segment has
>> refcnt
>>>> == 1 (so it is freed), do you think this scenario is real/should be considered? If so, the former can be safely freed only by calling
>>>> rte_pktmbuf_free_seg which does not iterate. So why to keep next pointing to something?
>>>
>>> I think we need it, not just to keep things the same with rte_pktmbuf_free(), but because it is a right thing to do.
>>> Let say you have a packet in 2 mbufs chained together, both mbufs have refcnt==2.
>>> Then:
>>> rte_pktmbuf_free(firs_mbuf);
>>> rte_pktmbuf_free(firs_mbuf);
>>>
>>> Would work correctly and free both mbufs back to the mempool.
>>>
>>> While after:
>>> rte_pktmbuf_free_chain(first_mbuf);
>>> rte_pktmbuf_free_chain(first_mbuf);
>>>
>>> We would have first_mbuf freed back into the mempool, while second would get lost(memory leaking).
>>> Basically free() shouldn't modify any filed inside mbuf, except refcnt if rte_mbuf_refcnt_update(m, -1) > 0
>>>
>>> About your case, when: first_mbuf->refcnt==2 and second_mbuf->refcnt==1.
>>> Right now, rte_pktmbuf_free() can't handle such cases properly,
>>> and, as I know, such situation is not considered as valid one.
>>
>> I'm not sure I understand what you are saying. To me, the case you are
>> describing is similar to the case below, and it should work properly:
>>
>> /* allocate a packet and clone it. After that, m1 has a
>> * refcnt of 2 */
>> m1 = rte_pktmbuf_alloc();
>> clone1 = rte_pktmbuf_clone(m1);
>>
>> /* allocate another packet */
>> m2 = rte_pktmbuf_alloc();
>>
>> /* chain m2 after m1, updating fields like total length.
>> * After that, m1 has 2 segments, the first one has a refcnt
>> * of 1 and the second has a refcnt of 2 */
>> mbuf_concat(m1, m2);
>>
>> /* This will decrement the refcnt on the first segment and
>> * free the second segment */
>> rte_pktmbuf_free(m1);
>>
>> /* free the indirect mbuf, and as the refcnt is 1 on the
>> * direct mbuf (m1), also release it */
>> rte_pktmbuf_free(clone1);
>>
>> Am I missing something?
>
> The scenario you described would work I believe, as second reference for m1 is from indirect mbuf.
> So rte_pktmbuf_free(clone1) would just call __rte_mbuf_raw_free(m1).
>
> The scenario I am talking about is:
> No indirect mbufs pointing to m1 data buffer.
> m1->next == m2; m1->refcnt==2;
> m2->next == NULL; m2->rectn==1;
>
> And then:
> rte_pktmbuf_free(m1); //after that m2 is freed, but m1->next == m2
> rte_pktmbuf_free(m1); //would call rte_pktmbuf_free_seg(m2)
>
> That one would not work correctly, and I think considered as invalid case right now.
Ok, I agree this is invalid and should not happen.
Thanks,
Olivier
More information about the dev
mailing list