Re: [dpdk-dev] [PATCH v2 11/12] examples/ipsec-secgw: add app mode worker

[Anoob Joseph <anoobj@marvell.com>]

Hi Konstantin,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Wednesday, January 29, 2020 9:05 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal <akhil.goyal@nxp.com>;
> Nicolau, Radu <radu.nicolau@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: Lukas Bartosik <lbartosik@marvell.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Narayana Prasad Raju Athreya
> <pathreya@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>; Archana
> Muniganti <marchana@marvell.com>; Tejasree Kondoj
> <ktejasree@marvell.com>; Vamsi Krishna Attunuru <vattunuru@marvell.com>;
> dev@dpdk.org
> Subject: [EXT] RE: [PATCH v2 11/12] examples/ipsec-secgw: add app mode
> worker
> 
> External Email
> 
> ----------------------------------------------------------------------
> > Add application inbound/outbound worker thread and IPsec application
> > processing code for event mode.
> >
> > Exampple ipsec-secgw command in app mode:
> > ipsec-secgw -w 0002:02:00.0,ipsec_in_max_spi=128 -w
> > 0002:03:00.0,ipsec_in_max_spi=128 -w 0002:0e:00.0 -w 0002:10:00.1
> > --log-level=8 -c 0x1 -- -P -p 0x3 -u 0x1 --config "(1,0,0),(0,0,0)"
> > -f aes-gcm.cfg --transfer-mode event --schedule-type parallel
> >
> > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> > Signed-off-by: Lukasz Bartosik <lbartosik@marvell.com>
> > ---
> >  examples/ipsec-secgw/ipsec-secgw.c  |  45 +---
> > examples/ipsec-secgw/ipsec-secgw.h  |  69 ++++++
> >  examples/ipsec-secgw/ipsec.h        |  22 --
> >  examples/ipsec-secgw/ipsec_worker.c | 418
> > +++++++++++++++++++++++++++++++++++-
> >  examples/ipsec-secgw/ipsec_worker.h |  39 ++++
> >  5 files changed, 533 insertions(+), 60 deletions(-)  create mode
> > 100644 examples/ipsec-secgw/ipsec_worker.h
> >
> > diff --git a/examples/ipsec-secgw/ipsec-secgw.c
> > b/examples/ipsec-secgw/ipsec-secgw.c
> > index 86215fb..7d844bb 100644
> > --- a/examples/ipsec-secgw/ipsec-secgw.c
> > +++ b/examples/ipsec-secgw/ipsec-secgw.c
> > @@ -50,12 +50,11 @@
> >
> >  #include "event_helper.h"
> >  #include "ipsec.h"
> > +#include "ipsec_worker.h"
> >  #include "parser.h"
> >
> >  volatile bool force_quit;
> >
> > -#define RTE_LOGTYPE_IPSEC RTE_LOGTYPE_USER1
> > -
> >  #define MAX_JUMBO_PKT_LEN  9600
> >
> >  #define MEMPOOL_CACHE_SIZE 256
> > @@ -85,29 +84,6 @@ volatile bool force_quit;  static uint16_t nb_rxd =
> > IPSEC_SECGW_RX_DESC_DEFAULT;  static uint16_t nb_txd =
> > IPSEC_SECGW_TX_DESC_DEFAULT;
> >
> > -#if RTE_BYTE_ORDER != RTE_LITTLE_ENDIAN -#define
> __BYTES_TO_UINT64(a,
> > b, c, d, e, f, g, h) \
> > -	(((uint64_t)((a) & 0xff) << 56) | \
> > -	((uint64_t)((b) & 0xff) << 48) | \
> > -	((uint64_t)((c) & 0xff) << 40) | \
> > -	((uint64_t)((d) & 0xff) << 32) | \
> > -	((uint64_t)((e) & 0xff) << 24) | \
> > -	((uint64_t)((f) & 0xff) << 16) | \
> > -	((uint64_t)((g) & 0xff) << 8)  | \
> > -	((uint64_t)(h) & 0xff))
> > -#else
> > -#define __BYTES_TO_UINT64(a, b, c, d, e, f, g, h) \
> > -	(((uint64_t)((h) & 0xff) << 56) | \
> > -	((uint64_t)((g) & 0xff) << 48) | \
> > -	((uint64_t)((f) & 0xff) << 40) | \
> > -	((uint64_t)((e) & 0xff) << 32) | \
> > -	((uint64_t)((d) & 0xff) << 24) | \
> > -	((uint64_t)((c) & 0xff) << 16) | \
> > -	((uint64_t)((b) & 0xff) << 8) | \
> > -	((uint64_t)(a) & 0xff))
> > -#endif
> > -#define ETHADDR(a, b, c, d, e, f) (__BYTES_TO_UINT64(a, b, c, d, e,
> > f, 0, 0))
> > -
> >  #define ETHADDR_TO_UINT64(addr) __BYTES_TO_UINT64( \
> >  		(addr)->addr_bytes[0], (addr)->addr_bytes[1], \
> >  		(addr)->addr_bytes[2], (addr)->addr_bytes[3], \ @@ -119,18
> +95,6 @@
> > static uint16_t nb_txd = IPSEC_SECGW_TX_DESC_DEFAULT;
> >
> >  #define MTU_TO_FRAMELEN(x)	((x) + RTE_ETHER_HDR_LEN +
> RTE_ETHER_CRC_LEN)
> >
> > -/* port/source ethernet addr and destination ethernet addr */ -struct
> > ethaddr_info {
> > -	uint64_t src, dst;
> > -};
> > -
> > -struct ethaddr_info ethaddr_tbl[RTE_MAX_ETHPORTS] = {
> > -	{ 0, ETHADDR(0x00, 0x16, 0x3e, 0x7e, 0x94, 0x9a) },
> > -	{ 0, ETHADDR(0x00, 0x16, 0x3e, 0x22, 0xa1, 0xd9) },
> > -	{ 0, ETHADDR(0x00, 0x16, 0x3e, 0x08, 0x69, 0x26) },
> > -	{ 0, ETHADDR(0x00, 0x16, 0x3e, 0x49, 0x9e, 0xdd) }
> > -};
> > -
> >  struct flow_info flow_info_tbl[RTE_MAX_ETHPORTS];
> >
> >  #define CMD_LINE_OPT_CONFIG		"config"
> > @@ -183,6 +147,13 @@ static const struct option lgopts[] = {
> >  	{NULL, 0, 0, 0}
> >  };
> >
> > +struct ethaddr_info ethaddr_tbl[RTE_MAX_ETHPORTS] = {
> > +	{ 0, ETHADDR(0x00, 0x16, 0x3e, 0x7e, 0x94, 0x9a) },
> > +	{ 0, ETHADDR(0x00, 0x16, 0x3e, 0x22, 0xa1, 0xd9) },
> > +	{ 0, ETHADDR(0x00, 0x16, 0x3e, 0x08, 0x69, 0x26) },
> > +	{ 0, ETHADDR(0x00, 0x16, 0x3e, 0x49, 0x9e, 0xdd) } };
> > +
> >  /* mask of enabled ports */
> >  static uint32_t enabled_port_mask;
> >  static uint64_t enabled_cryptodev_mask = UINT64_MAX; diff --git
> > a/examples/ipsec-secgw/ipsec-secgw.h
> > b/examples/ipsec-secgw/ipsec-secgw.h
> > index 5b19e29..926ce5d 100644
> > --- a/examples/ipsec-secgw/ipsec-secgw.h
> > +++ b/examples/ipsec-secgw/ipsec-secgw.h
> > @@ -4,10 +4,79 @@
> >  #ifndef _IPSEC_SECGW_H_
> >  #define _IPSEC_SECGW_H_
> >
> > +#include <rte_hash.h>
> > +
> > +#define NB_SOCKETS 4
> > +
> > +#define MAX_PKT_BURST 32
> > +
> > +#define RTE_LOGTYPE_IPSEC RTE_LOGTYPE_USER1
> > +
> >  #define NB_SOCKETS 4
> 
> Duplicate, NB_SOCKETS already defined, see above.

