From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ob0-f174.google.com (mail-ob0-f174.google.com [209.85.214.174]) by dpdk.org (Postfix) with ESMTP id D9A2E37A4 for ; Mon, 23 Mar 2015 18:31:19 +0100 (CET) Received: by obcjt1 with SMTP id jt1so108265931obc.2 for ; Mon, 23 Mar 2015 10:31:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=ZoCmyhnaiT5DPVeIwKQl2+axv4ZRzfMRcbEp3Cpblj4=; b=W3/3iX6ou2lQ240MQ/ojH8/RxovmU5qeBf5x6WVawxHKoo2w3Py7NEU/euQ8aO7Hpe BiOzEGbi0hP1qwVM3wcBBD3xfHXSPUNYP7zGuqnWK6KEXwCgGHV9wFbMNggguF8xa4/U KXeYfNlJbV0uNHWf0/Ykbw3BIwhDFsCh+YLdR93PuiHAh179sJpWzdYGluOx4+XUKbVF ob0Yz+q/lhLHuIxlqaY3ZL1G8CWYV2EqVWsnVUGLOkdaYai+oI+Vq1Wo6IwltsnP7kll 3BocQtPj6sfgxfApF2ZsoKP76EqZq/QWsAwmQy8juGJxieCx17sj5dRPNT8YaKxNgKHX 1Ekw== MIME-Version: 1.0 X-Received: by 10.60.124.42 with SMTP id mf10mr125772oeb.81.1427131879305; Mon, 23 Mar 2015 10:31:19 -0700 (PDT) Received: by 10.202.105.138 with HTTP; Mon, 23 Mar 2015 10:31:19 -0700 (PDT) Received: by 10.202.105.138 with HTTP; Mon, 23 Mar 2015 10:31:19 -0700 (PDT) In-Reply-To: <551042D2.5040300@6wind.com> References: <1426710078-11230-1-git-send-email-vadim.suraev@gmail.com> <20150318205856.GA5151@neilslaptop.think-freely.org> <550A8BB5.9040104@6wind.com> <20150319131639.GB1992@hmsreliant.think-freely.org> <551042D2.5040300@6wind.com> Date: Mon, 23 Mar 2015 19:31:19 +0200 Message-ID: From: Vadim Suraev To: Olivier MATZ Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v2] rte_mbuf: mbuf bulk alloc/free functions added + unittest X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Mar 2015 17:31:20 -0000 Hi, Olivier, No, I personally need to free a chain using mempool bulk. If I'm not mistaken, it has been proposed by one of reviewers to have lower level function, so it was done (I'm sorry if misunderstood) Regards, Vadim. On Mar 23, 2015 8:44 PM, "Olivier MATZ" wrote: > Hi Neil, > > On 03/19/2015 02:16 PM, Neil Horman wrote: > >> On 03/18/2015 09:58 PM, Neil Horman wrote: > >>>> +/** > >>>> + * Free a bulk of mbufs into its original mempool. > >>>> + * This function assumes: > >>>> + * - refcnt equals 1 > >>>> + * - mbufs are direct > >>>> + * - all mbufs must belong to the same mempool > >>>> + * > >>>> + * @param mbufs > >>>> + * Array of pointers to mbuf > >>>> + * @param count > >>>> + * Array size > >>>> + */ > >>>> +static inline void rte_pktmbuf_bulk_free(struct rte_mbuf **mbufs, > >>>> + unsigned count) > >>>> +{ > >>>> + unsigned idx; > >>>> + > >>>> + RTE_MBUF_ASSERT(count > 0); > >>>> + > >>>> + for (idx = 0; idx < count; idx++) { > >>>> + RTE_MBUF_ASSERT(mbufs[idx]->pool == mbufs[0]->pool); > >>>> + RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(mbufs[idx]) == 1); > >>>> + rte_mbuf_refcnt_set(mbufs[idx], 0); > >>> This is really a misuse of the API. The entire point of reference > counting is > >>> to know when an mbuf has no more references and can be freed. By > forcing all > >>> the reference counts to zero here, you allow the refcnt infrastructure > to be > >>> circumvented, causing memory leaks. > >>> > >>> I think what you need to do here is enhance the underlying pktmbuf > interface > >>> such that an rte_mbuf structure has a destructor method association > with it > >>> which is called when its refcnt reaches zero. That way the > >>> rte_pktmbuf_bulk_free function can just decrement the refcnt on each > >>> mbuf_structure, and the pool as a whole can be returned when the > destructor > >>> function discovers that all mbufs in that bulk pool are freed. > >> > >> I don't really understand what's the problem here. The API explicitly > >> describes the conditions for calling this functions: the segments are > >> directs, they belong to the same mempool, and their refcnt is 1. > >> > >> This function could be useful in a driver which knows that the mbuf > >> it allocated matches this conditions. I think an application that > >> only uses direct mbufs and one mempool could also use this function. > > > > > > That last condition is my issue with this patch, that the user has to > know what > > refcnts are. It makes this api useful for little more than the test > case that > > is provided with it. Its irritating enough that for singly allocated > mbufs the > > user has to know what the refcount of a buffer is before freeing, but at > least > > they can macrotize a {rte_pktmbuf_refcnt_update; > if(rte_pktmbuf_refct_read) then > > free} operation. > > > > With this, you've placed the user in charge of not only doing that, but > also of > > managing the relationship between pktmbufs and the pool they came from. > while > > that makes sense for the test case, it really doesn't in any general use > case in > > which packet processing is ever deferred or queued, because it means > that the > > application is now responsible for holding a pointer to every packet it > > allocates and checking its refcount periodically until it completes. > > > > There is never any reason that an application won't need to do this > management, > > so making it the purview of the application to handle rather than > properly > > integrating that functionality in the library is really a false savings. > > There are some places where you know that the prerequisites are met, > so you can save cycles by using this function. > > From what I imagine, if in a driver you allocate mbufs, chain them and > for some reason you realize you have to free them, you can use this > function instead of freeing them one by one. > > Also, as it's up to the application to decide how many mbuf pools are > created, and whether indirect mbufs are used or not, the application > can take the short path of using this function in some conditions. > > Vadim, maybe you have another reason or use case for adding this > function? Could you detail why you need it and how it improves your > use case? > > Regards, > Olivier >