[dpdk-dev,v2,03/12] cryptodev: avoid dependency on rte_vdev.h
Checks
Commit Message
The helper API, rte_cryptodev_vdev_pmd_init(), has a parameter of
struct rte_vdev_device *, which needs to include rte_vdev.h.
We will move vdev into drivers/bus, so we need to avoid such
dependency. And also, we cannot break the ABI.
This patch changes that pointer to void *, and defines an internal
structure same with struct rte_vdev_device.
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
lib/librte_cryptodev/rte_cryptodev_pmd.c | 9 +-
lib/librte_cryptodev/rte_cryptodev_vdev.c | 161 ++++++++++++++++++++++++++++++
lib/librte_cryptodev/rte_cryptodev_vdev.h | 3 +-
3 files changed, 169 insertions(+), 4 deletions(-)
create mode 100644 lib/librte_cryptodev/rte_cryptodev_vdev.c
Comments
On Thu, Sep 28, 2017 at 3:55 PM, Jianfeng Tan <jianfeng.tan@intel.com> wrote:
> The helper API, rte_cryptodev_vdev_pmd_init(), has a parameter of
> struct rte_vdev_device *, which needs to include rte_vdev.h.
>
> We will move vdev into drivers/bus, so we need to avoid such
> dependency. And also, we cannot break the ABI.
>
> This patch changes that pointer to void *, and defines an internal
> structure same with struct rte_vdev_device.
>
This code duplication is unnecessary. Also you are doing evil thinks
and define a type with the same name in multiple places. Look at my
series how to do this properly.
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
> lib/librte_cryptodev/rte_cryptodev_pmd.c | 9 +-
> lib/librte_cryptodev/rte_cryptodev_vdev.c | 161 ++++++++++++++++++++++++++++++
> lib/librte_cryptodev/rte_cryptodev_vdev.h | 3 +-
> 3 files changed, 169 insertions(+), 4 deletions(-)
> create mode 100644 lib/librte_cryptodev/rte_cryptodev_vdev.c
>
> diff --git a/lib/librte_cryptodev/rte_cryptodev_pmd.c b/lib/librte_cryptodev/rte_cryptodev_pmd.c
> index a57faad..60b3980 100644
> --- a/lib/librte_cryptodev/rte_cryptodev_pmd.c
> +++ b/lib/librte_cryptodev/rte_cryptodev_pmd.c
> @@ -75,9 +75,14 @@ rte_cryptodev_vdev_parse_integer_arg(const char *key __rte_unused,
> return 0;
> }
>
> +struct vdev_device {
> + TAILQ_ENTRY(rte_vdev_device) next;
> + struct rte_device device;
> +};
> +
> struct rte_cryptodev *
> rte_cryptodev_vdev_pmd_init(const char *name, size_t dev_private_size,
> - int socket_id, struct rte_vdev_device *vdev)
> + int socket_id, void *vdev)
> {
> struct rte_cryptodev *cryptodev;
>
> @@ -99,7 +104,7 @@ rte_cryptodev_vdev_pmd_init(const char *name, size_t dev_private_size,
> " data");
> }
>
> - cryptodev->device = &vdev->device;
> + cryptodev->device = &((struct vdev_device *)vdev)->device;
>
> /* initialise user call-back tail queue */
> TAILQ_INIT(&(cryptodev->link_intr_cbs));
> diff --git a/lib/librte_cryptodev/rte_cryptodev_vdev.c b/lib/librte_cryptodev/rte_cryptodev_vdev.c
> new file mode 100644
> index 0000000..4af8d51
> --- /dev/null
> +++ b/lib/librte_cryptodev/rte_cryptodev_vdev.c
> @@ -0,0 +1,161 @@
> +/*-
> + * BSD LICENSE
> + *
> + * Copyright(c) 2017 Intel Corporation. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in
> + * the documentation and/or other materials provided with the
> + * distribution.
> + * * Neither the name of the copyright holder nor the names of its
> + * contributors may be used to endorse or promote products derived
> + * from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <sys/queue.h>
> +
> +#include "rte_cryptodev_vdev.h"
> +#include "rte_cryptodev_pci.h"
> +#include "rte_cryptodev_pmd.h"
> +
> +/**
> + * Parse name from argument
> + */
> +static int
> +rte_cryptodev_vdev_parse_name_arg(const char *key __rte_unused,
> + const char *value, void *extra_args)
> +{
> + struct rte_crypto_vdev_init_params *params = extra_args;
> +
> + if (strlen(value) >= RTE_CRYPTODEV_NAME_MAX_LEN - 1) {
> + CDEV_LOG_ERR("Invalid name %s, should be less than "
> + "%u bytes", value,
> + RTE_CRYPTODEV_NAME_MAX_LEN - 1);
> + return -1;
> + }
> +
> + strncpy(params->name, value, RTE_CRYPTODEV_NAME_MAX_LEN);
> +
> + return 0;
> +}
> +
> +/**
> + * Parse integer from argument
> + */
> +static int
> +rte_cryptodev_vdev_parse_integer_arg(const char *key __rte_unused,
> + const char *value, void *extra_args)
> +{
> + int *i = extra_args;
> +
> + *i = atoi(value);
> + if (*i < 0) {
> + CDEV_LOG_ERR("Argument has to be positive.");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +struct vdev_device {
> + TAILQ_ENTRY(rte_vdev_device) next;
> + struct rte_device device;
> +};
> +
> +struct rte_cryptodev *
> +rte_cryptodev_vdev_pmd_init(const char *name, size_t dev_private_size,
> + int socket_id, void *vdev)
> +{
> + struct rte_cryptodev *cryptodev;
> +
> + /* allocate device structure */
> + cryptodev = rte_cryptodev_pmd_allocate(name, socket_id);
> + if (cryptodev == NULL)
> + return NULL;
> +
> + /* allocate private device structure */
> + if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> + cryptodev->data->dev_private =
> + rte_zmalloc_socket("cryptodev device private",
> + dev_private_size,
> + RTE_CACHE_LINE_SIZE,
> + socket_id);
> +
> + if (cryptodev->data->dev_private == NULL)
> + rte_panic("Cannot allocate memzone for private device"
> + " data");
> + }
> +
> + cryptodev->device = &((struct vdev_device *)vdev)->device;
> +
> + /* initialise user call-back tail queue */
> + TAILQ_INIT(&(cryptodev->link_intr_cbs));
> +
> + return cryptodev;
> +}
> +
> +int
> +rte_cryptodev_vdev_parse_init_params(struct rte_crypto_vdev_init_params *params,
> + const char *input_args)
> +{
> + struct rte_kvargs *kvlist = NULL;
> + int ret = 0;
> +
> + if (params == NULL)
> + return -EINVAL;
> +
> + if (input_args) {
> + kvlist = rte_kvargs_parse(input_args,
> + cryptodev_vdev_valid_params);
> + if (kvlist == NULL)
> + return -1;
> +
> + ret = rte_kvargs_process(kvlist,
> + RTE_CRYPTODEV_VDEV_MAX_NB_QP_ARG,
> + &rte_cryptodev_vdev_parse_integer_arg,
> + ¶ms->max_nb_queue_pairs);
> + if (ret < 0)
> + goto free_kvlist;
> +
> + ret = rte_kvargs_process(kvlist,
> + RTE_CRYPTODEV_VDEV_MAX_NB_SESS_ARG,
> + &rte_cryptodev_vdev_parse_integer_arg,
> + ¶ms->max_nb_sessions);
> + if (ret < 0)
> + goto free_kvlist;
> +
> + ret = rte_kvargs_process(kvlist, RTE_CRYPTODEV_VDEV_SOCKET_ID,
> + &rte_cryptodev_vdev_parse_integer_arg,
> + ¶ms->socket_id);
> + if (ret < 0)
> + goto free_kvlist;
> +
> + ret = rte_kvargs_process(kvlist, RTE_CRYPTODEV_VDEV_NAME,
> + &rte_cryptodev_vdev_parse_name_arg,
> + params);
> + if (ret < 0)
> + goto free_kvlist;
> + }
> +
> +free_kvlist:
> + rte_kvargs_free(kvlist);
> + return ret;
> +}
> diff --git a/lib/librte_cryptodev/rte_cryptodev_vdev.h b/lib/librte_cryptodev/rte_cryptodev_vdev.h
> index 94ab9d3..1142d1d 100644
> --- a/lib/librte_cryptodev/rte_cryptodev_vdev.h
> +++ b/lib/librte_cryptodev/rte_cryptodev_vdev.h
> @@ -33,7 +33,6 @@
> #ifndef _RTE_CRYPTODEV_VDEV_H_
> #define _RTE_CRYPTODEV_VDEV_H_
>
> -#include <rte_vdev.h>
> #include <inttypes.h>
>
> #include "rte_cryptodev.h"
> @@ -80,7 +79,7 @@ struct rte_crypto_vdev_init_params {
> */
> struct rte_cryptodev *
> rte_cryptodev_vdev_pmd_init(const char *name, size_t dev_private_size,
> - int socket_id, struct rte_vdev_device *vdev);
> + int socket_id, void *vdev);
>
> /**
> * @internal
> --
> 2.7.4
>
Hi Jan,
> -----Original Message-----
> From: jblunck@gmail.com [mailto:jblunck@gmail.com] On Behalf Of Jan
> Blunck
> Sent: Thursday, October 5, 2017 9:14 PM
> To: Tan, Jianfeng
> Cc: dev; Richardson, Bruce; Ananyev, Konstantin; De Lara Guarch, Pablo;
> Thomas Monjalon; yliu@fridaylinux.org; Maxime Coquelin; Tetsuya Mukawa;
> Yigit, Ferruh
> Subject: Re: [dpdk-dev] [PATCH v2 03/12] cryptodev: avoid dependency on
> rte_vdev.h
>
> On Thu, Sep 28, 2017 at 3:55 PM, Jianfeng Tan <jianfeng.tan@intel.com>
> wrote:
> > The helper API, rte_cryptodev_vdev_pmd_init(), has a parameter of
> > struct rte_vdev_device *, which needs to include rte_vdev.h.
> >
> > We will move vdev into drivers/bus, so we need to avoid such
> > dependency. And also, we cannot break the ABI.
> >
> > This patch changes that pointer to void *, and defines an internal
> > structure same with struct rte_vdev_device.
> >
>
> This code duplication is unnecessary. Also you are doing evil thinks
> and define a type with the same name in multiple places. Look at my
> series how to do this properly.
>
Moving related things into header file is also my plan. Thank for doing that, I'll rebase on your patchset.
Thanks,
Jianfeng
@@ -75,9 +75,14 @@ rte_cryptodev_vdev_parse_integer_arg(const char *key __rte_unused,
return 0;
}
+struct vdev_device {
+ TAILQ_ENTRY(rte_vdev_device) next;
+ struct rte_device device;
+};
+
struct rte_cryptodev *
rte_cryptodev_vdev_pmd_init(const char *name, size_t dev_private_size,
- int socket_id, struct rte_vdev_device *vdev)
+ int socket_id, void *vdev)
{
struct rte_cryptodev *cryptodev;
@@ -99,7 +104,7 @@ rte_cryptodev_vdev_pmd_init(const char *name, size_t dev_private_size,
" data");
}
- cryptodev->device = &vdev->device;
+ cryptodev->device = &((struct vdev_device *)vdev)->device;
/* initialise user call-back tail queue */
TAILQ_INIT(&(cryptodev->link_intr_cbs));
new file mode 100644
@@ -0,0 +1,161 @@
+/*-
+ * BSD LICENSE
+ *
+ * Copyright(c) 2017 Intel Corporation. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in
+ * the documentation and/or other materials provided with the
+ * distribution.
+ * * Neither the name of the copyright holder nor the names of its
+ * contributors may be used to endorse or promote products derived
+ * from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <sys/queue.h>
+
+#include "rte_cryptodev_vdev.h"
+#include "rte_cryptodev_pci.h"
+#include "rte_cryptodev_pmd.h"
+
+/**
+ * Parse name from argument
+ */
+static int
+rte_cryptodev_vdev_parse_name_arg(const char *key __rte_unused,
+ const char *value, void *extra_args)
+{
+ struct rte_crypto_vdev_init_params *params = extra_args;
+
+ if (strlen(value) >= RTE_CRYPTODEV_NAME_MAX_LEN - 1) {
+ CDEV_LOG_ERR("Invalid name %s, should be less than "
+ "%u bytes", value,
+ RTE_CRYPTODEV_NAME_MAX_LEN - 1);
+ return -1;
+ }
+
+ strncpy(params->name, value, RTE_CRYPTODEV_NAME_MAX_LEN);
+
+ return 0;
+}
+
+/**
+ * Parse integer from argument
+ */
+static int
+rte_cryptodev_vdev_parse_integer_arg(const char *key __rte_unused,
+ const char *value, void *extra_args)
+{
+ int *i = extra_args;
+
+ *i = atoi(value);
+ if (*i < 0) {
+ CDEV_LOG_ERR("Argument has to be positive.");
+ return -1;
+ }
+
+ return 0;
+}
+
+struct vdev_device {
+ TAILQ_ENTRY(rte_vdev_device) next;
+ struct rte_device device;
+};
+
+struct rte_cryptodev *
+rte_cryptodev_vdev_pmd_init(const char *name, size_t dev_private_size,
+ int socket_id, void *vdev)
+{
+ struct rte_cryptodev *cryptodev;
+
+ /* allocate device structure */
+ cryptodev = rte_cryptodev_pmd_allocate(name, socket_id);
+ if (cryptodev == NULL)
+ return NULL;
+
+ /* allocate private device structure */
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+ cryptodev->data->dev_private =
+ rte_zmalloc_socket("cryptodev device private",
+ dev_private_size,
+ RTE_CACHE_LINE_SIZE,
+ socket_id);
+
+ if (cryptodev->data->dev_private == NULL)
+ rte_panic("Cannot allocate memzone for private device"
+ " data");
+ }
+
+ cryptodev->device = &((struct vdev_device *)vdev)->device;
+
+ /* initialise user call-back tail queue */
+ TAILQ_INIT(&(cryptodev->link_intr_cbs));
+
+ return cryptodev;
+}
+
+int
+rte_cryptodev_vdev_parse_init_params(struct rte_crypto_vdev_init_params *params,
+ const char *input_args)
+{
+ struct rte_kvargs *kvlist = NULL;
+ int ret = 0;
+
+ if (params == NULL)
+ return -EINVAL;
+
+ if (input_args) {
+ kvlist = rte_kvargs_parse(input_args,
+ cryptodev_vdev_valid_params);
+ if (kvlist == NULL)
+ return -1;
+
+ ret = rte_kvargs_process(kvlist,
+ RTE_CRYPTODEV_VDEV_MAX_NB_QP_ARG,
+ &rte_cryptodev_vdev_parse_integer_arg,
+ ¶ms->max_nb_queue_pairs);
+ if (ret < 0)
+ goto free_kvlist;
+
+ ret = rte_kvargs_process(kvlist,
+ RTE_CRYPTODEV_VDEV_MAX_NB_SESS_ARG,
+ &rte_cryptodev_vdev_parse_integer_arg,
+ ¶ms->max_nb_sessions);
+ if (ret < 0)
+ goto free_kvlist;
+
+ ret = rte_kvargs_process(kvlist, RTE_CRYPTODEV_VDEV_SOCKET_ID,
+ &rte_cryptodev_vdev_parse_integer_arg,
+ ¶ms->socket_id);
+ if (ret < 0)
+ goto free_kvlist;
+
+ ret = rte_kvargs_process(kvlist, RTE_CRYPTODEV_VDEV_NAME,
+ &rte_cryptodev_vdev_parse_name_arg,
+ params);
+ if (ret < 0)
+ goto free_kvlist;
+ }
+
+free_kvlist:
+ rte_kvargs_free(kvlist);
+ return ret;
+}
@@ -33,7 +33,6 @@
#ifndef _RTE_CRYPTODEV_VDEV_H_
#define _RTE_CRYPTODEV_VDEV_H_
-#include <rte_vdev.h>
#include <inttypes.h>
#include "rte_cryptodev.h"
@@ -80,7 +79,7 @@ struct rte_crypto_vdev_init_params {
*/
struct rte_cryptodev *
rte_cryptodev_vdev_pmd_init(const char *name, size_t dev_private_size,
- int socket_id, struct rte_vdev_device *vdev);
+ int socket_id, void *vdev);
/**
* @internal