[Anoob] Good catch. Will fix in v3.
 
> 
> >
> >  #define UNPROTECTED_PORT(portid) (unprotected_port_mask & (1 <<
> > portid))
> 
> As you are moving it anyway probably a good time to put portid param in (), or
> even make it a static inline function.

[Anoob] I would prefer a static inline function. Shall I make this change in v3?
 
> 
> >
> > +#if RTE_BYTE_ORDER != RTE_LITTLE_ENDIAN #define
> __BYTES_TO_UINT64(a,
> > +b, c, d, e, f, g, h) \
> > +	(((uint64_t)((a) & 0xff) << 56) | \
> > +	((uint64_t)((b) & 0xff) << 48) | \
> > +	((uint64_t)((c) & 0xff) << 40) | \
> > +	((uint64_t)((d) & 0xff) << 32) | \
> > +	((uint64_t)((e) & 0xff) << 24) | \
> > +	((uint64_t)((f) & 0xff) << 16) | \
> > +	((uint64_t)((g) & 0xff) << 8)  | \
> > +	((uint64_t)(h) & 0xff))
> > +#else
> > +#define __BYTES_TO_UINT64(a, b, c, d, e, f, g, h) \
> > +	(((uint64_t)((h) & 0xff) << 56) | \
> > +	((uint64_t)((g) & 0xff) << 48) | \
> > +	((uint64_t)((f) & 0xff) << 40) | \
> > +	((uint64_t)((e) & 0xff) << 32) | \
> > +	((uint64_t)((d) & 0xff) << 24) | \
> > +	((uint64_t)((c) & 0xff) << 16) | \
> > +	((uint64_t)((b) & 0xff) << 8) | \
> > +	((uint64_t)(a) & 0xff))
> > +#endif
> > +
> > +#define ETHADDR(a, b, c, d, e, f) (__BYTES_TO_UINT64(a, b, c, d, e,
> > +f, 0, 0))
> > +
> > +struct traffic_type {
> > +	const uint8_t *data[MAX_PKT_BURST * 2];
> > +	struct rte_mbuf *pkts[MAX_PKT_BURST * 2];
> > +	void *saptr[MAX_PKT_BURST * 2];
> > +	uint32_t res[MAX_PKT_BURST * 2];
> > +	uint32_t num;
> > +};
> > +
> > +struct ipsec_traffic {
> > +	struct traffic_type ipsec;
> > +	struct traffic_type ip4;
> > +	struct traffic_type ip6;
> > +};
> > +
> > +/* Fields optimized for devices without burst */ struct
> > +traffic_type_nb {
> > +	const uint8_t *data;
> > +	struct rte_mbuf *pkt;
> > +	uint32_t res;
> > +	uint32_t num;
> > +};
> > +
> > +struct ipsec_traffic_nb {
> > +	struct traffic_type_nb ipsec;
> > +	struct traffic_type_nb ip4;
> > +	struct traffic_type_nb ip6;
> > +};
> > +
> > +/* port/source ethernet addr and destination ethernet addr */ struct
> > +ethaddr_info {
> > +	uint64_t src, dst;
> > +};
> > +
> > +struct ethaddr_info ethaddr_tbl[RTE_MAX_ETHPORTS];
> > +
> > +/* TODO: All var definitions need to be part of a .c file */
> 
> Seems like that TODO wasn't done :)
> Probably a good thing to add extern for all global vars declarations here, and
> keep actual definitions in ipsec-secgw.c.
> Same story for:
> +struct socket_ctx socket_ctx[NB_SOCKETS];
> in ipsec.h

[Anoob] Will do in v3.
 
> 
> > +
> >  /* Port mask to identify the unprotected ports */  uint32_t
> > unprotected_port_mask;
> >