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.