| Message ID | 20240422143102.251-3-zhangfei.gao@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce UADK compression driver | expand |
Hi Zhangfei, Overall, a well written driver. Please see below comment. > +static int > +uadk_compress_pmd_config(struct rte_compressdev *dev, > + struct rte_compressdev_config *config) > +{ > + char mp_name[RTE_MEMPOOL_NAMESIZE]; > + struct uadk_compress_priv *priv; > + struct rte_mempool *mp; > + int ret; > + > + if (dev == NULL || config == NULL) > + return -EINVAL; > + > + snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, > + "stream_mp_%u", dev->data->dev_id); > + priv = dev->data->dev_private; > + > + /* alloc resources */ > + ret = wd_comp_env_init(NULL); > + if (ret < 0) > + return -EINVAL; > + > + mp = priv->mp; > + if (mp == NULL) { > + mp = rte_mempool_create(mp_name, > + config->max_nb_priv_xforms + > + config->max_nb_streams, > + sizeof(struct uadk_stream), > + 0, 0, NULL, NULL, NULL, > + NULL, config->socket_id, 0); > + if (mp == NULL) { > + UADK_LOG(ERR, "Cannot create private xform pool on > socket %d\n", > + config->socket_id); > + ret = -ENOMEM; > + goto err_mempool; > + } > + priv->mp = mp; > + } Do you really need a mempool here? It is for uadk_stream which is just struct of pointer and an enum. It can simply be rte_malloc. And even you do not need uadk_compress_priv. This can be simplified. Right? Also remove the execution part of documentation from 1/3 and add it in 3/3 Since the PMD is complete in 3/3, release notes and execution part of documentation should be in last patch.
Hi, Akhil Thanks for your time. On Thu, 23 May 2024 at 23:38, Akhil Goyal <gakhil@marvell.com> wrote: > > Hi Zhangfei, > > Overall, a well written driver. > Please see below comment. > > > +static int > > +uadk_compress_pmd_config(struct rte_compressdev *dev, > > + struct rte_compressdev_config *config) > > +{ > > + char mp_name[RTE_MEMPOOL_NAMESIZE]; > > + struct uadk_compress_priv *priv; > > + struct rte_mempool *mp; > > + int ret; > > + > > + if (dev == NULL || config == NULL) > > + return -EINVAL; > > + > > + snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, > > + "stream_mp_%u", dev->data->dev_id); > > + priv = dev->data->dev_private; > > + > > + /* alloc resources */ > > + ret = wd_comp_env_init(NULL); > > + if (ret < 0) > > + return -EINVAL; > > + > > + mp = priv->mp; > > + if (mp == NULL) { > > + mp = rte_mempool_create(mp_name, > > + config->max_nb_priv_xforms + > > + config->max_nb_streams, > > + sizeof(struct uadk_stream), > > + 0, 0, NULL, NULL, NULL, > > + NULL, config->socket_id, 0); > > + if (mp == NULL) { > > + UADK_LOG(ERR, "Cannot create private xform pool on > > socket %d\n", > > + config->socket_id); > > + ret = -ENOMEM; > > + goto err_mempool; > > + } > > + priv->mp = mp; > > + } > > Do you really need a mempool here? It is for uadk_stream which is just struct of pointer and an enum. > It can simply be rte_malloc. > And even you do not need uadk_compress_priv. > This can be simplified. Right? Yes, good idea, this can be simplified, and can remove uadk_compress_priv as well. But it looks like rte_compressdev_pmd_create requires the priv data, otherwise it will return an error if private_data_size == 0. Could rte_compressdev_pmd_create be changed only alloc compressdev->data->dev_private only if data_size != 0. Or I am checking whether to simply add one priv. > > Also remove the execution part of documentation from 1/3 and add it in 3/3 > Since the PMD is complete in 3/3, release notes and execution part of documentation should be in last patch. OK. One more question, rte_compressdev_pmd_init_params does not have .max_nb_queue_pairs as rte_cryptodev_pmd_init_params. So dpdk-test-compress-perf will use 128 queues by default, except adding -l 1,2. Is this expected? Thanks >
> > > +static int > > > +uadk_compress_pmd_config(struct rte_compressdev *dev, > > > + struct rte_compressdev_config *config) > > > +{ > > > + char mp_name[RTE_MEMPOOL_NAMESIZE]; > > > + struct uadk_compress_priv *priv; > > > + struct rte_mempool *mp; > > > + int ret; > > > + > > > + if (dev == NULL || config == NULL) > > > + return -EINVAL; > > > + > > > + snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, > > > + "stream_mp_%u", dev->data->dev_id); > > > + priv = dev->data->dev_private; > > > + > > > + /* alloc resources */ > > > + ret = wd_comp_env_init(NULL); > > > + if (ret < 0) > > > + return -EINVAL; > > > + > > > + mp = priv->mp; > > > + if (mp == NULL) { > > > + mp = rte_mempool_create(mp_name, > > > + config->max_nb_priv_xforms + > > > + config->max_nb_streams, > > > + sizeof(struct uadk_stream), > > > + 0, 0, NULL, NULL, NULL, > > > + NULL, config->socket_id, 0); > > > + if (mp == NULL) { > > > + UADK_LOG(ERR, "Cannot create private xform pool on > > > socket %d\n", > > > + config->socket_id); > > > + ret = -ENOMEM; > > > + goto err_mempool; > > > + } > > > + priv->mp = mp; > > > + } > > > > Do you really need a mempool here? It is for uadk_stream which is just struct of > pointer and an enum. > > It can simply be rte_malloc. > > And even you do not need uadk_compress_priv. > > This can be simplified. Right? > > Yes, good idea, this can be simplified, and can remove > uadk_compress_priv as well. > > But it looks like rte_compressdev_pmd_create requires the priv data, > otherwise it will return an error if private_data_size == 0. > Could rte_compressdev_pmd_create be changed only alloc > compressdev->data->dev_private only if data_size != 0. > > Or I am checking whether to simply add one priv. Normally, each PMD need some priv space. Even you can also add capabilities in it instead of having global variable. > > > > > Also remove the execution part of documentation from 1/3 and add it in 3/3 > > Since the PMD is complete in 3/3, release notes and execution part of > documentation should be in last patch. > OK. > > One more question, > rte_compressdev_pmd_init_params does not have .max_nb_queue_pairs as > rte_cryptodev_pmd_init_params. > So dpdk-test-compress-perf will use 128 queues by default, except > adding -l 1,2. > Is this expected? > rte_compressdev_pmd_init_params is internal for PMD-lib interaction and should not be used by app. For application, rte_compressdev_info should be used and it has that max_nb_queue_pairs.
On Fri, 24 May 2024 at 18:20, Akhil Goyal <gakhil@marvell.com> wrote: > > > > > +static int > > > > +uadk_compress_pmd_config(struct rte_compressdev *dev, > > > > + struct rte_compressdev_config *config) > > > > +{ > > > > + char mp_name[RTE_MEMPOOL_NAMESIZE]; > > > > + struct uadk_compress_priv *priv; > > > > + struct rte_mempool *mp; > > > > + int ret; > > > > + > > > > + if (dev == NULL || config == NULL) > > > > + return -EINVAL; > > > > + > > > > + snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, > > > > + "stream_mp_%u", dev->data->dev_id); > > > > + priv = dev->data->dev_private; > > > > + > > > > + /* alloc resources */ > > > > + ret = wd_comp_env_init(NULL); > > > > + if (ret < 0) > > > > + return -EINVAL; > > > > + > > > > + mp = priv->mp; > > > > + if (mp == NULL) { > > > > + mp = rte_mempool_create(mp_name, > > > > + config->max_nb_priv_xforms + > > > > + config->max_nb_streams, > > > > + sizeof(struct uadk_stream), > > > > + 0, 0, NULL, NULL, NULL, > > > > + NULL, config->socket_id, 0); > > > > + if (mp == NULL) { > > > > + UADK_LOG(ERR, "Cannot create private xform pool on > > > > socket %d\n", > > > > + config->socket_id); > > > > + ret = -ENOMEM; > > > > + goto err_mempool; > > > > + } > > > > + priv->mp = mp; > > > > + } > > > > > > Do you really need a mempool here? It is for uadk_stream which is just struct of > > pointer and an enum. > > > It can simply be rte_malloc. > > > And even you do not need uadk_compress_priv. > > > This can be simplified. Right? > > > > Yes, good idea, this can be simplified, and can remove > > uadk_compress_priv as well. > > > > But it looks like rte_compressdev_pmd_create requires the priv data, > > otherwise it will return an error if private_data_size == 0. > > Could rte_compressdev_pmd_create be changed only alloc > > compressdev->data->dev_private only if data_size != 0. > > > > Or I am checking whether to simply add one priv. > > Normally, each PMD need some priv space. > Even you can also add capabilities in it instead of having global variable. Yes, understand. > > > > > > > > > Also remove the execution part of documentation from 1/3 and add it in 3/3 > > > Since the PMD is complete in 3/3, release notes and execution part of > > documentation should be in last patch. > > OK. > > > > One more question, > > rte_compressdev_pmd_init_params does not have .max_nb_queue_pairs as > > rte_cryptodev_pmd_init_params. > > So dpdk-test-compress-perf will use 128 queues by default, except > > adding -l 1,2. > > Is this expected? > > > rte_compressdev_pmd_init_params is internal for PMD-lib interaction and should not be used by app. > For application, rte_compressdev_info should be used and it has that max_nb_queue_pairs. > Is there a method to configure the max_nb_queue_pairs? Seems not workable to use RTE_PMD_REGISTER_PARAM_STRING like crypto, which only sets the init_params, which has no member. Or have to use a fixed value? Thanks
> > > > > > One more question, > > > rte_compressdev_pmd_init_params does not have .max_nb_queue_pairs as > > > rte_cryptodev_pmd_init_params. > > > So dpdk-test-compress-perf will use 128 queues by default, except > > > adding -l 1,2. > > > Is this expected? > > > > > rte_compressdev_pmd_init_params is internal for PMD-lib interaction and > should not be used by app. > > For application, rte_compressdev_info should be used and it has that > max_nb_queue_pairs. > > > > Is there a method to configure the max_nb_queue_pairs? > Seems not workable to use RTE_PMD_REGISTER_PARAM_STRING like crypto, > which only sets the init_params, which has no member. > Or have to use a fixed value? > max_nb_queue_pairs is a capability of the PMD which it should set in rte_compressdev_info as per what it supports. However, if you want to control it runtime, you can setup devargs and parse in your driver. Check drivers/crypto/cnxk/cnxk_cryptodev_devargs.c for reference.
diff --git a/drivers/compress/uadk/uadk_compress_pmd.c b/drivers/compress/uadk/uadk_compress_pmd.c index d73524ce84..25858855e6 100644 --- a/drivers/compress/uadk/uadk_compress_pmd.c +++ b/drivers/compress/uadk/uadk_compress_pmd.c @@ -12,18 +12,321 @@ #include "uadk_compress_pmd_private.h" +static const struct +rte_compressdev_capabilities uadk_compress_pmd_capabilities[] = { + { /* Deflate */ + .algo = RTE_COMP_ALGO_DEFLATE, + .comp_feature_flags = RTE_COMP_FF_SHAREABLE_PRIV_XFORM | + RTE_COMP_FF_HUFFMAN_FIXED | + RTE_COMP_FF_HUFFMAN_DYNAMIC, + }, + + RTE_COMP_END_OF_CAPABILITIES_LIST() +}; + +static int +uadk_compress_pmd_config(struct rte_compressdev *dev, + struct rte_compressdev_config *config) +{ + char mp_name[RTE_MEMPOOL_NAMESIZE]; + struct uadk_compress_priv *priv; + struct rte_mempool *mp; + int ret; + + if (dev == NULL || config == NULL) + return -EINVAL; + + snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, + "stream_mp_%u", dev->data->dev_id); + priv = dev->data->dev_private; + + /* alloc resources */ + ret = wd_comp_env_init(NULL); + if (ret < 0) + return -EINVAL; + + mp = priv->mp; + if (mp == NULL) { + mp = rte_mempool_create(mp_name, + config->max_nb_priv_xforms + + config->max_nb_streams, + sizeof(struct uadk_stream), + 0, 0, NULL, NULL, NULL, + NULL, config->socket_id, 0); + if (mp == NULL) { + UADK_LOG(ERR, "Cannot create private xform pool on socket %d\n", + config->socket_id); + ret = -ENOMEM; + goto err_mempool; + } + priv->mp = mp; + } + + return 0; + +err_mempool: + wd_comp_env_uninit(); + return ret; +} + +static int +uadk_compress_pmd_start(struct rte_compressdev *dev __rte_unused) +{ + return 0; +} + +static void +uadk_compress_pmd_stop(struct rte_compressdev *dev __rte_unused) +{ +} + +static int +uadk_compress_pmd_close(struct rte_compressdev *dev) +{ + struct uadk_compress_priv *priv = + (struct uadk_compress_priv *)dev->data->dev_private; + + /* free resources */ + rte_mempool_free(priv->mp); + priv->mp = NULL; + wd_comp_env_uninit(); + + return 0; +} + +static void +uadk_compress_pmd_stats_get(struct rte_compressdev *dev, + struct rte_compressdev_stats *stats) +{ + int qp_id; + + for (qp_id = 0; qp_id < dev->data->nb_queue_pairs; qp_id++) { + struct uadk_qp *qp = dev->data->queue_pairs[qp_id]; + + stats->enqueued_count += qp->qp_stats.enqueued_count; + stats->dequeued_count += qp->qp_stats.dequeued_count; + stats->enqueue_err_count += qp->qp_stats.enqueue_err_count; + stats->dequeue_err_count += qp->qp_stats.dequeue_err_count; + } +} + +static void +uadk_compress_pmd_stats_reset(struct rte_compressdev *dev) +{ + int qp_id; + + for (qp_id = 0; qp_id < dev->data->nb_queue_pairs; qp_id++) { + struct uadk_qp *qp = dev->data->queue_pairs[qp_id]; + + memset(&qp->qp_stats, 0, sizeof(qp->qp_stats)); + } +} + +static void +uadk_compress_pmd_info_get(struct rte_compressdev *dev, + struct rte_compressdev_info *dev_info) +{ + if (dev_info != NULL) { + dev_info->driver_name = dev->device->driver->name; + dev_info->feature_flags = dev->feature_flags; + dev_info->capabilities = uadk_compress_pmd_capabilities; + } +} + +static int +uadk_compress_pmd_qp_release(struct rte_compressdev *dev, uint16_t qp_id) +{ + struct uadk_qp *qp = dev->data->queue_pairs[qp_id]; + + if (qp != NULL) { + rte_ring_free(qp->processed_pkts); + rte_free(qp); + dev->data->queue_pairs[qp_id] = NULL; + } + + return 0; +} + +static int +uadk_pmd_qp_set_unique_name(struct rte_compressdev *dev, + struct uadk_qp *qp) +{ + unsigned int n = snprintf(qp->name, sizeof(qp->name), + "uadk_pmd_%u_qp_%u", + dev->data->dev_id, qp->id); + + if (n >= sizeof(qp->name)) + return -EINVAL; + + return 0; +} + +static struct rte_ring * +uadk_pmd_qp_create_processed_pkts_ring(struct uadk_qp *qp, + unsigned int ring_size, int socket_id) +{ + struct rte_ring *r = qp->processed_pkts; + + if (r) { + if (rte_ring_get_size(r) >= ring_size) { + UADK_LOG(INFO, "Reusing existing ring %s for processed packets", + qp->name); + return r; + } + + UADK_LOG(ERR, "Unable to reuse existing ring %s for processed packets", + qp->name); + return NULL; + } + + return rte_ring_create(qp->name, ring_size, socket_id, + RING_F_EXACT_SZ); +} + +static int +uadk_compress_pmd_qp_setup(struct rte_compressdev *dev, uint16_t qp_id, + uint32_t max_inflight_ops, int socket_id) +{ + struct uadk_qp *qp = NULL; + + /* Free memory prior to re-allocation if needed. */ + if (dev->data->queue_pairs[qp_id] != NULL) + uadk_compress_pmd_qp_release(dev, qp_id); + + /* Allocate the queue pair data structure. */ + qp = rte_zmalloc_socket("uadk PMD Queue Pair", sizeof(*qp), + RTE_CACHE_LINE_SIZE, socket_id); + if (qp == NULL) + return (-ENOMEM); + + qp->id = qp_id; + dev->data->queue_pairs[qp_id] = qp; + + if (uadk_pmd_qp_set_unique_name(dev, qp)) + goto qp_setup_cleanup; + + qp->processed_pkts = uadk_pmd_qp_create_processed_pkts_ring(qp, + max_inflight_ops, socket_id); + if (qp->processed_pkts == NULL) + goto qp_setup_cleanup; + + memset(&qp->qp_stats, 0, sizeof(qp->qp_stats)); + + return 0; + +qp_setup_cleanup: + if (qp) { + rte_free(qp); + qp = NULL; + } + return -EINVAL; +} + +static int +uadk_compress_pmd_xform_create(struct rte_compressdev *dev, + const struct rte_comp_xform *xform, + void **private_xform) +{ + struct uadk_compress_priv *priv = dev->data->dev_private; + struct wd_comp_sess_setup setup = {0}; + struct sched_params param = {0}; + struct uadk_stream *stream; + handle_t handle; + + if (xform == NULL) { + UADK_LOG(ERR, "invalid xform struct"); + return -EINVAL; + } + + if (rte_mempool_get(priv->mp, private_xform)) { + UADK_LOG(ERR, "Couldn't get object from session mempool"); + return -ENOMEM; + } + + stream = *((struct uadk_stream **)private_xform); + + switch (xform->type) { + case RTE_COMP_COMPRESS: + switch (xform->compress.algo) { + case RTE_COMP_ALGO_NULL: + break; + case RTE_COMP_ALGO_DEFLATE: + setup.alg_type = WD_DEFLATE; + setup.win_sz = WD_COMP_WS_8K; + setup.comp_lv = WD_COMP_L8; + setup.op_type = WD_DIR_COMPRESS; + param.type = setup.op_type; + param.numa_id = -1; /* choose nearby numa node */ + setup.sched_param = ¶m; + break; + default: + goto err; + } + break; + case RTE_COMP_DECOMPRESS: + switch (xform->decompress.algo) { + case RTE_COMP_ALGO_NULL: + break; + case RTE_COMP_ALGO_DEFLATE: + setup.alg_type = WD_DEFLATE; + setup.comp_lv = WD_COMP_L8; + setup.op_type = WD_DIR_DECOMPRESS; + param.type = setup.op_type; + param.numa_id = -1; /* choose nearby numa node */ + setup.sched_param = ¶m; + break; + default: + goto err; + } + break; + default: + UADK_LOG(ERR, "Algorithm %u is not supported.", xform->type); + goto err; + } + + handle = wd_comp_alloc_sess(&setup); + if (!handle) + goto err; + + stream->handle = handle; + stream->type = xform->type; + + return 0; + +err: + rte_mempool_put(priv->mp, private_xform); + return -EINVAL; +} + +static int +uadk_compress_pmd_xform_free(struct rte_compressdev *dev __rte_unused, void *private_xform) +{ + struct uadk_stream *stream = (struct uadk_stream *)private_xform; + struct rte_mempool *mp; + + if (!stream) + return -EINVAL; + + wd_comp_free_sess(stream->handle); + memset(stream, 0, sizeof(struct uadk_stream)); + mp = rte_mempool_from_obj(stream); + rte_mempool_put(mp, stream); + + return 0; +} + static struct rte_compressdev_ops uadk_compress_pmd_ops = { - .dev_configure = NULL, - .dev_start = NULL, - .dev_stop = NULL, - .dev_close = NULL, - .stats_get = NULL, - .stats_reset = NULL, - .dev_infos_get = NULL, - .queue_pair_setup = NULL, - .queue_pair_release = NULL, - .private_xform_create = NULL, - .private_xform_free = NULL, + .dev_configure = uadk_compress_pmd_config, + .dev_start = uadk_compress_pmd_start, + .dev_stop = uadk_compress_pmd_stop, + .dev_close = uadk_compress_pmd_close, + .stats_get = uadk_compress_pmd_stats_get, + .stats_reset = uadk_compress_pmd_stats_reset, + .dev_infos_get = uadk_compress_pmd_info_get, + .queue_pair_setup = uadk_compress_pmd_qp_setup, + .queue_pair_release = uadk_compress_pmd_qp_release, + .private_xform_create = uadk_compress_pmd_xform_create, + .private_xform_free = uadk_compress_pmd_xform_free, .stream_create = NULL, .stream_free = NULL, }; diff --git a/drivers/compress/uadk/uadk_compress_pmd_private.h b/drivers/compress/uadk/uadk_compress_pmd_private.h index a96aea7c73..97a921316e 100644 --- a/drivers/compress/uadk/uadk_compress_pmd_private.h +++ b/drivers/compress/uadk/uadk_compress_pmd_private.h @@ -10,6 +10,22 @@ struct uadk_compress_priv { struct rte_mempool *mp; }; +struct __rte_cache_aligned uadk_qp { + /* Ring for placing process packets */ + struct rte_ring *processed_pkts; + /* Queue pair statistics */ + struct rte_compressdev_stats qp_stats; + /* Queue Pair Identifier */ + uint16_t id; + /* Unique Queue Pair Name */ + char name[RTE_COMPRESSDEV_NAME_MAX_LEN]; +}; + +struct __rte_cache_aligned uadk_stream { + handle_t handle; + enum rte_comp_xform_type type; +}; + extern int uadk_compress_logtype; #define UADK_LOG(level, fmt, ...) \
Support the basic dev control operations: configure, close, start, stop, infos_get, and queue pairs operations, etc. Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> --- drivers/compress/uadk/uadk_compress_pmd.c | 325 +++++++++++++++++- .../compress/uadk/uadk_compress_pmd_private.h | 16 + 2 files changed, 330 insertions(+), 11 deletions(-)