[dpdk-dev,v1,2/6] mempool: implement abstract mempool info API
Checks
Commit Message
From: "Artem V. Andreev" <Artem.Andreev@oktetlabs.ru>
Primarily, it is intended as a way for the mempool driver to provide
additional information on how it lays up objects inside the mempool.
Signed-off-by: Artem V. Andreev <Artem.Andreev@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
lib/librte_mempool/rte_mempool.h | 41 ++++++++++++++++++++++++++++++
lib/librte_mempool/rte_mempool_ops.c | 15 +++++++++++
lib/librte_mempool/rte_mempool_version.map | 7 +++++
3 files changed, 63 insertions(+)
Comments
On Mon, Mar 26, 2018 at 05:12:55PM +0100, Andrew Rybchenko wrote:
> From: "Artem V. Andreev" <Artem.Andreev@oktetlabs.ru>
>
> Primarily, it is intended as a way for the mempool driver to provide
> additional information on how it lays up objects inside the mempool.
>
> Signed-off-by: Artem V. Andreev <Artem.Andreev@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
I think it's a good idea to have a way to query mempool features
or parameters. The approach chosen in this patch looks similar
to what we have with ethdev devinfo, right?
[...]
> /**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Additional information about the mempool
> + */
> +struct rte_mempool_info;
> +
[...]
> +/* wrapper to get additional mempool info */
> +int
> +rte_mempool_ops_get_info(const struct rte_mempool *mp,
> + struct rte_mempool_info *info)
> +{
> + struct rte_mempool_ops *ops;
> +
> + ops = rte_mempool_get_ops(mp->ops_index);
> +
> + RTE_FUNC_PTR_OR_ERR_RET(ops->get_info, -ENOTSUP);
> + return ops->get_info(mp, info);
> +}
Thinking in terms of ABI compatibility, it looks that each time we will
add or remove a field, it will break the ABI because the info structure
will change.
Well, it's maybe nitpicking, because most of the time adding a field in
info structure goes with adding a field in the mempool struct, which
will anyway break the ABI.
Another approach is to have a function
rte_mempool_info_contig_block_size(mp) whose ABI can be more easily
wrapped with VERSION_SYMBOL().
On my side I'm fine with your current approach, especially given how few
usages of VERSION_SYMBOL() we can find in DPDK. But in case you feel
it's better to have a function...
On 04/19/2018 07:42 PM, Olivier Matz wrote:
> On Mon, Mar 26, 2018 at 05:12:55PM +0100, Andrew Rybchenko wrote:
>> From: "Artem V. Andreev" <Artem.Andreev@oktetlabs.ru>
>>
>> Primarily, it is intended as a way for the mempool driver to provide
>> additional information on how it lays up objects inside the mempool.
>>
>> Signed-off-by: Artem V. Andreev <Artem.Andreev@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> I think it's a good idea to have a way to query mempool features
> or parameters. The approach chosen in this patch looks similar
> to what we have with ethdev devinfo, right?
Yes.
> [...]
>
>> /**
>> + * @warning
>> + * @b EXPERIMENTAL: this API may change without prior notice.
>> + *
>> + * Additional information about the mempool
>> + */
>> +struct rte_mempool_info;
>> +
> [...]
>
>> +/* wrapper to get additional mempool info */
>> +int
>> +rte_mempool_ops_get_info(const struct rte_mempool *mp,
>> + struct rte_mempool_info *info)
>> +{
>> + struct rte_mempool_ops *ops;
>> +
>> + ops = rte_mempool_get_ops(mp->ops_index);
>> +
>> + RTE_FUNC_PTR_OR_ERR_RET(ops->get_info, -ENOTSUP);
>> + return ops->get_info(mp, info);
>> +}
> Thinking in terms of ABI compatibility, it looks that each time we will
> add or remove a field, it will break the ABI because the info structure
> will change.
>
> Well, it's maybe nitpicking, because most of the time adding a field in
> info structure goes with adding a field in the mempool struct, which
> will anyway break the ABI.
>
> Another approach is to have a function
> rte_mempool_info_contig_block_size(mp) whose ABI can be more easily
> wrapped with VERSION_SYMBOL().
>
> On my side I'm fine with your current approach, especially given how few
> usages of VERSION_SYMBOL() we can find in DPDK. But in case you feel
> it's better to have a function...
I'd prefer to keep current solution. Otherwise it could result in too many
different functions to get various information about mempool driver
features/characteristics. Also it could be not very convenient to get
many parameters.
May be we should align info structure size to cache line to avoid size
changes in many cases? Typically it will be used on slow path and
located on caller stack and adding some bytes more should not
be a problem.
Hi Andrew,
> > > + * @warning
> > > + * @b EXPERIMENTAL: this API may change without prior notice.
> > > + *
> > > + * Additional information about the mempool
> > > + */
> > > +struct rte_mempool_info;
> > > +
> > [...]
> >
> > > +/* wrapper to get additional mempool info */
> > > +int
> > > +rte_mempool_ops_get_info(const struct rte_mempool *mp,
> > > + struct rte_mempool_info *info)
> > > +{
> > > + struct rte_mempool_ops *ops;
> > > +
> > > + ops = rte_mempool_get_ops(mp->ops_index);
> > > +
> > > + RTE_FUNC_PTR_OR_ERR_RET(ops->get_info, -ENOTSUP);
> > > + return ops->get_info(mp, info);
> > > +}
> > Thinking in terms of ABI compatibility, it looks that each time we will
> > add or remove a field, it will break the ABI because the info structure
> > will change.
> >
> > Well, it's maybe nitpicking, because most of the time adding a field in
> > info structure goes with adding a field in the mempool struct, which
> > will anyway break the ABI.
> >
> > Another approach is to have a function
> > rte_mempool_info_contig_block_size(mp) whose ABI can be more easily
> > wrapped with VERSION_SYMBOL().
> >
> > On my side I'm fine with your current approach, especially given how few
> > usages of VERSION_SYMBOL() we can find in DPDK. But in case you feel
> > it's better to have a function...
>
> I'd prefer to keep current solution. Otherwise it could result in too many
> different functions to get various information about mempool driver
> features/characteristics. Also it could be not very convenient to get
> many parameters.
>
> May be we should align info structure size to cache line to avoid size
> changes in many cases? Typically it will be used on slow path and
> located on caller stack and adding some bytes more should not
> be a problem.
Yes, that could be a good thing to do.
Thanks,
Olivier
@@ -190,6 +190,14 @@ struct rte_mempool_memhdr {
};
/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Additional information about the mempool
+ */
+struct rte_mempool_info;
+
+/**
* The RTE mempool structure.
*/
struct rte_mempool {
@@ -499,6 +507,16 @@ int rte_mempool_op_populate_default(struct rte_mempool *mp,
void *vaddr, rte_iova_t iova, size_t len,
rte_mempool_populate_obj_cb_t *obj_cb, void *obj_cb_arg);
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Get some additional information about a mempool.
+ */
+typedef int (*rte_mempool_get_info_t)(const struct rte_mempool *mp,
+ struct rte_mempool_info *info);
+
+
/** Structure defining mempool operations structure */
struct rte_mempool_ops {
char name[RTE_MEMPOOL_OPS_NAMESIZE]; /**< Name of mempool ops struct. */
@@ -517,6 +535,10 @@ struct rte_mempool_ops {
* provided memory chunk.
*/
rte_mempool_populate_t populate;
+ /**
+ * Get mempool info
+ */
+ rte_mempool_get_info_t get_info;
} __rte_cache_aligned;
#define RTE_MEMPOOL_MAX_OPS_IDX 16 /**< Max registered ops structs */
@@ -680,6 +702,25 @@ int rte_mempool_ops_populate(struct rte_mempool *mp, unsigned int max_objs,
void *obj_cb_arg);
/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Wrapper for mempool_ops get_info callback.
+ *
+ * @param[in] mp
+ * Pointer to the memory pool.
+ * @param[out] info
+ * Pointer to the rte_mempool_info structure
+ * @return
+ * - 0: Success; The mempool driver supports retrieving supplementary
+ * mempool information
+ * - -ENOTSUP - doesn't support get_info ops (valid case).
+ */
+__rte_experimental
+int rte_mempool_ops_get_info(const struct rte_mempool *mp,
+ struct rte_mempool_info *info);
+
+/**
* @internal wrapper for mempool_ops free callback.
*
* @param mp
@@ -59,6 +59,7 @@ rte_mempool_register_ops(const struct rte_mempool_ops *h)
ops->get_count = h->get_count;
ops->calc_mem_size = h->calc_mem_size;
ops->populate = h->populate;
+ ops->get_info = h->get_info;
rte_spinlock_unlock(&rte_mempool_ops_table.sl);
@@ -134,6 +135,20 @@ rte_mempool_ops_populate(struct rte_mempool *mp, unsigned int max_objs,
obj_cb_arg);
}
+/* wrapper to get additional mempool info */
+int
+rte_mempool_ops_get_info(const struct rte_mempool *mp,
+ struct rte_mempool_info *info)
+{
+ struct rte_mempool_ops *ops;
+
+ ops = rte_mempool_get_ops(mp->ops_index);
+
+ RTE_FUNC_PTR_OR_ERR_RET(ops->get_info, -ENOTSUP);
+ return ops->get_info(mp, info);
+}
+
+
/* sets mempool ops previously registered by rte_mempool_register_ops. */
int
rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
@@ -57,3 +57,10 @@ DPDK_18.05 {
rte_mempool_op_populate_default;
} DPDK_17.11;
+
+EXPERIMENTAL {
+ global:
+
+ rte_mempool_ops_get_info;
+
+} DPDK_18.05;