Re: [dpdk-dev] [EXT] [PATCH 5/5] mempool: prevent objects from being across pages

[Jerin Jacob <jerinjacobk@gmail.com>]

On Wed, Oct 30, 2019 at 1:16 PM Andrew Rybchenko
<arybchenko@solarflare.com> wrote:
>

> >>   int
> >>   rte_mempool_op_populate_default(struct rte_mempool *mp, unsigned int
> >> max_objs,
> >>              void *vaddr, rte_iova_t iova, size_t len,
> >>              rte_mempool_populate_obj_cb_t *obj_cb, void *obj_cb_arg)
> >> {
> >> -    size_t total_elt_sz;
> >> +    char *va = vaddr;
> >> +    size_t total_elt_sz, pg_sz;
> >>      size_t off;
> >>      unsigned int i;
> >>      void *obj;
> >>
> >> +    rte_mempool_get_page_size(mp, &pg_sz);
> >> +
> >>      total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size;
> >>
> >> -    for (off = 0, i = 0; off + total_elt_sz <= len && i < max_objs; i++) {
> >> +    for (off = 0, i = 0; i < max_objs; i++) {
> >> +            /* align offset to next page start if required */
> >> +            if (check_obj_bounds(va + off, pg_sz, total_elt_sz) < 0)
> >> +                    off += RTE_PTR_ALIGN_CEIL(va + off, pg_sz) - (va +
> >> off);
> > Moving offset to the start of next page and than freeing (vaddr + off + header_size) to pool, this scheme is not aligning with octeontx2 mempool's buf alignment requirement(buffer address needs to be multiple of buffer size).
>
> It sounds line octeontx2 mempool should have its own populate callback
> which cares about it.

Driver specific populate function is not a bad idea. The only concern
would be to

# We need to duplicate rte_mempool_op_populate_default() and
rte_mempool_op_calc_mem_size_default()
# We need to make sure if some one changes the
rte_mempool_op_populate_default() and
rte_mempool_op_calc_mem_size_default() then he/she needs to update the
drivers too

# I would like to add one more point here is that:
- Calculation of  object pad requirements for MEMPOOL_F_NO_SPREAD i.e
optimize_object_size()
is NOT GENERIC. i.e get_gcd() based logic is not generic. DDR
controller defines the address to DDR channel "spread" and it will be
based on SoC or Mirco architecture.
So we need to consider that as well.