From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from proxy.6wind.com (host.76.145.23.62.rev.coltfrance.com [62.23.145.76]) by dpdk.org (Postfix) with ESMTP id 494911518 for ; Thu, 28 Jul 2016 10:32:46 +0200 (CEST) Received: from [10.16.0.195] (unknown [10.16.0.195]) by proxy.6wind.com (Postfix) with ESMTP id 060CD246DC; Thu, 28 Jul 2016 10:32:45 +0200 (CEST) From: Olivier MATZ To: Jerin Jacob , Hemant Agrawal References: <20160727095128.GA11679@localhost.localdomain> Cc: David Hunt , "dev@dpdk.org" , Thomas Monjalon , "viktorin@rehivetech.com" , Shreyansh Jain Message-ID: <5799C32C.3050907@6wind.com> Date: Thu, 28 Jul 2016 10:32:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160727095128.GA11679@localhost.localdomain> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] usages issue with external mempool 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: Thu, 28 Jul 2016 08:32:46 -0000 Hi Hemant, Jerin, On 07/27/2016 11:51 AM, Jerin Jacob wrote: > On Tue, Jul 26, 2016 at 10:11:13AM +0000, Hemant Agrawal wrote: >> Hi, >> There was lengthy discussions w.r.t external mempool patches. However, I am still finding usages issue with the agreed approach. >> >> The existing API to create packet mempool, "rte_pktmbuf_pool_create" does not provide the option to change the object init iterator. This may be the reason that many applications (e.g. OVS) are using rte_mempool_create to create packet mempool with their own object iterator (e.g. ovs_rte_pktmbuf_init). >> >> e.g the existing usages are: >> dmp->mp = rte_mempool_create(mp_name, mp_size, MBUF_SIZE(mtu), >> MP_CACHE_SZ, >> sizeof(struct rte_pktmbuf_pool_private), >> rte_pktmbuf_pool_init, NULL, >> ovs_rte_pktmbuf_init, NULL, >> socket_id, 0); >> >> >> With the new API set for packet pool create, this need to be changed to: >> >> dmp->mp = rte_mempool_create_empty(mp_name, mp_size, MBUF_SIZE(mtu), >> MP_CACHE_SZ, >> sizeof(struct rte_pktmbuf_pool_private), >> socket_id, 0); >> if (dmp->mp == NULL) >> break; >> >> rte_errno = rte_mempool_set_ops_byname(dmp-mp, >> RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL); >> if (rte_errno != 0) { >> RTE_LOG(ERR, MBUF, "error setting mempool handler\n"); >> return NULL; >> } >> rte_pktmbuf_pool_init(dmp->mp, NULL); >> >> ret = rte_mempool_populate_default(dmp->mp); >> if (ret < 0) { >> rte_mempool_free(dmp->mp); >> rte_errno = -ret; >> return NULL; >> } >> >> rte_mempool_obj_iter(dmp->mp, ovs_rte_pktmbuf_init, NULL); >> >> This is not a user friendly approach to ask for changing 1 API to 6 new APIs. Or, am I missing something? The example you are giving first still works today, right? Since the mempool rework, as the objects are linked together in the mempool, it is also possible to use rte_pktmbuf_pool_create() and call another iterator after, like below: mp = rte_pktmbuf_pool_create(name, size, cache_size, priv_size, data_room_size, socket_id); if (mp == NULL) handle_error(); rte_mempool_obj_iter(mp, ovs_rte_pktmbuf_init); By the way, rte_mempool_set_ops_byname() is not needed in your example above since it sets the default ops. > > I agree, To me, this is very bad. I have raised this concern earlier > also > > Since applications like OVS goes through "rte_mempool_create" for > even packet buffer pool creation. IMO it make senses to extend > "rte_mempool_create" to take one more argument to provide external pool > handler name(NULL for default). I don't see any valid technical reason > to treat external pool handler based mempool creation API different > from default handler. I disagree that changing from one function do_many_stuff(11 args) to several do_one_stuff(few args) functions is a regression. I don't feel that having a new function with 12 args solves anything. What is the problem of having 20 lines of code for initializing a mbuf pool? The new API gives more flexibility, and it allow an application to define its own function if the default one cannot be used. I think that the name of the functions pretty well defines what they do: rte_mempool_create_empty(): create an empty mempool rte_mempool_set_ops_byname(): set the mempool handler from its name rte_pktmbuf_pool_init(): initialize the mempool as a packet pool rte_mempool_populate_default(): populate the pool with objects rte_mempool_obj_iter(): call a function for each object >> I think, we should do one of the following: >> >> 1. Enhance "rte_pktmbuf_pool_create" to optionally accept "rte_mempool_obj_cb_t *obj_init, void *obj_init_arg" as inputs. If obj_init is not present, default can be used. This function was introduced to simplify the creation of mbuf pools compared to mempool_create(). As I said above, you can still call rte_mempool_obj_iter() after. >> 2. Create a new wrapper API (e.g. e_pktmbuf_pool_create_new) with the above said behavior e.g.: >> /* helper to create a mbuf pool */ >> struct rte_mempool * >> rte_pktmbuf_pool_create_new(const char *name, unsigned n, >> unsigned cache_size, uint16_t priv_size, uint16_t data_room_size, >> rte_mempool_obj_cb_t *obj_init, void *obj_init_arg, >> int socket_id) Same comment here. >> 3. Let the existing rte_mempool_create accept flag as "MEMPOOL_F_HW_PKT_POOL". Obviously, if this flag is set - all other flag values should be ignored. This was discussed earlier also. You say we should do one of these points. But what is the link with the point 1/ and 2/ ? You say we should add a flag which: - (obviously) will make all other flag values be ignored - (probably also obviously) will prevent to use rte_mempool_set_ops_byname() later So to conclude, as I understand, your issue is having 20 lines of code to initialize a mbuf pool, and you would prefer to have one function with all possible parameters, is that correct? If that's the case, sorry but I feel it's clearer to have shorter functions. Regards, Olivier