Re: [PATCH v12 0/3] mempool: add external mempool manager

[Shreyansh Jain <shreyansh.jain@nxp.com>]

Though I am late to the discussion...

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, June 15, 2016 10:10 PM
> To: Hunt, David <david.hunt@intel.com>; Jan Viktorin
> <viktorin@rehivetech.com>
> Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com; Shreyansh Jain
> <shreyansh.jain@nxp.com>
> Subject: Re: [PATCH v12 0/3] mempool: add external mempool manager
> 
> 
> 
> On 06/15/2016 06:34 PM, Hunt, David wrote:
> >
> >
> > On 15/6/2016 1:03 PM, Olivier MATZ wrote:
> >> [...]
> >>
> >> The opaque pointer would be saved in mempool structure, and used
> >> when the mempool is populated (calling mempool_ops_alloc).
> >> The type of the structure pointed by the opaque has to be defined
> >> (and documented) into each mempool_ops manager.
> >>
> >>
> >> Olivier
> >
> >
> > OK, just to be sure before I post another patchset.....
> >
> > For the rte_mempool_struct:
> >          struct rte_mempool_memhdr_list mem_list; /**< List of memory
> > chunks */
> > +       void *ops_args;                  /**< optional args for ops
> > alloc. */
> >
> > (at the end of the struct, as it's just on the control path, not to
> > affect fast path)
> 
> Hmm, I would put it just after pool_data.

+1
And, would 'pool_config' (picked from a previous email from David) a better name?

>From a user perspective, the application is passing a configuration item to the pool to work one. Only the application and mempool allocator understand it (opaque).
As for 'ops_arg', it would be to control 'assignment-of-operations' to the framework.

Maybe just my point of view.

> 
> 
> >
> > Then change function params:
> >   int
> > -rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name);
> > +rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
> > +               void *ops_args);
> >
> > And (almost) finally in the rte_mempool_set_ops_byname function:
> >          mp->ops_index = i;
> > +       mp->ops_args = ops_args;
> >          return 0;
> >
> > Then (actually) finally, add a null to all the calls to
> > rte_mempool_set_ops_byname.
> >
> > OK? :)
> >
> 
> Else, looks good to me! Thanks David.

Me too. Though I would like to clarify something for my understanding:

Mempool->pool_data => Used by allocator to store private data
Mempool->pool_config => (or ops_arg) used by allocator to access user/app provided value.

Is that correct?

-
Shreyansh