Re: [dpdk-dev] [PATCH v5 8/8] mempool: update range info to pool

[Olivier MATZ <olivier.matz@6wind.com>]

On Thu, Sep 07, 2017 at 02:26:49PM +0530, santosh wrote:
> 
> On Thursday 07 September 2017 02:00 PM, Olivier MATZ wrote:
> > On Wed, Sep 06, 2017 at 04:58:34PM +0530, Santosh Shukla wrote:
> >> HW pool manager e.g. Octeontx SoC demands s/w to program start and end
> >> address of pool. Currently, there is no such handle in external mempool.
> >> Introducing rte_mempool_update_range handle which will let HW(pool
> >> manager) to know when common layer selects hugepage:
> >> For each hugepage - update its start/end address to HW pool manager.
> >>
> >> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> >> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >> ---
> >>  lib/librte_mempool/rte_mempool.c           |  3 +++
> >>  lib/librte_mempool/rte_mempool.h           | 22 ++++++++++++++++++++++
> >>  lib/librte_mempool/rte_mempool_ops.c       | 13 +++++++++++++
> >>  lib/librte_mempool/rte_mempool_version.map |  1 +
> >>  4 files changed, 39 insertions(+)
> >>
> >> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> >> index 38dab1067..65f17a7a7 100644
> >> --- a/lib/librte_mempool/rte_mempool.c
> >> +++ b/lib/librte_mempool/rte_mempool.c
> >> @@ -363,6 +363,9 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char *vaddr,
> >>  	struct rte_mempool_memhdr *memhdr;
> >>  	int ret;
> >>  
> >> +	/* update range info to mempool */
> >> +	rte_mempool_ops_update_range(mp, vaddr, paddr, len);
> >> +
> >
> > My understanding is that the 2 capability flags imply that the mempool
> > is composed of only one memory area (rte_mempool_memhdr). Do you confirm?
> 
> yes.
> 
> > So in your case, you will be notified only once with the full range of
> > the mempool. But if there are several memory areas, the function will
> > be called each time.
> >
> > So I suggest to rename rte_mempool_ops_update_range() in
> > rte_mempool_ops_register_memory_area(), which goal is to notify the mempool
> > handler each time a new memory area is added.
> >
> > This should be properly explained in the API comments.
> >
> > I think this handler can return an error code (0 on success, negative on
> > error). On error, rte_mempool_populate_phys() should fail.
> >
> Will rename to rte_mempool_ops_register_memory_area() and
> change return type from void to int.
> 
> 
> return description:
> 0 : for success
> <0 : failure, such that
>  - if handler returns -ENOTSUP then valid error case--> no error handling at mempool layer
>  - Otherwise  rte_mempool_populate_phys () fails.
> 
> Are you okay with error return? pl. confirm. 

yes.
Just take care of doubled spaces and avoid "-->".