Hi Akhil, Konstantin,
Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Akhil Goyal <gak...@marvell.com>
> Sent: Tuesday, May 16, 2023 9:27 PM
> To: Anoob Joseph <ano...@marvell.com>; Thomas Monjalon
> <tho...@monjalon.net>; Jerin Jacob Kollanukkaran <jer...@marvell.com>;
> Konstantin Ananyev <konstantin.v.anan...@yandex.ru>; Bernard
> Iremonger <bernard.iremon...@intel.com>
> Cc: Hemant Agrawal <hemant.agra...@nxp.com>; Mattias Rönnblom
> <mattias.ronnb...@ericsson.com>; Kiran Kumar Kokkilagadda
> <kirankum...@marvell.com>; Volodymyr Fialko <vfia...@marvell.com>;
> dev@dpdk.org; Olivier Matz <olivier.m...@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 <ano...@marvell.com>
> > Signed-off-by: Kiran Kumar K <kirankum...@marvell.com>
> > Signed-off-by: Volodymyr Fialko <vfia...@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
