[dpdk-dev] [PATCH v1 1/9] mempool: add op to calculate memory size to be allocated

[Andrew Rybchenko]

On 03/11/2018 03:51 PM, santosh wrote:
> Hi Andrew,
>
>
> On Saturday 10 March 2018 09:09 PM, Andrew Rybchenko wrote:
>> Size of memory chunk required to populate mempool objects depends
>> on how objects are stored in the memory. Different mempool drivers
>> may have different requirements and a new operation allows to
>> calculate memory size in accordance with driver requirements and
>> advertise requirements on minimum memory chunk size and alignment
>> in a generic way.
>>
>> Bump ABI version since the patch breaks it.
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko at solarflare.com>
>> ---
>> RFCv2 -> v1:
>>   - move default calc_mem_size callback to rte_mempool_ops_default.c
>>   - add ABI changes to release notes
>>   - name default callback consistently: rte_mempool_op_<callback>_default()
>>   - bump ABI version since it is the first patch which breaks ABI
>>   - describe default callback behaviour in details
>>   - avoid introduction of internal function to cope with depration
>>     (keep it to deprecation patch)
>>   - move cache-line or page boundary chunk alignment to default callback
>>   - highlight that min_chunk_size and align parameters are output only
>>
> [...]
>
>> diff --git a/lib/librte_mempool/rte_mempool_ops_default.c b/lib/librte_mempool/rte_mempool_ops_default.c
>> new file mode 100644
>> index 0000000..57fe79b
>> --- /dev/null
>> +++ b/lib/librte_mempool/rte_mempool_ops_default.c
>> @@ -0,0 +1,38 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause
>> + * Copyright(c) 2016 Intel Corporation.
>> + * Copyright(c) 2016 6WIND S.A.
>> + * Copyright(c) 2018 Solarflare Communications Inc.
>> + */
>> +
>> +#include <rte_mempool.h>
>> +
>> +ssize_t
>> +rte_mempool_op_calc_mem_size_default(const struct rte_mempool *mp,
>> +				     uint32_t obj_num, uint32_t pg_shift,
>> +				     size_t *min_chunk_size, size_t *align)
>> +{
>> +	unsigned int mp_flags;
>> +	int ret;
>> +	size_t total_elt_sz;
>> +	size_t mem_size;
>> +
>> +	/* Get mempool capabilities */
>> +	mp_flags = 0;
>> +	ret = rte_mempool_ops_get_capabilities(mp, &mp_flags);
>> +	if ((ret < 0) && (ret != -ENOTSUP))
>> +		return ret;
>> +
>> +	total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size;
>> +
>> +	mem_size = rte_mempool_xmem_size(obj_num, total_elt_sz, pg_shift,
>> +					 mp->flags | mp_flags);
>> +
> Looks ok to me except a nit:
> (mp->flags | mp_flags) style expression is to differentiate that
> mp_flags holds driver specific flag like BLK_ALIGN and mp->flags
> has appl specific flags.. is it so? If not then why not simply
> do like:
> mp->flags |= mp_flags.

In fact it does not mater a lot since the code is removed in the patch 3.
Here it is required just for consistency. Also, mp argument is a const
which will not allow to change its members.