Hi,
> -----Original Message-----
> From: Andrew Rybchenko <arybche...@solarflare.com>
> Sent: Wednesday, July 24, 2019 12:58 PM
> To: Vamsi Krishna Attunuru <vattun...@marvell.com>; dev@dpdk.org
> Cc: tho...@monjalon.net; Jerin Jacob Kollanukkaran <jer...@marvell.com>;
> olivier.m...@6wind.com; ferruh.yi...@intel.com; anatoly.bura...@intel.com;
> Kiran Kumar Kokkilagadda <kirankum...@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 <arybche...@solarflare.com>
> >> Sent: Wednesday, July 24, 2019 1:04 AM
> >> To: Vamsi Krishna Attunuru <vattun...@marvell.com>; dev@dpdk.org
> >> Cc: tho...@monjalon.net; Jerin Jacob Kollanukkaran
> >> <jer...@marvell.com>; olivier.m...@6wind.com; ferruh.yi...@intel.com;
> >> anatoly.bura...@intel.com; Kiran Kumar Kokkilagadda
> >> <kirankum...@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 <arybche...@solarflare.com>
> >>>> Sent: Tuesday, July 23, 2019 4:38 PM
> >>>> To: Vamsi Krishna Attunuru <vattun...@marvell.com>; dev@dpdk.org
> >>>> Cc: tho...@monjalon.net; Jerin Jacob Kollanukkaran
> >>>> <jer...@marvell.com>; olivier.m...@6wind.com;
> >>>> ferruh.yi...@intel.com; anatoly.bura...@intel.com; Kiran Kumar
> >>>> Kokkilagadda <kirankum...@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, vattun...@marvell.com wrote:
> >>>>> From: Vamsi Attunuru <vattun...@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 <vattun...@marvell.com>
> >>>>> Signed-off-by: Kiran Kumar K <kirankum...@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.
