[dpdk-dev] [PATCH v8 1/3] mempool: support external mempool operations
Hunt, David
david.hunt at intel.com
Thu Jun 9 12:33:33 CEST 2016
Hi Olivier,
On 8/6/2016 1:13 PM, Olivier Matz wrote:
> Hi David,
>
> Please find some comments below.
>
> On 06/03/2016 04:58 PM, David Hunt wrote:
>
>> --- a/lib/librte_mempool/rte_mempool.h
>> +++ b/lib/librte_mempool/rte_mempool.h
>> +/**
>> + * Prototype for implementation specific data provisioning function.
>> + *
>> + * The function should provide the implementation specific memory for
>> + * for use by the other mempool ops functions in a given mempool ops struct.
>> + * E.g. the default ops provides an instance of the rte_ring for this purpose.
>> + * it will mostlikely point to a different type of data structure, and
>> + * will be transparent to the application programmer.
>> + */
>> +typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp);
> In http://dpdk.org/ml/archives/dev/2016-June/040233.html, I suggested
> to change the prototype to return an int (-errno) and directly set
> the set mp->pool_data (void *) or mp->pool_if (uint64_t). No cast
> would be required in this latter case.
Done.
> By the way, there is a typo in the comment:
> "mostlikely" -> "most likely"
Fixed.
>> --- /dev/null
>> +++ b/lib/librte_mempool/rte_mempool_default.c
>> +static void
>> +common_ring_free(struct rte_mempool *mp)
>> +{
>> + rte_ring_free((struct rte_ring *)mp->pool_data);
>> +}
> I don't think the cast is needed here.
> (same in other functions)
Removed.
>
>> --- /dev/null
>> +++ b/lib/librte_mempool/rte_mempool_ops.c
>> +/* add a new ops struct in rte_mempool_ops_table, return its index */
>> +int
>> +rte_mempool_ops_register(const struct rte_mempool_ops *h)
>> +{
>> + struct rte_mempool_ops *ops;
>> + int16_t ops_index;
>> +
>> + rte_spinlock_lock(&rte_mempool_ops_table.sl);
>> +
>> + if (rte_mempool_ops_table.num_ops >=
>> + RTE_MEMPOOL_MAX_OPS_IDX) {
>> + rte_spinlock_unlock(&rte_mempool_ops_table.sl);
>> + RTE_LOG(ERR, MEMPOOL,
>> + "Maximum number of mempool ops structs exceeded\n");
>> + return -ENOSPC;
>> + }
>> +
>> + if (h->put == NULL || h->get == NULL || h->get_count == NULL) {
>> + rte_spinlock_unlock(&rte_mempool_ops_table.sl);
>> + RTE_LOG(ERR, MEMPOOL,
>> + "Missing callback while registering mempool ops\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (strlen(h->name) >= sizeof(ops->name) - 1) {
>> + RTE_LOG(DEBUG, EAL, "%s(): mempool_ops <%s>: name too long\n",
>> + __func__, h->name);
>> + rte_errno = EEXIST;
>> + return NULL;
>> + }
> This has already been noticed by Shreyansh, but in case of:
>
> rte_mempool_ops.c:75:10: error: return makes integer from pointer
> without a cast [-Werror=int-conversion]
> return NULL;
> ^
Changed to return -EEXIST
>
>> +/* sets mempool ops previously registered by rte_mempool_ops_register */
>> +int
>> +rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name)
>
> When I compile with shared libraries enabled, I get the following error:
>
> librte_reorder.so: undefined reference to `rte_mempool_ops_table'
> librte_mbuf.so: undefined reference to `rte_mempool_set_ops_byname'
> ...
>
> The new functions and global variables must be in
> rte_mempool_version.map. This was in v5
> ( http://dpdk.org/ml/archives/dev/2016-May/039365.html ) but
> was dropped in v6.
OK, Added.
>
> Regards,
> Olivier
Thanks,
David.
More information about the dev
mailing list