From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier MATZ Subject: Re: [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest Date: Thu, 19 Mar 2015 11:54:55 +0100 Message-ID: <550AAAFF.2010905@6wind.com> References: <1426628169-1735-1-git-send-email-vadim.suraev@gmail.com> <2601191342CEEE43887BDE71AB977258213F6F10@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258213F7053@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258213F706D@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258213F7136@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258213F7188@irsmsx105.ger.corp.intel.com> <550A850D.9010309@6wind.com> <2601191342CEEE43887BDE71AB977258213F749B@irsmsx105.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: "dev-VfR2kkLFssw@public.gmane.org" To: "Ananyev, Konstantin" , "vadim.suraev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org" Return-path: In-Reply-To: <2601191342CEEE43887BDE71AB977258213F749B-pww93C2UFcwu0RiL9chJVbfspsVTdybXVpNB7YpNyf8@public.gmane.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" 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