[dpdk-dev] [PATCH v8 1/5] mempool: populate mempool with page sized chunks of memory

[Vamsi Krishna Attunuru]

Hi,

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko at solarflare.com>
> Sent: Wednesday, July 24, 2019 12:58 PM
> To: Vamsi Krishna Attunuru <vattunuru at marvell.com>; dev at dpdk.org
> Cc: thomas at monjalon.net; Jerin Jacob Kollanukkaran <jerinj at marvell.com>;
> olivier.matz at 6wind.com; ferruh.yigit at intel.com; anatoly.burakov at intel.com;
> Kiran Kumar Kokkilagadda <kirankumark at marvell.com>
> Subject: Re: [dpdk-dev] [PATCH v8 1/5] mempool: populate mempool with page
> sized chunks of memory
> 
> On 7/24/19 10:09 AM, Vamsi Krishna Attunuru wrote:
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <arybchenko at solarflare.com>
> >> Sent: Wednesday, July 24, 2019 1:04 AM
> >> To: Vamsi Krishna Attunuru <vattunuru at marvell.com>; dev at dpdk.org
> >> Cc: thomas at monjalon.net; Jerin Jacob Kollanukkaran
> >> <jerinj at marvell.com>; olivier.matz at 6wind.com; ferruh.yigit at intel.com;
> >> anatoly.burakov at intel.com; Kiran Kumar Kokkilagadda
> >> <kirankumark at marvell.com>
> >> Subject: Re: [dpdk-dev] [PATCH v8 1/5] mempool: populate mempool with
> >> page sized chunks of memory
> >>
> >> On 7/23/19 3:28 PM, Vamsi Krishna Attunuru wrote:
> >>>> -----Original Message-----
> >>>> From: Andrew Rybchenko <arybchenko at solarflare.com>
> >>>> Sent: Tuesday, July 23, 2019 4:38 PM
> >>>> To: Vamsi Krishna Attunuru <vattunuru at marvell.com>; dev at dpdk.org
> >>>> Cc: thomas at monjalon.net; Jerin Jacob Kollanukkaran
> >>>> <jerinj at marvell.com>; olivier.matz at 6wind.com;
> >>>> ferruh.yigit at intel.com; anatoly.burakov at intel.com; Kiran Kumar
> >>>> Kokkilagadda <kirankumark at marvell.com>
> >>>> Subject: Re: [dpdk-dev] [PATCH v8 1/5] mempool: populate mempool
> >>>> with page sized chunks of memory
> >>>>
> >>>> On 7/23/19 8:38 AM, vattunuru at marvell.com wrote:
> >>>>> From: Vamsi Attunuru <vattunuru at marvell.com>
> >>>>>
> >>>>> Patch adds a routine to populate mempool from page aligned and
> >>>>> page sized chunks of memory to ensures memory objs do not fall
> >>>>> across the page boundaries. It's useful for applications that
> >>>>> require physically contiguous mbuf memory while running in IOVA=VA
> mode.
> >>>>>
> >>>>> Signed-off-by: Vamsi Attunuru <vattunuru at marvell.com>
> >>>>> Signed-off-by: Kiran Kumar K <kirankumark at marvell.com>
> >>>>> ---
> >> <...>
> >>
> >>>>> +	int ret;
> >>>>> +
> >>>>> +	ret = mempool_ops_alloc_once(mp);
> >>>>> +	if (ret != 0)
> >>>>> +		return ret;
> >>>>> +
> >>>>> +	if (mp->nb_mem_chunks != 0)
> >>>>> +		return -EEXIST;
> >>>>> +
> >>>>> +	pg_sz = get_min_page_size(mp->socket_id);
> >>>>> +	pg_shift = rte_bsf32(pg_sz);
> >>>>> +
> >>>>> +	for (mz_id = 0, n = mp->size; n > 0; mz_id++, n -= ret) {
> >>>>> +
> >>>>> +		rte_mempool_op_calc_mem_size_default(mp, n, pg_shift,
> >>>>> +			     &chunk_size, &align);
> >>>> It is incorrect to ignore mempool pool ops and enforce default
> >>>> handler. Use rte_mempool_ops_calc_mem_size().
> >>>> Also it is better to treat negative return value as an error as
> >>>> default function does.
> >>>> (May be it my mistake in return value description that it is not mentioned).
> >>>>
> >>> Yes, I thought so, but ops_calc_mem_size() would in turn call
> >>> mempool pmd's calc_mem_size() op which may/may not return required
> >>> chunk_size and align values in this case. Or else it would be
> >>> skipped completely and use
> >> pg_sz for both memzone len and align, anyways this  page sized
> >> alignment will suits the pmd's specific align requirements.
> >>
> >> Anyway it is incorrect to violate driver ops. default is definitely
> >> wrong for bucket.
> >> min_chunk_size and align is mempool driver requirements. You can
> >> harden it, but should not violate it.
> > fine, I will modify the routine as below,  call pmd's calc_mem_size() op and
> over write min_chunk_size if it does not suit for this function's purpose.
> >
> > +       total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size;
> > +       if (total_elt_sz > pg_sz)
> > +               return ret;
> >
> > +       for (mz_id = 0, n = mp->size; n > 0; mz_id++, n -= ret) {
> >
> > -               rte_mempool_op_calc_mem_size_default(mp, n, pg_shift,
> > -                            &chunk_size, &align);
> > +               ret = rte_mempool_ops_calc_mem_size(mp, n, pg_shift,
> > +                               &min_chunk_size, &align);
> > +
> > +               if (ret < 0)
> >                          goto fail;
> >
> > +               if (min_chunk_size > pg_sz)
> > +                       min_chunk_size = pg_sz;
> >
> > Changes look fine.?
> 
> No, you can't violate min_chunk_size requirement. If it is unacceptable, error
> should be returned. So, you should not check total_elt_sz above separately.

Fine, will put min_chunk_size req check and return error if it's inappropriate.
I will send V9 with addressing all other comments.
Btw, sorry for delayed response.