RE: [PATCH v2 04/22] pdcp: add packet group

[Anoob Joseph <anoobj@marvell.com>]

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