Re: [dpdk-dev] [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Olivier,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Thursday, March 19, 2015 8:13 AM
> To: Ananyev, Konstantin; vadim.suraev@gmail.com
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest
> 
> Hi Konstantin,
> 
> On 03/18/2015 04:13 PM, Ananyev, Konstantin wrote:
> >
> >> From: Vadim Suraev [mailto:vadim.suraev@gmail.com]
> >> Sent: Wednesday, March 18, 2015 10:41 AM
> >> To: Ananyev, Konstantin
> >> Cc: dev@dpdk.org
> >> Subject: Re: [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest
> >>
> >> 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.
Konstantin


> 
> Thanks,
> Olivier