On Tue, Dec 19, 2017 at 04:16:33PM +0530, Hemant Agrawal wrote:
> Hi Olivier,
>
> On 12/19/2017 3:54 PM, Olivier MATZ wrote:
> > Hi Hemant,
> >
> > On Wed, Dec 06, 2017 at 06:01:12PM +0530, Hemant Agrawal wrote:
> > > This is required for the optimizations w.r.t hw mempools.
> > > They will use different kind of optimizations if the buffers
> > > are from single contiguous memzone.
> > >
> > > Signed-off-by: Hemant Agrawal <hemant.agra...@nxp.com>
> > > ---
> > > lib/librte_mempool/rte_mempool.c | 7 +++++--
> > > lib/librte_mempool/rte_mempool.h | 5 +++++
> > > 2 files changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/librte_mempool/rte_mempool.c
> > > b/lib/librte_mempool/rte_mempool.c
> > > index d50dba4..9d3737c 100644
> > > --- a/lib/librte_mempool/rte_mempool.c
> > > +++ b/lib/librte_mempool/rte_mempool.c
> > > @@ -387,13 +387,16 @@ rte_mempool_populate_iova(struct rte_mempool *mp,
> > > char *vaddr,
> > > total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size;
> > >
> > > /* Detect pool area has sufficient space for elements */
> > > - if (mp->flags & MEMPOOL_F_CAPA_PHYS_CONTIG) {
> > > - if (len < total_elt_sz * mp->size) {
> > > + if (len < total_elt_sz * mp->size) {
> > > + if (mp->flags & MEMPOOL_F_CAPA_PHYS_CONTIG) {
> > > RTE_LOG(ERR, MEMPOOL,
> > > "pool area %" PRIx64 " not enough\n",
> > > (uint64_t)len);
> > > return -ENOSPC;
> > > }
> > > + } else {
> > > + /* Memory will be allocated from multiple memzones */
> > > + mp->flags |= MEMPOOL_F_MULTI_MEMZONE;
> > > }
> > >
> > > memhdr = rte_zmalloc("MEMPOOL_MEMHDR", sizeof(*memhdr), 0);
> > > diff --git a/lib/librte_mempool/rte_mempool.h
> > > b/lib/librte_mempool/rte_mempool.h
> > > index 721227f..394a4fe 100644
> > > --- a/lib/librte_mempool/rte_mempool.h
> > > +++ b/lib/librte_mempool/rte_mempool.h
> > > @@ -292,6 +292,11 @@ struct rte_mempool {
> > > */
> > > #define MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS 0x0080
> > >
> > > +/* Indicates that the mempool buffers are allocated from multiple
> > > memzones
> > > + * the buffer may or may not be physically contiguous.
> > > + */
> > > +#define MEMPOOL_F_MULTI_MEMZONE 0x0100
> > > +
> > > /**
> > > * @internal When debug is enabled, store some statistics.
> > > *
> > > --
> > > 2.7.4
> > >
> >
> > I'm not confortable with adding more and more flags, as I explained
> > here: http://dpdk.org/ml/archives/dev/2017-December/083909.html
>
> This particular flag is not about how to populate mempool. This is just
> indicating how the mempool was populated - a status flag. This information
> is just helpful for the PMDs.
>
> At least I am not able to see that this particular flag is being very driver
> specific.
That's true, I commented too fast :)
And what about using mp->nb_mem_chunks instead? Would it do the job
in your use-case?
> > It makes the generic code very complex, and probably buggy (many
> > flags are incompatible with other flags).
> >
> > I'm thinking about moving the populate_* functions in the drivers
> > (this is described a bit more in the link above). What do you think
> > about this approach?
> >
>
> The idea is good and it will give fine control to the individual mempools to
> populate the memory the way they want. However, on the downside, it will
> also lead to lot of duplicate code or similar code. It may also lead to a
> maintenance issue for the mempool PMD owner.
Yes, that will be the drawback. If we do this, we should try to keep some
common helpers in the mempool lib.
