From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 245E15A35 for ; Wed, 1 Apr 2015 17:18:38 +0200 (CEST) Received: from [185.65.185.242] (helo=[192.168.1.30]) by mail.droids-corp.org with esmtpsa (TLS1.2:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1YdKTd-0006df-0o; Wed, 01 Apr 2015 17:22:48 +0200 Message-ID: <551C0C42.6080307@6wind.com> Date: Wed, 01 Apr 2015 17:18:26 +0200 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.5.0 MIME-Version: 1.0 To: "Ananyev, Konstantin" References: <1427302838-8285-1-git-send-email-olivier.matz@6wind.com> <1427385595-15011-1-git-send-email-olivier.matz@6wind.com> <1427385595-15011-2-git-send-email-olivier.matz@6wind.com> <2601191342CEEE43887BDE71AB97725821407D4D@irsmsx105.ger.corp.intel.com> <55151DDE.8040301@6wind.com> <55156188.6040101@6wind.com> <2601191342CEEE43887BDE71AB9772582140814C@irsmsx105.ger.corp.intel.com> <55157477.7090207@6wind.com> <2601191342CEEE43887BDE71AB9772582140FB21@irsmsx105.ger.corp.intel.com> <5519AA46.1090409@6wind.com> <2601191342CEEE43887BDE71AB97725821412EE9@irsmsx105.ger.corp.intel.com> <551AEF08.8020009@6wind.com> <2601191342CEEE43887BDE71AB9772582141368A@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB9772582141368A@irsmsx105.ger.corp.intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v2 1/5] mbuf: fix clone support when application uses private mbuf data 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: Wed, 01 Apr 2015 15:18:38 -0000 Hi, On 04/01/2015 03:48 PM, Ananyev, Konstantin wrote: >>>>>> With this solution, there are 2 options: >>>>>> - no mempool modification, so each application/driver has to add >>>>>> priv_size bytes to the object to get the mbuf pointer. This does not >>>>>> look feasible. >>>>>> - change the mempool to support private area before each object. I >>>>>> think mempool API is already quite complex, and I think it's not >>>>>> the role of the mempool library to provide such features. >>>>> >>>>> >>>>> In fact, I think the changes would be minimal. >>>>> All we have to do, is to make changes in rte_pktmbuf_init(): >>>>> >>>>> void >>>>> rte_pktmbuf_init(struct rte_mempool *mp, >>>>> __attribute__((unused)) void *opaque_arg, >>>>> void *_m, >>>>> __attribute__((unused)) unsigned i) >>>>> { >>>>> >>>>> //extract priv_size from mempool (discussed above). >>>>> uint16_t priv_size = rte_mbufpool_get_priv_size(mp); >>>>> >>>>> struct rte_mbuf *m = _m + priv_size; >>>>> uint32_t buf_len = mp->elt_size - sizeof(struct rte_mbuf) - priv_size; >>>>> >>>>> .... >>>>> >>>>> >>>>> With that way priv_size inside mbuf would always be the size its own private space, >>>>> for both direct and indirect mbus., so we don't require to set priv_size at attach/detach. >>>>> Again RTE_MBUF_TO_BADDR() and RTE_MBUF_FROM_BADDR() would just work without any modifications. >>>> >>>> I'm not sure I'm getting it. The argument '_m' of your >>>> rte_pktmbuf_init() is the pointer to the priv data, right? >>>> So it means that the mbuf is located priv_size bytes after. >>>> >>>> The rte_pktmbuf_init() function is called by mempool_create(), >>>> and the _m parameter is a pointer to a mempool object. So >>>> in my understanding, mempool_get() would not return a rte_mbuf >>>> but a pointer to the application private data. >>> >>> Ah my bad, forgot that mempool's obj_init() returns void now :( >>> To make this approach work also need to change it, so it return a pointer to the new object. >>> So, rte_pktmbuf_init() would return m and then in mempool_add_elem(): >>> >>> if (obj_init) >>> obj = obj_init(mp, obj_init_arg, obj, obj_idx); >>> >>> rte_ring_sp_enqueue(mp->ring, obj); >> >> Yes, but modififying mempool is what I wanted to avoid for several >> reasons. First, I think that changing the mempool_create() API here >> (actually the obj_init prototype) would impact existing applications. >> >> Also, I'm afraid that storing a different address than the start >> address of the object would have additional impacts. For instance, >> some data is supposed to be stored before the object, see >> __mempool_from_obj() or mempool_obj_audit(). > > mempool_obj_audit() should be ok, I think, but yes - > rte_mempool_from_obj() would change the behaviour and > can't be used by mempool_obj_audit() anymore. > > Ok, I am convinced - let's stick with private space between rte_mbuf and buf_adddr, as you suggested. > >> >> Finally, I believe that mempool is not the proper place to do >> modifications that are only needed for mbufs. If we really want >> to implement mbuf private data in that way, maybe it would be >> better to add a new layer above mempool (a mbuf pool layer). > > Not that I am against it, but seems like even more massive change - > every application would need to be changed to use rte_mbufpool_create(), no? Yes, indeed that would be a significant change for the applications, which is probably not what we want, except if we can keep backward compatibility. Maybe it's possible. That's something to keep in mind if I send a patch series that introduce a new rte_mbuf_pool_create() function. Regards, Olivier