[prev in list] [next in list] [prev in thread] [next in thread] 

List:       dpdk-dev
Subject:    Re: [dpdk-dev] usages issue with external mempool
From:       Hemant Agrawal <hemant.agrawal () nxp ! com>
Date:       2016-07-29 10:09:24
Message-ID: DB5PR04MB1605E322CCE2D8687D98947C89010 () DB5PR04MB1605 ! eurprd04 ! prod ! outlook ! com
[Download RAW message or body]

Hi Oliver

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Thursday, July 28, 2016 2:03 PM
> 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?

[Hemant] No. The rte_mempool_create, may not work with offloaded mempool.  In the \
current code, the default option in rte_mempool_create is hardcoded as "ring_mp_mc". \
So it will not use any other config file specified mempool. One possible solution, I \
see that the default option in rte_mempool_create can be changed to  \
"CONFIG_RTE_MBUF_DEFAULT_MEMPOOL_OPS"  from "ring_mp_mc".

> 
> 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);
> 
[Hemant] Why? The purpose of rte_pktmbuf_pool_create was to provide a usable wrapper. \
If applications are not able to use it, we should retrospect and upgrade it as per \
common usages. your ease-of-usages definition may differ from my definition. 

> By the way, rte_mempool_set_ops_byname() is not needed in your example
> above since it sets the default ops.

[Hemant] I think it is needed. I could not find a MEMPOOL_REGISTER_OPS for "default" \
value. Default may not be ""ring_mp_mc".

> 
> > 
> > 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.
> 
[Hemant]  see comment above. 

> > > 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.

[Hemant]  The new shorter functions are good.  However, providing a usable wrapper \
will help. 

[Hemant] Also,  rte_mempool_create should have been deprecated or it should be \
enhanced to work with external mempool. 12th argument or flag, these are just \
different way to make it work.  The sad story is that it does not work today with \
external mempool and it is not deprecated. 

Regards,
Hemant


[prev in list] [next in list] [prev in thread] [next in thread]