Re: [dpdk-dev] [PATCH v10 1/3] mempool: support external mempool operations

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2016-06-14 14:20, Hunt, David:
> 
> Hi Thomas,
> 
> On 14/6/2016 1:55 PM, Thomas Monjalon wrote:
> > Hi David,
> >
> > 2016-06-14 10:46, David Hunt:
> >> Until now, the objects stored in a mempool were internally stored in a
> >> ring. This patch introduces the possibility to register external handlers
> >> replacing the ring.
> >>
> >> The default behavior remains unchanged, but calling the new function
> >> rte_mempool_set_handler() right after rte_mempool_create_empty() allows
> >> the user to change the handler that will be used when populating
> >> the mempool.
> >>
> >> This patch also adds a set of default ops (function callbacks) based
> >> on rte_ring.
> >>
> >> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> >> Signed-off-by: David Hunt <david.hunt@intel.com>
> > Glad to see we are close to have this feature integrated.
> >
> > I've just looked into few details before pushing.
> > One of them are the comments. In mempool they were all ended by a dot.
> > Please check the new comments.
> 
> Do you mean the rte_mempool struct definition, or all comments? Shall I 
> leave the
> old comments the way they were before the change, or will I clean up?
> If I clean up, I'd suggest I add a separate patch for that.

Just check and clean the comments added in this patch.

> > The doc/guides/rel_notes/deprecation.rst must be updated to remove
> > the deprecation notice in this patch.
> 
> Will do. As a separate patch in the set?

In this patch.

> > Isn't there some explanations to add in
> > doc/guides/prog_guide/mempool_lib.rst?
> 
> Yes, I'll adapt some of the cover letter, and add as a separate patch.

It is OK (and better) to add it in this patch.
Maybe you can request John's help for doc review.

> > Isn't there a better name than "default" for the default implementation?
> > I don't think the filename rte_mempool_default.c is meaningful.
> 
> I could call it rte_mempool_ring.c? Since the default handler is ring based?

It is an idea.

Thanks