[v2,04/22] pdcp: add packet group
Checks
Commit Message
Crypto processing in PDCP is performed asynchronously by
rte_cryptodev_enqueue_burst() and rte_cryptodev_dequeue_burst(). Since
cryptodev dequeue can return crypto operations belonging to multiple
entities, rte_pdcp_pkt_crypto_group() is added to help grouping crypto
operations belonging to same entity.
Signed-off-by: Anoob Joseph <anoobj@marvell.com>
Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
---
lib/pdcp/meson.build | 1 +
lib/pdcp/rte_pdcp.h | 2 +
lib/pdcp/rte_pdcp_group.h | 125 ++++++++++++++++++++++++++++++++++++++
3 files changed, 128 insertions(+)
create mode 100644 lib/pdcp/rte_pdcp_group.h
Comments
> Subject: [PATCH v2 04/22] pdcp: add packet group
>
> Crypto processing in PDCP is performed asynchronously by
> rte_cryptodev_enqueue_burst() and rte_cryptodev_dequeue_burst(). Since
> cryptodev dequeue can return crypto operations belonging to multiple
> entities, rte_pdcp_pkt_crypto_group() is added to help grouping crypto
> operations belonging to same entity.
>
> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
> Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
> ---
> lib/pdcp/meson.build | 1 +
> lib/pdcp/rte_pdcp.h | 2 +
> lib/pdcp/rte_pdcp_group.h | 125
> ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 128 insertions(+)
> create mode 100644 lib/pdcp/rte_pdcp_group.h
>
> diff --git a/lib/pdcp/meson.build b/lib/pdcp/meson.build
> index ccaf426240..08679b743a 100644
> --- a/lib/pdcp/meson.build
> +++ b/lib/pdcp/meson.build
> @@ -13,5 +13,6 @@ sources = files(
> 'rte_pdcp.c',
> )
> headers = files('rte_pdcp.h')
> +indirect_headers += files('rte_pdcp_group.h')
>
> deps += ['mbuf', 'net', 'cryptodev', 'security']
> diff --git a/lib/pdcp/rte_pdcp.h b/lib/pdcp/rte_pdcp.h
> index 75dc569f66..54f88e3fd3 100644
> --- a/lib/pdcp/rte_pdcp.h
> +++ b/lib/pdcp/rte_pdcp.h
> @@ -247,6 +247,8 @@ rte_pdcp_pkt_post_process(const struct
> rte_pdcp_entity *entity,
> return entity->post_process(entity, in_mb, out_mb, num, nb_err);
> }
>
> +#include <rte_pdcp_group.h>
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/lib/pdcp/rte_pdcp_group.h b/lib/pdcp/rte_pdcp_group.h
> new file mode 100644
> index 0000000000..cb322f55c7
> --- /dev/null
> +++ b/lib/pdcp/rte_pdcp_group.h
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2023 Marvell.
> + */
> +
> +#ifndef RTE_PDCP_GROUP_H
> +#define RTE_PDCP_GROUP_H
> +
> +/**
> + * @file rte_pdcp_group.h
> + *
> + * RTE PDCP grouping support.
> + * It is not recommended to include this file directly, include <rte_pdcp.h>
> + * instead.
> + * Provides helper functions to process completed crypto-ops and group
> related
> + * packets by sessions they belong to.
> + */
Why do we need to have a separate public header file which we do not wish user to use directly
for just a single API?
Can it not be added into rte_pdcp.h?
> +
> +#include <rte_common.h>
> +#include <rte_crypto.h>
> +#include <rte_cryptodev.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/**
> + * Group packets belonging to same PDCP entity.
> + */
> +struct rte_pdcp_group {
> + union {
> + uint64_t val;
> + void *ptr;
> + } id; /**< Grouped by value */
> + struct rte_mbuf **m; /**< Start of the group */
> + uint32_t cnt; /**< Number of entries in the group */
> + int32_t rc; /**< Status code associated with the group */
> +};
> +
> +/**
> + * Take crypto-op as an input and extract pointer to related PDCP entity.
> + * @param cop
> + * The address of an input *rte_crypto_op* structure.
> + * @return
> + * The pointer to the related *rte_pdcp_entity* structure.
> + */
> +static inline struct rte_pdcp_entity *
> +rte_pdcp_en_from_cop(const struct rte_crypto_op *cop)
> +{
> + void *sess = cop->sym[0].session;
> +
> + return (struct rte_pdcp_entity *)
> + rte_cryptodev_sym_session_opaque_data_get(sess);
> +}
> +
> +/**
> + * Take as input completed crypto ops, extract related mbufs and group them
> by
> + * *rte_pdcp_entity* they belong to. Mbuf for which the crypto operation has
> + * failed would be flagged using *RTE_MBUF_F_RX_SEC_OFFLOAD_FAILED*
> flag
> + * in rte_mbuf.ol_flags. The crypto_ops would be freed after the grouping.
> + *
> + * Note that application must ensure only crypto-ops prepared by lib_pdcp is
> + * provided back to @see rte_pdcp_pkt_crypto_group().
> + *
> + * @param cop
> + * The address of an array of *num* pointers to the input *rte_crypto_op*
> + * structures.
> + * @param[out] mb
> + * The address of an array of *num* pointers to output *rte_mbuf* structures.
> + * @param[out] grp
> + * The address of an array of *num* to output *rte_pdcp_group* structures.
> + * @param num
> + * The maximum number of crypto-ops to process.
> + * @return
> + * Number of filled elements in *grp* array.
> + *
> + */
> +static inline uint16_t
> +rte_pdcp_pkt_crypto_group(struct rte_crypto_op *cop[], struct rte_mbuf
> *mb[],
> + struct rte_pdcp_group grp[], uint16_t num)
> +{
> + uint32_t i, j = 0, n = 0;
> + void *ns, *ps = NULL;
> + struct rte_mbuf *m;
> +
> + for (i = 0; i != num; i++) {
> + m = cop[i]->sym[0].m_src;
> + ns = cop[i]->sym[0].session;
> +
> + m->ol_flags |= RTE_MBUF_F_RX_SEC_OFFLOAD;
> + if (cop[i]->status != RTE_CRYPTO_OP_STATUS_SUCCESS)
> + m->ol_flags |=
> RTE_MBUF_F_RX_SEC_OFFLOAD_FAILED;
> +
> + /* Different entity */
> + if (ps != ns) {
> +
> + /* Finalize open group and start a new one */
> + if (ps != NULL) {
> + grp[n].cnt = mb + j - grp[n].m;
> + n++;
> + }
> +
> + /* Start new group */
> + grp[n].m = mb + j;
> + ps = ns;
> + grp[n].id.ptr = rte_pdcp_en_from_cop(cop[i]);
> + }
> +
> + mb[j++] = m;
> + rte_crypto_op_free(cop[i]);
> + }
> +
> + /* Finalize last group */
> + if (ps != NULL) {
> + grp[n].cnt = mb + j - grp[n].m;
> + n++;
> + }
> +
> + return n;
> +}
These APIs are being called from application directly (as per the cover letter).
Should be marked as experimental and also add them in version.map
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* RTE_PDCP_GROUP_H */
> --
> 2.25.1
Hi Akhil, Konstantin,
Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Tuesday, May 16, 2023 9:27 PM
> To: Anoob Joseph <anoobj@marvell.com>; Thomas Monjalon
> <thomas@monjalon.net>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>; Bernard
> Iremonger <bernard.iremonger@intel.com>
> Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; Mattias Rönnblom
> <mattias.ronnblom@ericsson.com>; Kiran Kumar Kokkilagadda
> <kirankumark@marvell.com>; Volodymyr Fialko <vfialko@marvell.com>;
> dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>
> Subject: RE: [PATCH v2 04/22] pdcp: add packet group
>
> > Subject: [PATCH v2 04/22] pdcp: add packet group
> >
> > Crypto processing in PDCP is performed asynchronously by
> > rte_cryptodev_enqueue_burst() and rte_cryptodev_dequeue_burst().
> Since
> > cryptodev dequeue can return crypto operations belonging to multiple
> > entities, rte_pdcp_pkt_crypto_group() is added to help grouping crypto
> > operations belonging to same entity.
> >
> > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
> > Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
> > ---
> > lib/pdcp/meson.build | 1 +
> > lib/pdcp/rte_pdcp.h | 2 +
> > lib/pdcp/rte_pdcp_group.h | 125
> > ++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 128 insertions(+)
> > create mode 100644 lib/pdcp/rte_pdcp_group.h
> >
> > diff --git a/lib/pdcp/meson.build b/lib/pdcp/meson.build index
> > ccaf426240..08679b743a 100644
> > --- a/lib/pdcp/meson.build
> > +++ b/lib/pdcp/meson.build
> > @@ -13,5 +13,6 @@ sources = files(
> > 'rte_pdcp.c',
> > )
> > headers = files('rte_pdcp.h')
> > +indirect_headers += files('rte_pdcp_group.h')
> >
> > deps += ['mbuf', 'net', 'cryptodev', 'security'] diff --git
> > a/lib/pdcp/rte_pdcp.h b/lib/pdcp/rte_pdcp.h index
> > 75dc569f66..54f88e3fd3 100644
> > --- a/lib/pdcp/rte_pdcp.h
> > +++ b/lib/pdcp/rte_pdcp.h
> > @@ -247,6 +247,8 @@ rte_pdcp_pkt_post_process(const struct
> > rte_pdcp_entity *entity,
> > return entity->post_process(entity, in_mb, out_mb, num, nb_err); }
> >
> > +#include <rte_pdcp_group.h>
> > +
> > #ifdef __cplusplus
> > }
> > #endif
> > diff --git a/lib/pdcp/rte_pdcp_group.h b/lib/pdcp/rte_pdcp_group.h new
> > file mode 100644 index 0000000000..cb322f55c7
> > --- /dev/null
> > +++ b/lib/pdcp/rte_pdcp_group.h
> > @@ -0,0 +1,125 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2023 Marvell.
> > + */
> > +
> > +#ifndef RTE_PDCP_GROUP_H
> > +#define RTE_PDCP_GROUP_H
> > +
> > +/**
> > + * @file rte_pdcp_group.h
> > + *
> > + * RTE PDCP grouping support.
> > + * It is not recommended to include this file directly, include
> > +<rte_pdcp.h>
> > + * instead.
> > + * Provides helper functions to process completed crypto-ops and
> > +group
> > related
> > + * packets by sessions they belong to.
> > + */
>
> Why do we need to have a separate public header file which we do not wish
> user to use directly for just a single API?
>
> Can it not be added into rte_pdcp.h?
[Anoob] This follows how the code organization is done in lib IPsec. My understanding is, it is done for 2 reasons,
1. Separate out helper inline functions into a separate header
2. Main header focus on core design of the protocol interface, ie, structs, macros & APIs.
Also, lib_pdcp.h is bound to grow. So rather than splitting it later, why don't we start with better organized code layout which is already followed in similar use case, ie, lib IPsec.
@Konstantin, did you have any other thoughts when you made the split this way?
>
> > +
> > +#include <rte_common.h>
> > +#include <rte_crypto.h>
> > +#include <rte_cryptodev.h>
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +/**
> > + * Group packets belonging to same PDCP entity.
> > + */
> > +struct rte_pdcp_group {
> > + union {
> > + uint64_t val;
> > + void *ptr;
> > + } id; /**< Grouped by value */
> > + struct rte_mbuf **m; /**< Start of the group */
> > + uint32_t cnt; /**< Number of entries in the group */
> > + int32_t rc; /**< Status code associated with the group */
> > +};
> > +
> > +/**
> > + * Take crypto-op as an input and extract pointer to related PDCP entity.
> > + * @param cop
> > + * The address of an input *rte_crypto_op* structure.
> > + * @return
> > + * The pointer to the related *rte_pdcp_entity* structure.
> > + */
> > +static inline struct rte_pdcp_entity * rte_pdcp_en_from_cop(const
> > +struct rte_crypto_op *cop) {
> > + void *sess = cop->sym[0].session;
> > +
> > + return (struct rte_pdcp_entity *)
> > + rte_cryptodev_sym_session_opaque_data_get(sess);
> > +}
> > +
> > +/**
> > + * Take as input completed crypto ops, extract related mbufs and
> > +group them
> > by
> > + * *rte_pdcp_entity* they belong to. Mbuf for which the crypto
> > + operation has
> > + * failed would be flagged using
> *RTE_MBUF_F_RX_SEC_OFFLOAD_FAILED*
> > flag
> > + * in rte_mbuf.ol_flags. The crypto_ops would be freed after the
> grouping.
> > + *
> > + * Note that application must ensure only crypto-ops prepared by
> > +lib_pdcp is
> > + * provided back to @see rte_pdcp_pkt_crypto_group().
> > + *
> > + * @param cop
> > + * The address of an array of *num* pointers to the input
> *rte_crypto_op*
> > + * structures.
> > + * @param[out] mb
> > + * The address of an array of *num* pointers to output *rte_mbuf*
> structures.
> > + * @param[out] grp
> > + * The address of an array of *num* to output *rte_pdcp_group*
> structures.
> > + * @param num
> > + * The maximum number of crypto-ops to process.
> > + * @return
> > + * Number of filled elements in *grp* array.
> > + *
> > + */
> > +static inline uint16_t
> > +rte_pdcp_pkt_crypto_group(struct rte_crypto_op *cop[], struct
> > +rte_mbuf
> > *mb[],
> > + struct rte_pdcp_group grp[], uint16_t num) {
> > + uint32_t i, j = 0, n = 0;
> > + void *ns, *ps = NULL;
> > + struct rte_mbuf *m;
> > +
> > + for (i = 0; i != num; i++) {
> > + m = cop[i]->sym[0].m_src;
> > + ns = cop[i]->sym[0].session;
> > +
> > + m->ol_flags |= RTE_MBUF_F_RX_SEC_OFFLOAD;
> > + if (cop[i]->status != RTE_CRYPTO_OP_STATUS_SUCCESS)
> > + m->ol_flags |=
> > RTE_MBUF_F_RX_SEC_OFFLOAD_FAILED;
> > +
> > + /* Different entity */
> > + if (ps != ns) {
> > +
> > + /* Finalize open group and start a new one */
> > + if (ps != NULL) {
> > + grp[n].cnt = mb + j - grp[n].m;
> > + n++;
> > + }
> > +
> > + /* Start new group */
> > + grp[n].m = mb + j;
> > + ps = ns;
> > + grp[n].id.ptr = rte_pdcp_en_from_cop(cop[i]);
> > + }
> > +
> > + mb[j++] = m;
> > + rte_crypto_op_free(cop[i]);
> > + }
> > +
> > + /* Finalize last group */
> > + if (ps != NULL) {
> > + grp[n].cnt = mb + j - grp[n].m;
> > + n++;
> > + }
> > +
> > + return n;
> > +}
>
> These APIs are being called from application directly (as per the cover letter).
> Should be marked as experimental and also add them in version.map
[Anoob] Agreed. Will do.
>
>
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* RTE_PDCP_GROUP_H */
> > --
> > 2.25.1
@@ -13,5 +13,6 @@ sources = files(
'rte_pdcp.c',
)
headers = files('rte_pdcp.h')
+indirect_headers += files('rte_pdcp_group.h')
deps += ['mbuf', 'net', 'cryptodev', 'security']
@@ -247,6 +247,8 @@ rte_pdcp_pkt_post_process(const struct rte_pdcp_entity *entity,
return entity->post_process(entity, in_mb, out_mb, num, nb_err);
}
+#include <rte_pdcp_group.h>
+
#ifdef __cplusplus
}
#endif
new file mode 100644
@@ -0,0 +1,125 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(C) 2023 Marvell.
+ */
+
+#ifndef RTE_PDCP_GROUP_H
+#define RTE_PDCP_GROUP_H
+
+/**
+ * @file rte_pdcp_group.h
+ *
+ * RTE PDCP grouping support.
+ * It is not recommended to include this file directly, include <rte_pdcp.h>
+ * instead.
+ * Provides helper functions to process completed crypto-ops and group related
+ * packets by sessions they belong to.
+ */
+
+#include <rte_common.h>
+#include <rte_crypto.h>
+#include <rte_cryptodev.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * Group packets belonging to same PDCP entity.
+ */
+struct rte_pdcp_group {
+ union {
+ uint64_t val;
+ void *ptr;
+ } id; /**< Grouped by value */
+ struct rte_mbuf **m; /**< Start of the group */
+ uint32_t cnt; /**< Number of entries in the group */
+ int32_t rc; /**< Status code associated with the group */
+};
+
+/**
+ * Take crypto-op as an input and extract pointer to related PDCP entity.
+ * @param cop
+ * The address of an input *rte_crypto_op* structure.
+ * @return
+ * The pointer to the related *rte_pdcp_entity* structure.
+ */
+static inline struct rte_pdcp_entity *
+rte_pdcp_en_from_cop(const struct rte_crypto_op *cop)
+{
+ void *sess = cop->sym[0].session;
+
+ return (struct rte_pdcp_entity *)
+ rte_cryptodev_sym_session_opaque_data_get(sess);
+}
+
+/**
+ * Take as input completed crypto ops, extract related mbufs and group them by
+ * *rte_pdcp_entity* they belong to. Mbuf for which the crypto operation has
+ * failed would be flagged using *RTE_MBUF_F_RX_SEC_OFFLOAD_FAILED* flag
+ * in rte_mbuf.ol_flags. The crypto_ops would be freed after the grouping.
+ *
+ * Note that application must ensure only crypto-ops prepared by lib_pdcp is
+ * provided back to @see rte_pdcp_pkt_crypto_group().
+ *
+ * @param cop
+ * The address of an array of *num* pointers to the input *rte_crypto_op*
+ * structures.
+ * @param[out] mb
+ * The address of an array of *num* pointers to output *rte_mbuf* structures.
+ * @param[out] grp
+ * The address of an array of *num* to output *rte_pdcp_group* structures.
+ * @param num
+ * The maximum number of crypto-ops to process.
+ * @return
+ * Number of filled elements in *grp* array.
+ *
+ */
+static inline uint16_t
+rte_pdcp_pkt_crypto_group(struct rte_crypto_op *cop[], struct rte_mbuf *mb[],
+ struct rte_pdcp_group grp[], uint16_t num)
+{
+ uint32_t i, j = 0, n = 0;
+ void *ns, *ps = NULL;
+ struct rte_mbuf *m;
+
+ for (i = 0; i != num; i++) {
+ m = cop[i]->sym[0].m_src;
+ ns = cop[i]->sym[0].session;
+
+ m->ol_flags |= RTE_MBUF_F_RX_SEC_OFFLOAD;
+ if (cop[i]->status != RTE_CRYPTO_OP_STATUS_SUCCESS)
+ m->ol_flags |= RTE_MBUF_F_RX_SEC_OFFLOAD_FAILED;
+
+ /* Different entity */
+ if (ps != ns) {
+
+ /* Finalize open group and start a new one */
+ if (ps != NULL) {
+ grp[n].cnt = mb + j - grp[n].m;
+ n++;
+ }
+
+ /* Start new group */
+ grp[n].m = mb + j;
+ ps = ns;
+ grp[n].id.ptr = rte_pdcp_en_from_cop(cop[i]);
+ }
+
+ mb[j++] = m;
+ rte_crypto_op_free(cop[i]);
+ }
+
+ /* Finalize last group */
+ if (ps != NULL) {
+ grp[n].cnt = mb + j - grp[n].m;
+ n++;
+ }
+
+ return n;
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* RTE_PDCP_GROUP_H */