Re: [PATCH v1] mempool/dpaa2: add DPAA2 hardware offloaded mempool

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

Hi Hemant,

On Fri, 24 Mar 2017 17:42:46 +0100, Olivier Matz <olivier.matz@6wind.com> wrote:
> > > From high level, I'm still a little puzzled by the amount of references
> > > to mbuf in a mempool handler code, which should theorically handle any
> > > kind of objects.
> > > 
> > > Is it planned to support other kind of objects?
> > > Does this driver passes the mempool autotest?
> > > Can the user be aware of these limitations?

Some more comments.

I think the mempool model as it is today in DPDK does not match your
driver model.

For instance, the fact that the hardware is able return the mbuf in the
pool by itself makes me think that the mbuf rework patchset [1] can break
your driver. Especially this patch [2], that expects that m->refcnt=1,
m->nb_segs=1 and m->next=NULL when allocating from a pool.

- Can this handler can be used with another driver?
- Can your driver be used with another mempool handler?
- Is the dpaa driver the only driver that would take advantage of
  the mempool handler? Will it work with cloned mbufs?


Defining a flag like this in your private code should not be done:

   #define MEMPOOL_F_HW_PKT_POOL (1 << ((sizeof(int) * 8) - 1))

Nothing prevents to break it if someone touches the generic flags in
mempool. And hope that no other driver does the same :)

Maybe you can do the same without flag, for instance by checking if
(m->pool == pmd->pool)?



I think a new mempool handler should pass the mempool tests, or at least
we should add a new API that would describe the capabilities or something
like that (for instance: support mbuf pool only, support multiprocess).


To conclude, I'm quite reserved.
Well, all the code is in driver/, meaning it does not pollute the rest.


Regards,
Olivier

[1] http://dpdk.org/ml/archives/dev/2017-March/059693.html
[2] http://dpdk.org/dev/patchwork/patch/21602/