Hi Olivier, > -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz at 6wind.com] > Sent: Thursday, March 19, 2015 8:13 AM > To: Ananyev, Konstantin; vadim.suraev at gmail.com > Cc: dev at 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 at gmail.com] > >> Sent: Wednesday, March 18, 2015 10:41 AM > >> To: Ananyev, Konstantin > >> Cc: dev at 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
