[PATCH-RFC] platform: linux-dpdk: crypto accelaration support

Message ID DCD768B7-8867-46BD-AE9E-8C89177A3E9C@nokia-bell-labs.com
State New
Headers show

Commit Message

Elo, Matias (Nokia - FI/Espoo) Nov. 16, 2016, 9:11 a.m.
Hi Krishna,

Some comments below.

-Matias


Secondly, to avoid confusion internal function names shouldn’t start with 'odp_’. The functions defined in odp_internal.h are old and should also be renamed (not in this commit).

Comments

Balakrishna Garapati Nov. 21, 2016, 8:26 a.m. | #1
On 16 November 2016 at 10:11, Elo, Matias (Nokia - FI/Espoo) <
matias.elo@nokia-bell-labs.com> wrote:

> Hi Krishna,
>
> Some comments below.
>
> -Matias
>
>
> diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux-dpdk/
> Makefile.am
> index 488f2c0..6ce0ce6 100644
> --- a/platform/linux-dpdk/Makefile.am
> +++ b/platform/linux-dpdk/Makefile.am
> @@ -121,6 +121,7 @@ noinst_HEADERS = \
>  ${top_srcdir}/platform/linux-generic/include/odp_classification_inlines.h
> \
>  ${top_srcdir}/platform/linux-generic/include/odp_classification_internal.h
> \
>  ${top_srcdir}/platform/linux-generic/include/odp_crypto_internal.h \
> +  ${srcdir}/include/odp_crypto_dpdk.h \
>
>
>
> Move one line up to follow alphabetical order.
>
>
> diff --git a/platform/linux-dpdk/include/odp_crypto_dpdk.h
> b/platform/linux-dpdk/include/odp_crypto_dpdk.h
> new file mode 100644
> index 0000000..559ac94
> --- /dev/null
> +++ b/platform/linux-dpdk/include/odp_crypto_dpdk.h
>
>
> Do you need a separate header for this? Couldn’t you use the
> linux-dpdk/include/odp/api/crypto.h, which is currently only a link to
> the matching file in linux-generic?
>

> diff --git a/platform/linux-dpdk/include/odp_packet_dpdk.h
> b/platform/linux-dpdk/include/odp_packet_dpdk.h
> index 495d5e6..c83089b 100644
> --- a/platform/linux-dpdk/include/odp_packet_dpdk.h
> +++ b/platform/linux-dpdk/include/odp_packet_dpdk.h
> @@ -43,6 +43,9 @@
> #include <rte_jhash.h>
> #include <rte_hash_crc.h>
>
> +#include <rte_crypto.h>
> +#include <rte_cryptodev.h>
> +
>
>
> Do you need to include the crypto headers here? Seems odd as there is no
> crypto code in odp_packet_dpdk.c.
>
> diff --git a/platform/linux-dpdk/odp_crypto_dpdk.c
> b/platform/linux-dpdk/odp_crypto_dpdk.c
> new file mode 100644
> index 0000000..edb72a8
> --- /dev/null
> +++ b/platform/linux-dpdk/odp_crypto_dpdk.c
>
>
>
> Related to the odp_crypto_dpdk.h comment above, could you use odp_crypto.c
> name here? You are replacing the whole file from linux-generic anyway.
>
Sure I will rename the file to odp_crypto.c.

>
> Also, checkpatch throws a lot of complaints about this file.
>
> +
> +typedef struct odp_crypto_global_s odp_crypto_global_t;
> +typedef struct odp_dpdk_session_entry_t odp_dpdk_session_entry;
> +struct odp_dpdk_session_entry_t {
> + struct odp_dpdk_session_entry_t *next;
> + uint64_t rte_session;
> + odp_bool_t do_cipher_first;
> + struct rte_crypto_sym_xform cipher_xform;
> + struct rte_crypto_sym_xform auth_xform;
> + struct {
> + uint8_t *data;
> + uint16_t length;
> + } iv;
> + odp_queue_t compl_queue;           /**< Async mode completion event
> queue */
> + odp_pool_t output_pool;            /**< Output buffer pool */
> +};
>
>
> The naming convention is not to use odp_ prefix with implementation
> internal data types.
>
> +
> +struct odp_crypto_global_s {
> + odp_spinlock_t                lock;
> + uint8_t enabled_crypto_devs;
> + uint8_t enabled_crypto_dev_ids[RTE_CRYPTO_MAX_DEVS];
> + odp_dpdk_session_entry *free;
> + odp_dpdk_session_entry sessions[0];
>
>
> You could probably use MAX_SESSIONS define here.
>
> +static
> +odp_crypto_generic_op_result_t *get_op_result_from_event(odp_event_t ev)
>
>
> Move keywords and return value types to the same line with function names.
>
> +int
> +odp_crypto_init_global(void)
> +{
> + size_t mem_size;
> + odp_shm_t shm;
> + int idx;
> +
> + /* Calculate the memory size we need */
> + mem_size  = sizeof(*global);
> + mem_size += (MAX_SESSIONS * sizeof(odp_dpdk_session_entry));
> +
> + /* Allocate our globally shared memory */
> + shm = odp_shm_reserve("crypto_pool", mem_size,
> +      ODP_CACHE_LINE_SIZE, 0);
> +
>
>
> Check return value.
>
>
> +int
> +odp_crypto_init_local(void)
> +{
>
>
> All application threads will call this functions, so it looks like the
> configure functions etc. will be called multiple times.
>
> +
> +int odp_crypto_term_global(void)
> +{
> + int rc = 0;
> + int ret;
> + int count = 0;
> + odp_dpdk_session_entry *session;
> +
> + for (session = global->free; session != NULL; session = session->next)
> + count++;
> + if (count != MAX_SESSIONS) {
> + ODP_ERR("crypto sessions still active\n");
> + rc = -1;
> + }
> +
> + ret = odp_shm_free(odp_shm_lookup("crypto_pool"));
>
>
> The shm API doesn’t define that names have to be unique, so a safer method
> would be to store
> the shm handle in global init and use it here directly.
>
>
> +int odp_crypto_term_local(void)
> +{
> + return 0;
> +}
>
>
> This is not an API function, so it is not needed.
>
>
> diff --git a/platform/linux-dpdk/odp_init.c b/platform/linux-dpdk/odp_
> init.c
> index d50c576..35398ad 100644
> --- a/platform/linux-dpdk/odp_init.c
> +++ b/platform/linux-dpdk/odp_init.c
> @@ -555,10 +555,17 @@ int odp_init_local(odp_instance_t instance,
> odp_thread_type_t thr_type)
> }
> stage = POOL_INIT;
>
> + if (odp_crypto_init_local()) {
> + ODP_ERR("ODP crypto init failed.\n");
> + return -1;
>
> + }
>
>
> Should probably use 'goto init_fail’ instead of return -1.
>
>
> @@ -606,6 +613,13 @@ int _odp_term_local(enum init_stage stage)
> }
> /* Fall through */
>
> + case CRYPTO_INIT:
> + if (odp_crypto_term_local()) {
> + ODP_ERR("ODP crypto term failed.\n");
> + rc = -1;
> + }
> + /* Fall through */
> +
> default:
> break;
> }
>
>
> To match the initialisation order in odp_init_local() this case should be
> above POOL_INIT.
>
>
> diff --git a/platform/linux-generic/include/odp_internal.h
> b/platform/linux-generic/include/odp_internal.h
> index 3429781..11e2c2f 100644
> --- a/platform/linux-generic/include/odp_internal.h
> +++ b/platform/linux-generic/include/odp_internal.h
> @@ -104,6 +104,8 @@ int odp_queue_term_global(void);
>
> int odp_crypto_init_global(void);
> int odp_crypto_term_global(void);
> +int odp_crypto_init_local(void);
> +int odp_crypto_term_local(void);
>
>
> Defining these functions under platform/linux-generic will create merge
> conflicts in the future. Move the definitions to platform/linux-dpdk.
>
> Secondly, to avoid confusion internal function names shouldn’t start with
> 'odp_’. The functions defined in odp_internal.h are old and should also be
> renamed (not in this commit).
>
>
>
Will fix all the above comments and send another version

/Krishna
Balakrishna Garapati Nov. 23, 2016, 9:07 a.m. | #2
sent v2.

/Krishna

On 21 November 2016 at 09:26, Krishna Garapati <
balakrishna.garapati@linaro.org> wrote:

>
>
> On 16 November 2016 at 10:11, Elo, Matias (Nokia - FI/Espoo) <
> matias.elo@nokia-bell-labs.com> wrote:
>
>> Hi Krishna,
>>
>> Some comments below.
>>
>> -Matias
>>
>>
>> diff --git a/platform/linux-dpdk/Makefile.am
>> b/platform/linux-dpdk/Makefile.am
>> index 488f2c0..6ce0ce6 100644
>> --- a/platform/linux-dpdk/Makefile.am
>> +++ b/platform/linux-dpdk/Makefile.am
>> @@ -121,6 +121,7 @@ noinst_HEADERS = \
>>  ${top_srcdir}/platform/linux-generic/include/odp_classification_inlines.h
>> \
>>  ${top_srcdir}/platform/linux-generic/include/odp_classification_internal.h
>> \
>>  ${top_srcdir}/platform/linux-generic/include/odp_crypto_internal.h \
>> +  ${srcdir}/include/odp_crypto_dpdk.h \
>>
>>
>>
>> Move one line up to follow alphabetical order.
>>
>>
>> diff --git a/platform/linux-dpdk/include/odp_crypto_dpdk.h
>> b/platform/linux-dpdk/include/odp_crypto_dpdk.h
>> new file mode 100644
>> index 0000000..559ac94
>> --- /dev/null
>> +++ b/platform/linux-dpdk/include/odp_crypto_dpdk.h
>>
>>
>> Do you need a separate header for this? Couldn’t you use the
>> linux-dpdk/include/odp/api/crypto.h, which is currently only a link to
>> the matching file in linux-generic?
>>
>
>> diff --git a/platform/linux-dpdk/include/odp_packet_dpdk.h
>> b/platform/linux-dpdk/include/odp_packet_dpdk.h
>> index 495d5e6..c83089b 100644
>> --- a/platform/linux-dpdk/include/odp_packet_dpdk.h
>> +++ b/platform/linux-dpdk/include/odp_packet_dpdk.h
>> @@ -43,6 +43,9 @@
>> #include <rte_jhash.h>
>> #include <rte_hash_crc.h>
>>
>> +#include <rte_crypto.h>
>> +#include <rte_cryptodev.h>
>> +
>>
>>
>> Do you need to include the crypto headers here? Seems odd as there is no
>> crypto code in odp_packet_dpdk.c.
>>
>> diff --git a/platform/linux-dpdk/odp_crypto_dpdk.c
>> b/platform/linux-dpdk/odp_crypto_dpdk.c
>> new file mode 100644
>> index 0000000..edb72a8
>> --- /dev/null
>> +++ b/platform/linux-dpdk/odp_crypto_dpdk.c
>>
>>
>>
>> Related to the odp_crypto_dpdk.h comment above, could you use
>> odp_crypto.c name here? You are replacing the whole file from linux-generic
>> anyway.
>>
> Sure I will rename the file to odp_crypto.c.
>
>>
>> Also, checkpatch throws a lot of complaints about this file.
>>
>> +
>> +typedef struct odp_crypto_global_s odp_crypto_global_t;
>> +typedef struct odp_dpdk_session_entry_t odp_dpdk_session_entry;
>> +struct odp_dpdk_session_entry_t {
>> + struct odp_dpdk_session_entry_t *next;
>> + uint64_t rte_session;
>> + odp_bool_t do_cipher_first;
>> + struct rte_crypto_sym_xform cipher_xform;
>> + struct rte_crypto_sym_xform auth_xform;
>> + struct {
>> + uint8_t *data;
>> + uint16_t length;
>> + } iv;
>> + odp_queue_t compl_queue;           /**< Async mode completion event
>> queue */
>> + odp_pool_t output_pool;            /**< Output buffer pool */
>> +};
>>
>>
>> The naming convention is not to use odp_ prefix with implementation
>> internal data types.
>>
>> +
>> +struct odp_crypto_global_s {
>> + odp_spinlock_t                lock;
>> + uint8_t enabled_crypto_devs;
>> + uint8_t enabled_crypto_dev_ids[RTE_CRYPTO_MAX_DEVS];
>> + odp_dpdk_session_entry *free;
>> + odp_dpdk_session_entry sessions[0];
>>
>>
>> You could probably use MAX_SESSIONS define here.
>>
>> +static
>> +odp_crypto_generic_op_result_t *get_op_result_from_event(odp_event_t ev)
>>
>>
>> Move keywords and return value types to the same line with function names.
>>
>> +int
>> +odp_crypto_init_global(void)
>> +{
>> + size_t mem_size;
>> + odp_shm_t shm;
>> + int idx;
>> +
>> + /* Calculate the memory size we need */
>> + mem_size  = sizeof(*global);
>> + mem_size += (MAX_SESSIONS * sizeof(odp_dpdk_session_entry));
>> +
>> + /* Allocate our globally shared memory */
>> + shm = odp_shm_reserve("crypto_pool", mem_size,
>> +      ODP_CACHE_LINE_SIZE, 0);
>> +
>>
>>
>> Check return value.
>>
>>
>> +int
>> +odp_crypto_init_local(void)
>> +{
>>
>>
>> All application threads will call this functions, so it looks like the
>> configure functions etc. will be called multiple times.
>>
>> +
>> +int odp_crypto_term_global(void)
>> +{
>> + int rc = 0;
>> + int ret;
>> + int count = 0;
>> + odp_dpdk_session_entry *session;
>> +
>> + for (session = global->free; session != NULL; session = session->next)
>> + count++;
>> + if (count != MAX_SESSIONS) {
>> + ODP_ERR("crypto sessions still active\n");
>> + rc = -1;
>> + }
>> +
>> + ret = odp_shm_free(odp_shm_lookup("crypto_pool"));
>>
>>
>> The shm API doesn’t define that names have to be unique, so a safer
>> method would be to store
>> the shm handle in global init and use it here directly.
>>
>>
>> +int odp_crypto_term_local(void)
>> +{
>> + return 0;
>> +}
>>
>>
>> This is not an API function, so it is not needed.
>>
>>
>> diff --git a/platform/linux-dpdk/odp_init.c
>> b/platform/linux-dpdk/odp_init.c
>> index d50c576..35398ad 100644
>> --- a/platform/linux-dpdk/odp_init.c
>> +++ b/platform/linux-dpdk/odp_init.c
>> @@ -555,10 +555,17 @@ int odp_init_local(odp_instance_t instance,
>> odp_thread_type_t thr_type)
>> }
>> stage = POOL_INIT;
>>
>> + if (odp_crypto_init_local()) {
>> + ODP_ERR("ODP crypto init failed.\n");
>> + return -1;
>>
>> + }
>>
>>
>> Should probably use 'goto init_fail’ instead of return -1.
>>
>>
>> @@ -606,6 +613,13 @@ int _odp_term_local(enum init_stage stage)
>> }
>> /* Fall through */
>>
>> + case CRYPTO_INIT:
>> + if (odp_crypto_term_local()) {
>> + ODP_ERR("ODP crypto term failed.\n");
>> + rc = -1;
>> + }
>> + /* Fall through */
>> +
>> default:
>> break;
>> }
>>
>>
>> To match the initialisation order in odp_init_local() this case should be
>> above POOL_INIT.
>>
>>
>> diff --git a/platform/linux-generic/include/odp_internal.h
>> b/platform/linux-generic/include/odp_internal.h
>> index 3429781..11e2c2f 100644
>> --- a/platform/linux-generic/include/odp_internal.h
>> +++ b/platform/linux-generic/include/odp_internal.h
>> @@ -104,6 +104,8 @@ int odp_queue_term_global(void);
>>
>> int odp_crypto_init_global(void);
>> int odp_crypto_term_global(void);
>> +int odp_crypto_init_local(void);
>> +int odp_crypto_term_local(void);
>>
>>
>> Defining these functions under platform/linux-generic will create merge
>> conflicts in the future. Move the definitions to platform/linux-dpdk.
>>
>> Secondly, to avoid confusion internal function names shouldn’t start with
>> 'odp_’. The functions defined in odp_internal.h are old and should also be
>> renamed (not in this commit).
>>
>>
>>
> Will fix all the above comments and send another version
>
> /Krishna
>
>
>

Patch hide | download patch | download mbox

diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux-dpdk/Makefile.am
index 488f2c0..6ce0ce6 100644
--- a/platform/linux-dpdk/Makefile.am
+++ b/platform/linux-dpdk/Makefile.am
@@ -121,6 +121,7 @@  noinst_HEADERS = \
 ${top_srcdir}/platform/linux-generic/include/odp_classification_inlines.h \
 ${top_srcdir}/platform/linux-generic/include/odp_classification_internal.h \
 ${top_srcdir}/platform/linux-generic/include/odp_crypto_internal.h \
+  ${srcdir}/include/odp_crypto_dpdk.h \


Move one line up to follow alphabetical order.


diff --git a/platform/linux-dpdk/include/odp_crypto_dpdk.h b/platform/linux-dpdk/include/odp_crypto_dpdk.h
new file mode 100644
index 0000000..559ac94
--- /dev/null
+++ b/platform/linux-dpdk/include/odp_crypto_dpdk.h

Do you need a separate header for this? Couldn’t you use the linux-dpdk/include/odp/api/crypto.h, which is currently only a link to the matching file in linux-generic?

diff --git a/platform/linux-dpdk/include/odp_packet_dpdk.h b/platform/linux-dpdk/include/odp_packet_dpdk.h
index 495d5e6..c83089b 100644
--- a/platform/linux-dpdk/include/odp_packet_dpdk.h
+++ b/platform/linux-dpdk/include/odp_packet_dpdk.h
@@ -43,6 +43,9 @@ 
#include <rte_jhash.h>
#include <rte_hash_crc.h>

+#include <rte_crypto.h>
+#include <rte_cryptodev.h>
+

Do you need to include the crypto headers here? Seems odd as there is no crypto code in odp_packet_dpdk.c.

diff --git a/platform/linux-dpdk/odp_crypto_dpdk.c b/platform/linux-dpdk/odp_crypto_dpdk.c
new file mode 100644
index 0000000..edb72a8
--- /dev/null
+++ b/platform/linux-dpdk/odp_crypto_dpdk.c


Related to the odp_crypto_dpdk.h comment above, could you use odp_crypto.c name here? You are replacing the whole file from linux-generic anyway.

Also, checkpatch throws a lot of complaints about this file.

+
+typedef struct odp_crypto_global_s odp_crypto_global_t;
+typedef struct odp_dpdk_session_entry_t odp_dpdk_session_entry;
+struct odp_dpdk_session_entry_t {
+ struct odp_dpdk_session_entry_t *next;
+ uint64_t rte_session;
+ odp_bool_t do_cipher_first;
+ struct rte_crypto_sym_xform cipher_xform;
+ struct rte_crypto_sym_xform auth_xform;
+ struct {
+ uint8_t *data;
+ uint16_t length;
+ } iv;
+ odp_queue_t compl_queue;           /**< Async mode completion event queue */
+ odp_pool_t output_pool;            /**< Output buffer pool */
+};

The naming convention is not to use odp_ prefix with implementation internal data types.

+
+struct odp_crypto_global_s {
+ odp_spinlock_t                lock;
+ uint8_t enabled_crypto_devs;
+ uint8_t enabled_crypto_dev_ids[RTE_CRYPTO_MAX_DEVS];
+ odp_dpdk_session_entry *free;
+ odp_dpdk_session_entry sessions[0];

You could probably use MAX_SESSIONS define here.

+static
+odp_crypto_generic_op_result_t *get_op_result_from_event(odp_event_t ev)

Move keywords and return value types to the same line with function names.

+int
+odp_crypto_init_global(void)
+{
+ size_t mem_size;
+ odp_shm_t shm;
+ int idx;
+
+ /* Calculate the memory size we need */
+ mem_size  = sizeof(*global);
+ mem_size += (MAX_SESSIONS * sizeof(odp_dpdk_session_entry));
+
+ /* Allocate our globally shared memory */
+ shm = odp_shm_reserve("crypto_pool", mem_size,
+      ODP_CACHE_LINE_SIZE, 0);
+

Check return value.


+int
+odp_crypto_init_local(void)
+{

All application threads will call this functions, so it looks like the configure functions etc. will be called multiple times.

+
+int odp_crypto_term_global(void)
+{
+ int rc = 0;
+ int ret;
+ int count = 0;
+ odp_dpdk_session_entry *session;
+
+ for (session = global->free; session != NULL; session = session->next)
+ count++;
+ if (count != MAX_SESSIONS) {
+ ODP_ERR("crypto sessions still active\n");
+ rc = -1;
+ }
+
+ ret = odp_shm_free(odp_shm_lookup("crypto_pool"));

The shm API doesn’t define that names have to be unique, so a safer method would be to store
the shm handle in global init and use it here directly.


+int odp_crypto_term_local(void)
+{
+ return 0;
+}

This is not an API function, so it is not needed.


diff --git a/platform/linux-dpdk/odp_init.c b/platform/linux-dpdk/odp_init.c
index d50c576..35398ad 100644
--- a/platform/linux-dpdk/odp_init.c
+++ b/platform/linux-dpdk/odp_init.c
@@ -555,10 +555,17 @@  int odp_init_local(odp_instance_t instance, odp_thread_type_t thr_type)
}
stage = POOL_INIT;

+ if (odp_crypto_init_local()) {
+ ODP_ERR("ODP crypto init failed.\n");
+ return -1;
+ }

Should probably use 'goto init_fail’ instead of return -1.


@@ -606,6 +613,13 @@  int _odp_term_local(enum init_stage stage)
}
/* Fall through */

+ case CRYPTO_INIT:
+ if (odp_crypto_term_local()) {
+ ODP_ERR("ODP crypto term failed.\n");
+ rc = -1;
+ }
+ /* Fall through */
+
default:
break;
}

To match the initialisation order in odp_init_local() this case should be above POOL_INIT.


diff --git a/platform/linux-generic/include/odp_internal.h b/platform/linux-generic/include/odp_internal.h
index 3429781..11e2c2f 100644
--- a/platform/linux-generic/include/odp_internal.h
+++ b/platform/linux-generic/include/odp_internal.h
@@ -104,6 +104,8 @@  int odp_queue_term_global(void);

int odp_crypto_init_global(void);
int odp_crypto_term_global(void);
+int odp_crypto_init_local(void);
+int odp_crypto_term_local(void);

Defining these functions under platform/linux-generic will create merge conflicts in the future. Move the definitions to platform/linux-dpdk.