[v2,09/12] examples/ipsec-secgw: add eventmode to ipsec-secgw
diff mbox series

Message ID 1579527918-360-10-git-send-email-anoobj@marvell.com
State Superseded, archived
Delegated to: akhil goyal
Headers show
Series
  • add eventmode to ipsec-secgw
Related show

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/checkpatch success coding style OK

Commit Message

Anoob Joseph Jan. 20, 2020, 1:45 p.m. UTC
From: Lukasz Bartosik <lbartosik@marvell.com>

Add eventmode support to ipsec-secgw. With the aid of event helper
configure and use the eventmode capabilities.

Signed-off-by: Anoob Joseph <anoobj@marvell.com>
Signed-off-by: Lukasz Bartosik <lbartosik@marvell.com>
---
 examples/ipsec-secgw/event_helper.c |   4 +-
 examples/ipsec-secgw/event_helper.h |  14 ++
 examples/ipsec-secgw/ipsec-secgw.c  | 341 +++++++++++++++++++++++++++++++++++-
 examples/ipsec-secgw/ipsec.h        |  11 ++
 examples/ipsec-secgw/sa.c           |  11 --
 5 files changed, 365 insertions(+), 16 deletions(-)

Comments

Konstantin Ananyev Jan. 29, 2020, 11:31 p.m. UTC | #1
> Add eventmode support to ipsec-secgw. With the aid of event helper
> configure and use the eventmode capabilities.
> 
> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> Signed-off-by: Lukasz Bartosik <lbartosik@marvell.com>
> ---
>  examples/ipsec-secgw/event_helper.c |   4 +-
>  examples/ipsec-secgw/event_helper.h |  14 ++
>  examples/ipsec-secgw/ipsec-secgw.c  | 341 +++++++++++++++++++++++++++++++++++-
>  examples/ipsec-secgw/ipsec.h        |  11 ++
>  examples/ipsec-secgw/sa.c           |  11 --
>  5 files changed, 365 insertions(+), 16 deletions(-)
> 
> diff --git a/examples/ipsec-secgw/event_helper.c b/examples/ipsec-secgw/event_helper.c
> index 9719ab4..54a98c9 100644
> --- a/examples/ipsec-secgw/event_helper.c
> +++ b/examples/ipsec-secgw/event_helper.c
> @@ -966,6 +966,8 @@ eh_find_worker(uint32_t lcore_id, struct eh_conf *conf,
>  	else
>  		curr_conf.cap.burst = EH_RX_TYPE_NON_BURST;
> 
> +	curr_conf.cap.ipsec_mode = conf->ipsec_mode;
> +
>  	/* Parse the passed list and see if we have matching capabilities */
> 
>  	/* Initialize the pointer used to traverse the list */
> @@ -1625,7 +1627,7 @@ eh_launch_worker(struct eh_conf *conf, struct eh_app_worker_params *app_wrkr,
>  	}
> 
>  	/* Get eventmode conf */
> -	em_conf = (struct eventmode_conf *)(conf->mode_params);
> +	em_conf = conf->mode_params;
> 
>  	/* Get core ID */
>  	lcore_id = rte_lcore_id();
> diff --git a/examples/ipsec-secgw/event_helper.h b/examples/ipsec-secgw/event_helper.h
> index 15a7bd6..cf5d346 100644
> --- a/examples/ipsec-secgw/event_helper.h
> +++ b/examples/ipsec-secgw/event_helper.h
> @@ -74,6 +74,14 @@ enum eh_tx_types {
>  	EH_TX_TYPE_NO_INTERNAL_PORT
>  };
> 
> +/**
> + * Event mode ipsec mode types
> + */
> +enum eh_ipsec_mode_types {
> +	EH_IPSEC_MODE_TYPE_APP = 0,
> +	EH_IPSEC_MODE_TYPE_DRIVER
> +};
> +
>  /* Event dev params */
>  struct eventdev_params {
>  	uint8_t eventdev_id;
> @@ -183,6 +191,10 @@ struct eh_conf {
>  		 */
>  	void *mode_params;
>  		/**< Mode specific parameters */
> +
> +		/** Application specific params */
> +	enum eh_ipsec_mode_types ipsec_mode;
> +		/**< Mode of ipsec run */
>  };
> 
>  /* Workers registered by the application */
> @@ -194,6 +206,8 @@ struct eh_app_worker_params {
>  			/**< Specify status of rx type burst */
>  			uint64_t tx_internal_port : 1;
>  			/**< Specify whether tx internal port is available */
> +			uint64_t ipsec_mode : 1;
> +			/**< Specify ipsec processing level */
>  		};
>  		uint64_t u64;
>  	} cap;
> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
> index d5e8fe5..f1cc3fb 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.c
> +++ b/examples/ipsec-secgw/ipsec-secgw.c
> @@ -2,6 +2,7 @@
>   * Copyright(c) 2016 Intel Corporation
>   */
> 
> +#include <stdbool.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <stdint.h>
> @@ -14,6 +15,7 @@
>  #include <sys/queue.h>
>  #include <stdarg.h>
>  #include <errno.h>
> +#include <signal.h>
>  #include <getopt.h>
> 
>  #include <rte_common.h>
> @@ -41,12 +43,17 @@
>  #include <rte_jhash.h>
>  #include <rte_cryptodev.h>
>  #include <rte_security.h>
> +#include <rte_bitmap.h>
> +#include <rte_eventdev.h>
>  #include <rte_ip.h>
>  #include <rte_ip_frag.h>
> 
> +#include "event_helper.h"
>  #include "ipsec.h"
>  #include "parser.h"
> 
> +volatile bool force_quit;
> +
>  #define RTE_LOGTYPE_IPSEC RTE_LOGTYPE_USER1
> 
>  #define MAX_JUMBO_PKT_LEN  9600
> @@ -133,12 +140,20 @@ struct flow_info flow_info_tbl[RTE_MAX_ETHPORTS];
>  #define CMD_LINE_OPT_CONFIG		"config"
>  #define CMD_LINE_OPT_SINGLE_SA		"single-sa"
>  #define CMD_LINE_OPT_CRYPTODEV_MASK	"cryptodev_mask"
> +#define CMD_LINE_OPT_TRANSFER_MODE	"transfer-mode"
> +#define CMD_LINE_OPT_SCHEDULE_TYPE	"schedule-type"
>  #define CMD_LINE_OPT_RX_OFFLOAD		"rxoffload"
>  #define CMD_LINE_OPT_TX_OFFLOAD		"txoffload"
>  #define CMD_LINE_OPT_REASSEMBLE		"reassemble"
>  #define CMD_LINE_OPT_MTU		"mtu"
>  #define CMD_LINE_OPT_FRAG_TTL		"frag-ttl"
> 
> +#define CMD_LINE_ARG_EVENT	"event"
> +#define CMD_LINE_ARG_POLL	"poll"
> +#define CMD_LINE_ARG_ORDERED	"ordered"
> +#define CMD_LINE_ARG_ATOMIC	"atomic"
> +#define CMD_LINE_ARG_PARALLEL	"parallel"
> +
>  enum {
>  	/* long options mapped to a short option */
> 
> @@ -149,6 +164,8 @@ enum {
>  	CMD_LINE_OPT_CONFIG_NUM,
>  	CMD_LINE_OPT_SINGLE_SA_NUM,
>  	CMD_LINE_OPT_CRYPTODEV_MASK_NUM,
> +	CMD_LINE_OPT_TRANSFER_MODE_NUM,
> +	CMD_LINE_OPT_SCHEDULE_TYPE_NUM,
>  	CMD_LINE_OPT_RX_OFFLOAD_NUM,
>  	CMD_LINE_OPT_TX_OFFLOAD_NUM,
>  	CMD_LINE_OPT_REASSEMBLE_NUM,
> @@ -160,6 +177,8 @@ static const struct option lgopts[] = {
>  	{CMD_LINE_OPT_CONFIG, 1, 0, CMD_LINE_OPT_CONFIG_NUM},
>  	{CMD_LINE_OPT_SINGLE_SA, 1, 0, CMD_LINE_OPT_SINGLE_SA_NUM},
>  	{CMD_LINE_OPT_CRYPTODEV_MASK, 1, 0, CMD_LINE_OPT_CRYPTODEV_MASK_NUM},
> +	{CMD_LINE_OPT_TRANSFER_MODE, 1, 0, CMD_LINE_OPT_TRANSFER_MODE_NUM},
> +	{CMD_LINE_OPT_SCHEDULE_TYPE, 1, 0, CMD_LINE_OPT_SCHEDULE_TYPE_NUM},
>  	{CMD_LINE_OPT_RX_OFFLOAD, 1, 0, CMD_LINE_OPT_RX_OFFLOAD_NUM},
>  	{CMD_LINE_OPT_TX_OFFLOAD, 1, 0, CMD_LINE_OPT_TX_OFFLOAD_NUM},
>  	{CMD_LINE_OPT_REASSEMBLE, 1, 0, CMD_LINE_OPT_REASSEMBLE_NUM},
> @@ -177,6 +196,7 @@ static int32_t numa_on = 1; /**< NUMA is enabled by default. */
>  static uint32_t nb_lcores;
>  static uint32_t single_sa;
>  static uint32_t single_sa_idx;
> +static uint32_t schedule_type;
> 
>  /*
>   * RX/TX HW offload capabilities to enable/use on ethernet ports.
> @@ -1185,7 +1205,7 @@ main_loop(__attribute__((unused)) void *dummy)
>  }
> 
>  static int32_t
> -check_params(void)
> +check_params(struct eh_conf *eh_conf)
>  {
>  	uint8_t lcore;
>  	uint16_t portid;
> @@ -1220,6 +1240,14 @@ check_params(void)
>  			return -1;
>  		}
>  	}
> +
> +	if (eh_conf->mode == EH_PKT_TRANSFER_MODE_POLL) {
> +		if (schedule_type) {
> +			printf("error: option --schedule-type applies only to event mode\n");
> +			return -1;
> +		}
> +	}

As a nit - might be better to keep check_params() intact,
and put this new check above into a separate function?
check_eh_conf() or so?
Another thing it seems a bit clumsy construction to have global var (scheduler_type)
just to figure out was particular option present on command line or not.
Probably simler way to avoid it - set initially em_conf->ext_params.sched_type to
some invalid value (-1 or so). Then after parse args you can check did its value
change or not.
As alternative thought: wouldn't it be better to unite both  --transfer-mode
and --schedule-type options into one?
Then possible values for this unite option would be:
"poll"
"event" (expands to "event-ordered")
"event-ordered"
"event-atomic"
"event-parallel"
And this situation you are checking above simply wouldn't be possible.
Again probably would be easier/simpler for users.


> +
>  	return 0;
>  }
> 
> @@ -1277,6 +1305,8 @@ print_usage(const char *prgname)
>  		" --config (port,queue,lcore)[,(port,queue,lcore)]"
>  		" [--single-sa SAIDX]"
>  		" [--cryptodev_mask MASK]"
> +		" [--transfer-mode MODE]"
> +		" [--schedule-type TYPE]"
>  		" [--" CMD_LINE_OPT_RX_OFFLOAD " RX_OFFLOAD_MASK]"
>  		" [--" CMD_LINE_OPT_TX_OFFLOAD " TX_OFFLOAD_MASK]"
>  		" [--" CMD_LINE_OPT_REASSEMBLE " REASSEMBLE_TABLE_SIZE]"
> @@ -1298,6 +1328,14 @@ print_usage(const char *prgname)
>  		"                     bypassing the SP\n"
>  		"  --cryptodev_mask MASK: Hexadecimal bitmask of the crypto\n"
>  		"                         devices to configure\n"
> +		"  --transfer-mode MODE\n"
> +		"               \"poll\"  : Packet transfer via polling (default)\n"
> +		"               \"event\" : Packet transfer via event device\n"
> +		"  --schedule-type TYPE queue schedule type, used only when\n"
> +		"                       transfer mode is set to event\n"
> +		"               \"ordered\"  : Ordered (default)\n"
> +		"               \"atomic\"   : Atomic\n"
> +		"               \"parallel\" : Parallel\n"
>  		"  --" CMD_LINE_OPT_RX_OFFLOAD
>  		": bitmask of the RX HW offload capabilities to enable/use\n"
>  		"                         (DEV_RX_OFFLOAD_*)\n"
> @@ -1432,8 +1470,45 @@ print_app_sa_prm(const struct app_sa_prm *prm)
>  	printf("Frag TTL: %" PRIu64 " ns\n", frag_ttl_ns);
>  }
> 
> +static int
> +parse_transfer_mode(struct eh_conf *conf, const char *optarg)
> +{
> +	if (!strcmp(CMD_LINE_ARG_POLL, optarg))
> +		conf->mode = EH_PKT_TRANSFER_MODE_POLL;
> +	else if (!strcmp(CMD_LINE_ARG_EVENT, optarg))
> +		conf->mode = EH_PKT_TRANSFER_MODE_EVENT;
> +	else {
> +		printf("Unsupported packet transfer mode\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +parse_schedule_type(struct eh_conf *conf, const char *optarg)
> +{
> +	struct eventmode_conf *em_conf = NULL;
> +
> +	/* Get eventmode conf */
> +	em_conf = conf->mode_params;
> +
> +	if (!strcmp(CMD_LINE_ARG_ORDERED, optarg))
> +		em_conf->ext_params.sched_type = RTE_SCHED_TYPE_ORDERED;
> +	else if (!strcmp(CMD_LINE_ARG_ATOMIC, optarg))
> +		em_conf->ext_params.sched_type = RTE_SCHED_TYPE_ATOMIC;
> +	else if (!strcmp(CMD_LINE_ARG_PARALLEL, optarg))
> +		em_conf->ext_params.sched_type = RTE_SCHED_TYPE_PARALLEL;
> +	else {
> +		printf("Unsupported queue schedule type\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int32_t
> -parse_args(int32_t argc, char **argv)
> +parse_args(int32_t argc, char **argv, struct eh_conf *eh_conf)
>  {
>  	int opt;
>  	int64_t ret;
> @@ -1522,6 +1597,7 @@ parse_args(int32_t argc, char **argv)
>  			/* else */
>  			single_sa = 1;
>  			single_sa_idx = ret;
> +			eh_conf->ipsec_mode = EH_IPSEC_MODE_TYPE_DRIVER;
>  			printf("Configured with single SA index %u\n",
>  					single_sa_idx);
>  			break;
> @@ -1536,6 +1612,26 @@ parse_args(int32_t argc, char **argv)
>  			/* else */
>  			enabled_cryptodev_mask = ret;
>  			break;
> +
> +		case CMD_LINE_OPT_TRANSFER_MODE_NUM:
> +			ret = parse_transfer_mode(eh_conf, optarg);
> +			if (ret < 0) {
> +				printf("Invalid packet transfer mode\n");
> +				print_usage(prgname);
> +				return -1;
> +			}
> +			break;
> +
> +		case CMD_LINE_OPT_SCHEDULE_TYPE_NUM:
> +			ret = parse_schedule_type(eh_conf, optarg);
> +			if (ret < 0) {
> +				printf("Invalid queue schedule type\n");
> +				print_usage(prgname);
> +				return -1;
> +			}
> +			schedule_type = 1;
> +			break;
> +
>  		case CMD_LINE_OPT_RX_OFFLOAD_NUM:
>  			ret = parse_mask(optarg, &dev_rx_offload);
>  			if (ret != 0) {
> @@ -2450,16 +2546,176 @@ create_default_ipsec_flow(uint16_t port_id, uint64_t rx_offloads)
>  		port_id);
>  }
> 

Wouldn't it be more natural to have these 2 functions below
(eh_conf_init(), eh_conf_uninit()) defined inside event_helper.c?

> +static struct eh_conf *
> +eh_conf_init(void)
> +{
> +	struct eventmode_conf *em_conf = NULL;
> +	struct eh_conf *conf = NULL;
> +	unsigned int eth_core_id;
> +	uint32_t nb_bytes;
> +	void *mem = NULL;
> +
> +	/* Allocate memory for config */
> +	conf = calloc(1, sizeof(struct eh_conf));
> +	if (conf == NULL) {
> +		printf("Failed to allocate memory for eventmode helper conf");
> +		goto err;
> +	}
> +
> +	/* Set default conf */
> +
> +	/* Packet transfer mode: poll */
> +	conf->mode = EH_PKT_TRANSFER_MODE_POLL;
> +	conf->ipsec_mode = EH_IPSEC_MODE_TYPE_APP;
> +
> +	/* Keep all ethernet ports enabled by default */
> +	conf->eth_portmask = -1;
> +
> +	/* Allocate memory for event mode params */
> +	conf->mode_params = calloc(1, sizeof(struct eventmode_conf));
> +	if (conf->mode_params == NULL) {
> +		printf("Failed to allocate memory for event mode params");
> +		goto err;
> +	}
> +
> +	/* Get eventmode conf */
> +	em_conf = conf->mode_params;
> +
> +	/* Allocate and initialize bitmap for eth cores */
> +	nb_bytes = rte_bitmap_get_memory_footprint(RTE_MAX_LCORE);
> +	if (!nb_bytes) {
> +		printf("Failed to get bitmap footprint");
> +		goto err;
> +	}
> +
> +	mem = rte_zmalloc("event-helper-ethcore-bitmap", nb_bytes,
> +			  RTE_CACHE_LINE_SIZE);
> +	if (!mem) {
> +		printf("Failed to allocate memory for eth cores bitmap\n");
> +		goto err;
> +	}
> +
> +	em_conf->eth_core_mask = rte_bitmap_init(RTE_MAX_LCORE, mem, nb_bytes);
> +	if (!em_conf->eth_core_mask) {
> +		printf("Failed to initialize bitmap");
> +		goto err;
> +	}
> +
> +	/* Schedule type: ordered */
> +	em_conf->ext_params.sched_type = RTE_SCHED_TYPE_ORDERED;
> +
> +	/* Set two cores as eth cores for Rx & Tx */
> +
> +	/* Use first core other than master core as Rx core */
> +	eth_core_id = rte_get_next_lcore(0,	/* curr core */
> +					 1,	/* skip master core */
> +					 0	/* wrap */);
> +
> +	rte_bitmap_set(em_conf->eth_core_mask, eth_core_id);
> +
> +	/* Use next core as Tx core */
> +	eth_core_id = rte_get_next_lcore(eth_core_id,	/* curr core */
> +					 1,		/* skip master core */
> +					 0		/* wrap */);
> +
> +	rte_bitmap_set(em_conf->eth_core_mask, eth_core_id);
> +
> +	return conf;
> +err:
> +	rte_free(mem);
> +	free(em_conf);
> +	free(conf);
> +	return NULL;
> +}
> +
> +static void
> +eh_conf_uninit(struct eh_conf *conf)
> +{
> +	struct eventmode_conf *em_conf = NULL;
> +
> +	/* Get eventmode conf */
> +	em_conf = conf->mode_params;
> +
> +	/* Free evenmode configuration memory */
> +	rte_free(em_conf->eth_core_mask);
> +	free(em_conf);
> +	free(conf);
> +}
> +
> +static void
> +signal_handler(int signum)
> +{
> +	if (signum == SIGINT || signum == SIGTERM) {
> +		printf("\n\nSignal %d received, preparing to exit...\n",
> +				signum);
> +		force_quit = true;
> +	}
> +}
> +
> +static void
> +inline_sessions_free(struct sa_ctx *sa_ctx)
> +{
> +	struct rte_ipsec_session *ips;
> +	struct ipsec_sa *sa;
> +	int32_t i, ret;
> +
> +	for (i = 0; i < IPSEC_SA_MAX_ENTRIES; i++) {
> +
> +		sa = &sa_ctx->sa[i];
> +		if (!sa->spi)
> +			continue;
> +
> +		ips = ipsec_get_primary_session(sa);
> +		if (ips->type != RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL &&
> +		    ips->type != RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO)
> +			continue;
> +
> +		ret = rte_security_session_destroy(
> +				rte_eth_dev_get_sec_ctx(sa->portid),
> +				ips->security.ses);
> +		if (ret)
> +			RTE_LOG(ERR, IPSEC, "Failed to destroy security "
> +					    "session type %d, spi %d\n",
> +					    ips->type, sa->spi);
> +	}
> +}
> +
> +static void
> +ev_mode_sess_verify(struct sa_ctx *sa_ctx)
> +{
> +	struct rte_ipsec_session *ips;
> +	struct ipsec_sa *sa;
> +	int32_t i;
> +
> +	if (!sa_ctx)
> +		return;
> +
> +	for (i = 0; i < IPSEC_SA_MAX_ENTRIES; i++) {
> +
> +		sa = &sa_ctx->sa[i];
> +		if (!sa->spi)
> +			continue;
> +
> +		ips = ipsec_get_primary_session(sa);
> +		if (ips->type != RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL)
> +			rte_exit(EXIT_FAILURE, "Event mode supports only "
> +				 "inline protocol sessions\n");

As I understand at that moment inline sessions already created on devices?
For consistency wouldn't it be better to do this check at parsing cfg file,
or straight after it? 

> +	}
> +
> +}
> +
>  int32_t
>  main(int32_t argc, char **argv)
>  {
>  	int32_t ret;
>  	uint32_t lcore_id;
> +	uint32_t cdev_id;
>  	uint32_t i;
>  	uint8_t socket_id;
>  	uint16_t portid;
>  	uint64_t req_rx_offloads[RTE_MAX_ETHPORTS];
>  	uint64_t req_tx_offloads[RTE_MAX_ETHPORTS];
> +	struct eh_conf *eh_conf = NULL;
>  	size_t sess_sz;
> 
>  	/* init EAL */
> @@ -2469,8 +2725,17 @@ main(int32_t argc, char **argv)
>  	argc -= ret;
>  	argv += ret;
> 
> +	force_quit = false;
> +	signal(SIGINT, signal_handler);
> +	signal(SIGTERM, signal_handler);
> +
> +	/* initialize event helper configuration */
> +	eh_conf = eh_conf_init();
> +	if (eh_conf == NULL)
> +		rte_exit(EXIT_FAILURE, "Failed to init event helper config");
> +
>  	/* parse application arguments (after the EAL ones) */
> -	ret = parse_args(argc, argv);
> +	ret = parse_args(argc, argv, eh_conf);
>  	if (ret < 0)
>  		rte_exit(EXIT_FAILURE, "Invalid parameters\n");
> 
> @@ -2487,7 +2752,7 @@ main(int32_t argc, char **argv)
>  		rte_exit(EXIT_FAILURE, "Invalid unprotected portmask 0x%x\n",
>  				unprotected_port_mask);
> 
> -	if (check_params() < 0)
> +	if (check_params(eh_conf) < 0)
>  		rte_exit(EXIT_FAILURE, "check_params failed\n");
> 
>  	ret = init_lcore_rx_queues();
> @@ -2529,6 +2794,18 @@ main(int32_t argc, char **argv)
> 
>  	cryptodevs_init();
> 
> +	/*
> +	 * Set the enabled port mask in helper config for use by helper
> +	 * sub-system. This will be used while initializing devices using
> +	 * helper sub-system.
> +	 */
> +	eh_conf->eth_portmask = enabled_port_mask;
> +
> +	/* Initialize eventmode components */
> +	ret = eh_devs_init(eh_conf);
> +	if (ret < 0)
> +		rte_exit(EXIT_FAILURE, "eh_devs_init failed, err=%d\n", ret);
> +
>  	/* start ports */
>  	RTE_ETH_FOREACH_DEV(portid) {
>  		if ((enabled_port_mask & (1 << portid)) == 0)
> @@ -2576,6 +2853,18 @@ main(int32_t argc, char **argv)
>  			sp4_init(&socket_ctx[socket_id], socket_id);
>  			sp6_init(&socket_ctx[socket_id], socket_id);
>  			rt_init(&socket_ctx[socket_id], socket_id);
> +
> +			/*
> +			 * Event mode currently supports only inline protocol
> +			 * sessions. If there are other types of sessions
> +			 * configured then exit with error.
> +			 */
> +			if (eh_conf->mode == EH_PKT_TRANSFER_MODE_EVENT) {
> +				ev_mode_sess_verify(
> +						socket_ctx[socket_id].sa_in);
> +				ev_mode_sess_verify(
> +						socket_ctx[socket_id].sa_out);
> +			}
>  		}
>  	}
> 
> @@ -2583,10 +2872,54 @@ main(int32_t argc, char **argv)
> 	
>  	/* launch per-lcore init on every lcore */
>  	rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
> +
>  	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>  		if (rte_eal_wait_lcore(lcore_id) < 0)
>  			return -1;
>  	}
> 
> +	/* Uninitialize eventmode components */
> +	ret = eh_devs_uninit(eh_conf);
> +	if (ret < 0)
> +		rte_exit(EXIT_FAILURE, "eh_devs_uninit failed, err=%d\n", ret);
> +
> +	/* Free eventmode configuration memory */
> +	eh_conf_uninit(eh_conf);
> +
> +	/* Destroy inline inbound and outbound sessions */
> +	for (i = 0; i < NB_SOCKETS && i < rte_socket_count(); i++) {
> +		socket_id = rte_socket_id_by_idx(i);
> +		inline_sessions_free(socket_ctx[socket_id].sa_in);

That causes a crash on 2 socket system with the config that uses
lcores only from the first socket.

> +		inline_sessions_free(socket_ctx[socket_id].sa_out);
> +	}
> +
> +	for (cdev_id = 0; cdev_id < rte_cryptodev_count(); cdev_id++) {
> +		printf("Closing cryptodev %d...", cdev_id);
> +		rte_cryptodev_stop(cdev_id);
> +		rte_cryptodev_close(cdev_id);
> +		printf(" Done\n");
> +	}
> +
> +	RTE_ETH_FOREACH_DEV(portid) {
> +		if ((enabled_port_mask & (1 << portid)) == 0)
> +			continue;
> +
> +		printf("Closing port %d...", portid);
> +		if (flow_info_tbl[portid].rx_def_flow) {
> +			struct rte_flow_error err;
> +
> +			ret = rte_flow_destroy(portid,
> +				flow_info_tbl[portid].rx_def_flow, &err);
> +			if (ret)
> +				RTE_LOG(ERR, IPSEC, "Failed to destroy flow "
> +					" for port %u, err msg: %s\n", portid,
> +					err.message);
> +		}
> +		rte_eth_dev_stop(portid);
> +		rte_eth_dev_close(portid);
> +		printf(" Done\n");
> +	}
> +	printf("Bye...\n");
> +
>  	return 0;
>  }
> diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
> index 28ff07d..0539aec 100644
> --- a/examples/ipsec-secgw/ipsec.h
> +++ b/examples/ipsec-secgw/ipsec.h
> @@ -153,6 +153,17 @@ struct ipsec_sa {
>  	struct rte_security_session_conf sess_conf;
>  } __rte_cache_aligned;
> 
> +struct sa_ctx {
> +	void *satbl; /* pointer to array of rte_ipsec_sa objects*/
> +	struct ipsec_sa sa[IPSEC_SA_MAX_ENTRIES];
> +	union {
> +		struct {
> +			struct rte_crypto_sym_xform a;
> +			struct rte_crypto_sym_xform b;
> +		};
> +	} xf[IPSEC_SA_MAX_ENTRIES];
> +};
> +
>  struct ipsec_mbuf_metadata {
>  	struct ipsec_sa *sa;
>  	struct rte_crypto_op cop;
> diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
> index c75a5a1..2ec3e17 100644
> --- a/examples/ipsec-secgw/sa.c
> +++ b/examples/ipsec-secgw/sa.c
> @@ -781,17 +781,6 @@ print_one_sa_rule(const struct ipsec_sa *sa, int inbound)
>  	printf("\n");
>  }
> 
> -struct sa_ctx {
> -	void *satbl; /* pointer to array of rte_ipsec_sa objects*/
> -	struct ipsec_sa sa[IPSEC_SA_MAX_ENTRIES];
> -	union {
> -		struct {
> -			struct rte_crypto_sym_xform a;
> -			struct rte_crypto_sym_xform b;
> -		};
> -	} xf[IPSEC_SA_MAX_ENTRIES];
> -};
> -
>  static struct sa_ctx *
>  sa_create(const char *name, int32_t socket_id)
>  {
> --
> 2.7.4
Lukasz Bartosik Jan. 30, 2020, 11:04 a.m. UTC | #2
Hi Konstantin,

Please see inline.

Thanks,
Lukasz

On 30.01.2020 00:31, Ananyev, Konstantin wrote:
> External Email
>
> ----------------------------------------------------------------------
>> Add eventmode support to ipsec-secgw. With the aid of event helper
>> configure and use the eventmode capabilities.
>>
>> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
>> Signed-off-by: Lukasz Bartosik <lbartosik@marvell.com>
>> ---
>>  examples/ipsec-secgw/event_helper.c |   4 +-
>>  examples/ipsec-secgw/event_helper.h |  14 ++
>>  examples/ipsec-secgw/ipsec-secgw.c  | 341 +++++++++++++++++++++++++++++++++++-
>>  examples/ipsec-secgw/ipsec.h        |  11 ++
>>  examples/ipsec-secgw/sa.c           |  11 --
>>  5 files changed, 365 insertions(+), 16 deletions(-)
>>
>> diff --git a/examples/ipsec-secgw/event_helper.c b/examples/ipsec-secgw/event_helper.c
>> index 9719ab4..54a98c9 100644
>> --- a/examples/ipsec-secgw/event_helper.c
>> +++ b/examples/ipsec-secgw/event_helper.c
>> @@ -966,6 +966,8 @@ eh_find_worker(uint32_t lcore_id, struct eh_conf *conf,
>>  	else
>>  		curr_conf.cap.burst = EH_RX_TYPE_NON_BURST;
>>
>> +	curr_conf.cap.ipsec_mode = conf->ipsec_mode;
>> +
>>  	/* Parse the passed list and see if we have matching capabilities */
>>
>>  	/* Initialize the pointer used to traverse the list */
>> @@ -1625,7 +1627,7 @@ eh_launch_worker(struct eh_conf *conf, struct eh_app_worker_params *app_wrkr,
>>  	}
>>
>>  	/* Get eventmode conf */
>> -	em_conf = (struct eventmode_conf *)(conf->mode_params);
>> +	em_conf = conf->mode_params;
>>
>>  	/* Get core ID */
>>  	lcore_id = rte_lcore_id();
>> diff --git a/examples/ipsec-secgw/event_helper.h b/examples/ipsec-secgw/event_helper.h
>> index 15a7bd6..cf5d346 100644
>> --- a/examples/ipsec-secgw/event_helper.h
>> +++ b/examples/ipsec-secgw/event_helper.h
>> @@ -74,6 +74,14 @@ enum eh_tx_types {
>>  	EH_TX_TYPE_NO_INTERNAL_PORT
>>  };
>>
>> +/**
>> + * Event mode ipsec mode types
>> + */
>> +enum eh_ipsec_mode_types {
>> +	EH_IPSEC_MODE_TYPE_APP = 0,
>> +	EH_IPSEC_MODE_TYPE_DRIVER
>> +};
>> +
>>  /* Event dev params */
>>  struct eventdev_params {
>>  	uint8_t eventdev_id;
>> @@ -183,6 +191,10 @@ struct eh_conf {
>>  		 */
>>  	void *mode_params;
>>  		/**< Mode specific parameters */
>> +
>> +		/** Application specific params */
>> +	enum eh_ipsec_mode_types ipsec_mode;
>> +		/**< Mode of ipsec run */
>>  };
>>
>>  /* Workers registered by the application */
>> @@ -194,6 +206,8 @@ struct eh_app_worker_params {
>>  			/**< Specify status of rx type burst */
>>  			uint64_t tx_internal_port : 1;
>>  			/**< Specify whether tx internal port is available */
>> +			uint64_t ipsec_mode : 1;
>> +			/**< Specify ipsec processing level */
>>  		};
>>  		uint64_t u64;
>>  	} cap;
>> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
>> index d5e8fe5..f1cc3fb 100644
>> --- a/examples/ipsec-secgw/ipsec-secgw.c
>> +++ b/examples/ipsec-secgw/ipsec-secgw.c
>> @@ -2,6 +2,7 @@
>>   * Copyright(c) 2016 Intel Corporation
>>   */
>>
>> +#include <stdbool.h>
>>  #include <stdio.h>
>>  #include <stdlib.h>
>>  #include <stdint.h>
>> @@ -14,6 +15,7 @@
>>  #include <sys/queue.h>
>>  #include <stdarg.h>
>>  #include <errno.h>
>> +#include <signal.h>
>>  #include <getopt.h>
>>
>>  #include <rte_common.h>
>> @@ -41,12 +43,17 @@
>>  #include <rte_jhash.h>
>>  #include <rte_cryptodev.h>
>>  #include <rte_security.h>
>> +#include <rte_bitmap.h>
>> +#include <rte_eventdev.h>
>>  #include <rte_ip.h>
>>  #include <rte_ip_frag.h>
>>
>> +#include "event_helper.h"
>>  #include "ipsec.h"
>>  #include "parser.h"
>>
>> +volatile bool force_quit;
>> +
>>  #define RTE_LOGTYPE_IPSEC RTE_LOGTYPE_USER1
>>
>>  #define MAX_JUMBO_PKT_LEN  9600
>> @@ -133,12 +140,20 @@ struct flow_info flow_info_tbl[RTE_MAX_ETHPORTS];
>>  #define CMD_LINE_OPT_CONFIG		"config"
>>  #define CMD_LINE_OPT_SINGLE_SA		"single-sa"
>>  #define CMD_LINE_OPT_CRYPTODEV_MASK	"cryptodev_mask"
>> +#define CMD_LINE_OPT_TRANSFER_MODE	"transfer-mode"
>> +#define CMD_LINE_OPT_SCHEDULE_TYPE	"schedule-type"
>>  #define CMD_LINE_OPT_RX_OFFLOAD		"rxoffload"
>>  #define CMD_LINE_OPT_TX_OFFLOAD		"txoffload"
>>  #define CMD_LINE_OPT_REASSEMBLE		"reassemble"
>>  #define CMD_LINE_OPT_MTU		"mtu"
>>  #define CMD_LINE_OPT_FRAG_TTL		"frag-ttl"
>>
>> +#define CMD_LINE_ARG_EVENT	"event"
>> +#define CMD_LINE_ARG_POLL	"poll"
>> +#define CMD_LINE_ARG_ORDERED	"ordered"
>> +#define CMD_LINE_ARG_ATOMIC	"atomic"
>> +#define CMD_LINE_ARG_PARALLEL	"parallel"
>> +
>>  enum {
>>  	/* long options mapped to a short option */
>>
>> @@ -149,6 +164,8 @@ enum {
>>  	CMD_LINE_OPT_CONFIG_NUM,
>>  	CMD_LINE_OPT_SINGLE_SA_NUM,
>>  	CMD_LINE_OPT_CRYPTODEV_MASK_NUM,
>> +	CMD_LINE_OPT_TRANSFER_MODE_NUM,
>> +	CMD_LINE_OPT_SCHEDULE_TYPE_NUM,
>>  	CMD_LINE_OPT_RX_OFFLOAD_NUM,
>>  	CMD_LINE_OPT_TX_OFFLOAD_NUM,
>>  	CMD_LINE_OPT_REASSEMBLE_NUM,
>> @@ -160,6 +177,8 @@ static const struct option lgopts[] = {
>>  	{CMD_LINE_OPT_CONFIG, 1, 0, CMD_LINE_OPT_CONFIG_NUM},
>>  	{CMD_LINE_OPT_SINGLE_SA, 1, 0, CMD_LINE_OPT_SINGLE_SA_NUM},
>>  	{CMD_LINE_OPT_CRYPTODEV_MASK, 1, 0, CMD_LINE_OPT_CRYPTODEV_MASK_NUM},
>> +	{CMD_LINE_OPT_TRANSFER_MODE, 1, 0, CMD_LINE_OPT_TRANSFER_MODE_NUM},
>> +	{CMD_LINE_OPT_SCHEDULE_TYPE, 1, 0, CMD_LINE_OPT_SCHEDULE_TYPE_NUM},
>>  	{CMD_LINE_OPT_RX_OFFLOAD, 1, 0, CMD_LINE_OPT_RX_OFFLOAD_NUM},
>>  	{CMD_LINE_OPT_TX_OFFLOAD, 1, 0, CMD_LINE_OPT_TX_OFFLOAD_NUM},
>>  	{CMD_LINE_OPT_REASSEMBLE, 1, 0, CMD_LINE_OPT_REASSEMBLE_NUM},
>> @@ -177,6 +196,7 @@ static int32_t numa_on = 1; /**< NUMA is enabled by default. */
>>  static uint32_t nb_lcores;
>>  static uint32_t single_sa;
>>  static uint32_t single_sa_idx;
>> +static uint32_t schedule_type;
>>
>>  /*
>>   * RX/TX HW offload capabilities to enable/use on ethernet ports.
>> @@ -1185,7 +1205,7 @@ main_loop(__attribute__((unused)) void *dummy)
>>  }
>>
>>  static int32_t
>> -check_params(void)
>> +check_params(struct eh_conf *eh_conf)
>>  {
>>  	uint8_t lcore;
>>  	uint16_t portid;
>> @@ -1220,6 +1240,14 @@ check_params(void)
>>  			return -1;
>>  		}
>>  	}
>> +
>> +	if (eh_conf->mode == EH_PKT_TRANSFER_MODE_POLL) {
>> +		if (schedule_type) {
>> +			printf("error: option --schedule-type applies only to event mode\n");
>> +			return -1;
>> +		}
>> +	}
>
> As a nit - might be better to keep check_params() intact,
> and put this new check above into a separate function?
> check_eh_conf() or so?

[Lukasz] I will put the check into new check_eh_conf() function.

> Another thing it seems a bit clumsy construction to have global var (scheduler_type)
> just to figure out was particular option present on command line or not.
> Probably simler way to avoid it - set initially em_conf->ext_params.sched_type to
> some invalid value (-1 or so). Then after parse args you can check did its value
> change or not.

[Lukasz] I will change it in V3.

> As alternative thought: wouldn't it be better to unite both  --transfer-mode
> and --schedule-type options into one?
> Then possible values for this unite option would be:
> "poll"
> "event" (expands to "event-ordered")
> "event-ordered"
> "event-atomic"
> "event-parallel"
> And this situation you are checking above simply wouldn't be possible.
> Again probably would be easier/simpler for users.

[Lukasz] I would rather not combine event mode parameters into one for two reason:
- to be consistent with poll where one configuration item is controlled with one option,
- if we come up with a need to add a new event mode parameter in future then we 
we will need to split event-ordered back to --transfer-mode and --schedule-type
to be consistent with how with provide event mode command line options.

>
>> +
>>  	return 0;
>>  }
>>
>> @@ -1277,6 +1305,8 @@ print_usage(const char *prgname)
>>  		" --config (port,queue,lcore)[,(port,queue,lcore)]"
>>  		" [--single-sa SAIDX]"
>>  		" [--cryptodev_mask MASK]"
>> +		" [--transfer-mode MODE]"
>> +		" [--schedule-type TYPE]"
>>  		" [--" CMD_LINE_OPT_RX_OFFLOAD " RX_OFFLOAD_MASK]"
>>  		" [--" CMD_LINE_OPT_TX_OFFLOAD " TX_OFFLOAD_MASK]"
>>  		" [--" CMD_LINE_OPT_REASSEMBLE " REASSEMBLE_TABLE_SIZE]"
>> @@ -1298,6 +1328,14 @@ print_usage(const char *prgname)
>>  		"                     bypassing the SP\n"
>>  		"  --cryptodev_mask MASK: Hexadecimal bitmask of the crypto\n"
>>  		"                         devices to configure\n"
>> +		"  --transfer-mode MODE\n"
>> +		"               \"poll\"  : Packet transfer via polling (default)\n"
>> +		"               \"event\" : Packet transfer via event device\n"
>> +		"  --schedule-type TYPE queue schedule type, used only when\n"
>> +		"                       transfer mode is set to event\n"
>> +		"               \"ordered\"  : Ordered (default)\n"
>> +		"               \"atomic\"   : Atomic\n"
>> +		"               \"parallel\" : Parallel\n"
>>  		"  --" CMD_LINE_OPT_RX_OFFLOAD
>>  		": bitmask of the RX HW offload capabilities to enable/use\n"
>>  		"                         (DEV_RX_OFFLOAD_*)\n"
>> @@ -1432,8 +1470,45 @@ print_app_sa_prm(const struct app_sa_prm *prm)
>>  	printf("Frag TTL: %" PRIu64 " ns\n", frag_ttl_ns);
>>  }
>>
>> +static int
>> +parse_transfer_mode(struct eh_conf *conf, const char *optarg)
>> +{
>> +	if (!strcmp(CMD_LINE_ARG_POLL, optarg))
>> +		conf->mode = EH_PKT_TRANSFER_MODE_POLL;
>> +	else if (!strcmp(CMD_LINE_ARG_EVENT, optarg))
>> +		conf->mode = EH_PKT_TRANSFER_MODE_EVENT;
>> +	else {
>> +		printf("Unsupported packet transfer mode\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int
>> +parse_schedule_type(struct eh_conf *conf, const char *optarg)
>> +{
>> +	struct eventmode_conf *em_conf = NULL;
>> +
>> +	/* Get eventmode conf */
>> +	em_conf = conf->mode_params;
>> +
>> +	if (!strcmp(CMD_LINE_ARG_ORDERED, optarg))
>> +		em_conf->ext_params.sched_type = RTE_SCHED_TYPE_ORDERED;
>> +	else if (!strcmp(CMD_LINE_ARG_ATOMIC, optarg))
>> +		em_conf->ext_params.sched_type = RTE_SCHED_TYPE_ATOMIC;
>> +	else if (!strcmp(CMD_LINE_ARG_PARALLEL, optarg))
>> +		em_conf->ext_params.sched_type = RTE_SCHED_TYPE_PARALLEL;
>> +	else {
>> +		printf("Unsupported queue schedule type\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int32_t
>> -parse_args(int32_t argc, char **argv)
>> +parse_args(int32_t argc, char **argv, struct eh_conf *eh_conf)
>>  {
>>  	int opt;
>>  	int64_t ret;
>> @@ -1522,6 +1597,7 @@ parse_args(int32_t argc, char **argv)
>>  			/* else */
>>  			single_sa = 1;
>>  			single_sa_idx = ret;
>> +			eh_conf->ipsec_mode = EH_IPSEC_MODE_TYPE_DRIVER;
>>  			printf("Configured with single SA index %u\n",
>>  					single_sa_idx);
>>  			break;
>> @@ -1536,6 +1612,26 @@ parse_args(int32_t argc, char **argv)
>>  			/* else */
>>  			enabled_cryptodev_mask = ret;
>>  			break;
>> +
>> +		case CMD_LINE_OPT_TRANSFER_MODE_NUM:
>> +			ret = parse_transfer_mode(eh_conf, optarg);
>> +			if (ret < 0) {
>> +				printf("Invalid packet transfer mode\n");
>> +				print_usage(prgname);
>> +				return -1;
>> +			}
>> +			break;
>> +
>> +		case CMD_LINE_OPT_SCHEDULE_TYPE_NUM:
>> +			ret = parse_schedule_type(eh_conf, optarg);
>> +			if (ret < 0) {
>> +				printf("Invalid queue schedule type\n");
>> +				print_usage(prgname);
>> +				return -1;
>> +			}
>> +			schedule_type = 1;
>> +			break;
>> +
>>  		case CMD_LINE_OPT_RX_OFFLOAD_NUM:
>>  			ret = parse_mask(optarg, &dev_rx_offload);
>>  			if (ret != 0) {
>> @@ -2450,16 +2546,176 @@ create_default_ipsec_flow(uint16_t port_id, uint64_t rx_offloads)
>>  		port_id);
>>  }
>>
>
> Wouldn't it be more natural to have these 2 functions below
> (eh_conf_init(), eh_conf_uninit()) defined inside event_helper.c?
>

[Lukasz] I will move these functions to event_helper.c.

>> +static struct eh_conf *
>> +eh_conf_init(void)
>> +{
>> +	struct eventmode_conf *em_conf = NULL;
>> +	struct eh_conf *conf = NULL;
>> +	unsigned int eth_core_id;
>> +	uint32_t nb_bytes;
>> +	void *mem = NULL;
>> +
>> +	/* Allocate memory for config */
>> +	conf = calloc(1, sizeof(struct eh_conf));
>> +	if (conf == NULL) {
>> +		printf("Failed to allocate memory for eventmode helper conf");
>> +		goto err;
>> +	}
>> +
>> +	/* Set default conf */
>> +
>> +	/* Packet transfer mode: poll */
>> +	conf->mode = EH_PKT_TRANSFER_MODE_POLL;
>> +	conf->ipsec_mode = EH_IPSEC_MODE_TYPE_APP;
>> +
>> +	/* Keep all ethernet ports enabled by default */
>> +	conf->eth_portmask = -1;
>> +
>> +	/* Allocate memory for event mode params */
>> +	conf->mode_params = calloc(1, sizeof(struct eventmode_conf));
>> +	if (conf->mode_params == NULL) {
>> +		printf("Failed to allocate memory for event mode params");
>> +		goto err;
>> +	}
>> +
>> +	/* Get eventmode conf */
>> +	em_conf = conf->mode_params;
>> +
>> +	/* Allocate and initialize bitmap for eth cores */
>> +	nb_bytes = rte_bitmap_get_memory_footprint(RTE_MAX_LCORE);
>> +	if (!nb_bytes) {
>> +		printf("Failed to get bitmap footprint");
>> +		goto err;
>> +	}
>> +
>> +	mem = rte_zmalloc("event-helper-ethcore-bitmap", nb_bytes,
>> +			  RTE_CACHE_LINE_SIZE);
>> +	if (!mem) {
>> +		printf("Failed to allocate memory for eth cores bitmap\n");
>> +		goto err;
>> +	}
>> +
>> +	em_conf->eth_core_mask = rte_bitmap_init(RTE_MAX_LCORE, mem, nb_bytes);
>> +	if (!em_conf->eth_core_mask) {
>> +		printf("Failed to initialize bitmap");
>> +		goto err;
>> +	}
>> +
>> +	/* Schedule type: ordered */
>> +	em_conf->ext_params.sched_type = RTE_SCHED_TYPE_ORDERED;
>> +
>> +	/* Set two cores as eth cores for Rx & Tx */
>> +
>> +	/* Use first core other than master core as Rx core */
>> +	eth_core_id = rte_get_next_lcore(0,	/* curr core */
>> +					 1,	/* skip master core */
>> +					 0	/* wrap */);
>> +
>> +	rte_bitmap_set(em_conf->eth_core_mask, eth_core_id);
>> +
>> +	/* Use next core as Tx core */
>> +	eth_core_id = rte_get_next_lcore(eth_core_id,	/* curr core */
>> +					 1,		/* skip master core */
>> +					 0		/* wrap */);
>> +
>> +	rte_bitmap_set(em_conf->eth_core_mask, eth_core_id);
>> +
>> +	return conf;
>> +err:
>> +	rte_free(mem);
>> +	free(em_conf);
>> +	free(conf);
>> +	return NULL;
>> +}
>> +
>> +static void
>> +eh_conf_uninit(struct eh_conf *conf)
>> +{
>> +	struct eventmode_conf *em_conf = NULL;
>> +
>> +	/* Get eventmode conf */
>> +	em_conf = conf->mode_params;
>> +
>> +	/* Free evenmode configuration memory */
>> +	rte_free(em_conf->eth_core_mask);
>> +	free(em_conf);
>> +	free(conf);
>> +}
>> +
>> +static void
>> +signal_handler(int signum)
>> +{
>> +	if (signum == SIGINT || signum == SIGTERM) {
>> +		printf("\n\nSignal %d received, preparing to exit...\n",
>> +				signum);
>> +		force_quit = true;
>> +	}
>> +}
>> +
>> +static void
>> +inline_sessions_free(struct sa_ctx *sa_ctx)
>> +{
>> +	struct rte_ipsec_session *ips;
>> +	struct ipsec_sa *sa;
>> +	int32_t i, ret;
>> +
>> +	for (i = 0; i < IPSEC_SA_MAX_ENTRIES; i++) {
>> +
>> +		sa = &sa_ctx->sa[i];
>> +		if (!sa->spi)
>> +			continue;
>> +
>> +		ips = ipsec_get_primary_session(sa);
>> +		if (ips->type != RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL &&
>> +		    ips->type != RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO)
>> +			continue;
>> +
>> +		ret = rte_security_session_destroy(
>> +				rte_eth_dev_get_sec_ctx(sa->portid),
>> +				ips->security.ses);
>> +		if (ret)
>> +			RTE_LOG(ERR, IPSEC, "Failed to destroy security "
>> +					    "session type %d, spi %d\n",
>> +					    ips->type, sa->spi);
>> +	}
>> +}
>> +
>> +static void
>> +ev_mode_sess_verify(struct sa_ctx *sa_ctx)
>> +{
>> +	struct rte_ipsec_session *ips;
>> +	struct ipsec_sa *sa;
>> +	int32_t i;
>> +
>> +	if (!sa_ctx)
>> +		return;
>> +
>> +	for (i = 0; i < IPSEC_SA_MAX_ENTRIES; i++) {
>> +
>> +		sa = &sa_ctx->sa[i];
>> +		if (!sa->spi)
>> +			continue;
>> +
>> +		ips = ipsec_get_primary_session(sa);
>> +		if (ips->type != RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL)
>> +			rte_exit(EXIT_FAILURE, "Event mode supports only "
>> +				 "inline protocol sessions\n");
>
> As I understand at that moment inline sessions already created on devices?
> For consistency wouldn't it be better to do this check at parsing cfg file,
> or straight after it? 
>

[Lukasz] I will move this check to be done after parsing cfg file into check_eh_conf() function.

>> +	}
>> +
>> +}
>> +
>>  int32_t
>>  main(int32_t argc, char **argv)
>>  {
>>  	int32_t ret;
>>  	uint32_t lcore_id;
>> +	uint32_t cdev_id;
>>  	uint32_t i;
>>  	uint8_t socket_id;
>>  	uint16_t portid;
>>  	uint64_t req_rx_offloads[RTE_MAX_ETHPORTS];
>>  	uint64_t req_tx_offloads[RTE_MAX_ETHPORTS];
>> +	struct eh_conf *eh_conf = NULL;
>>  	size_t sess_sz;
>>
>>  	/* init EAL */
>> @@ -2469,8 +2725,17 @@ main(int32_t argc, char **argv)
>>  	argc -= ret;
>>  	argv += ret;
>>
>> +	force_quit = false;
>> +	signal(SIGINT, signal_handler);
>> +	signal(SIGTERM, signal_handler);
>> +
>> +	/* initialize event helper configuration */
>> +	eh_conf = eh_conf_init();
>> +	if (eh_conf == NULL)
>> +		rte_exit(EXIT_FAILURE, "Failed to init event helper config");
>> +
>>  	/* parse application arguments (after the EAL ones) */
>> -	ret = parse_args(argc, argv);
>> +	ret = parse_args(argc, argv, eh_conf);
>>  	if (ret < 0)
>>  		rte_exit(EXIT_FAILURE, "Invalid parameters\n");
>>
>> @@ -2487,7 +2752,7 @@ main(int32_t argc, char **argv)
>>  		rte_exit(EXIT_FAILURE, "Invalid unprotected portmask 0x%x\n",
>>  				unprotected_port_mask);
>>
>> -	if (check_params() < 0)
>> +	if (check_params(eh_conf) < 0)
>>  		rte_exit(EXIT_FAILURE, "check_params failed\n");
>>
>>  	ret = init_lcore_rx_queues();
>> @@ -2529,6 +2794,18 @@ main(int32_t argc, char **argv)
>>
>>  	cryptodevs_init();
>>
>> +	/*
>> +	 * Set the enabled port mask in helper config for use by helper
>> +	 * sub-system. This will be used while initializing devices using
>> +	 * helper sub-system.
>> +	 */
>> +	eh_conf->eth_portmask = enabled_port_mask;
>> +
>> +	/* Initialize eventmode components */
>> +	ret = eh_devs_init(eh_conf);
>> +	if (ret < 0)
>> +		rte_exit(EXIT_FAILURE, "eh_devs_init failed, err=%d\n", ret);
>> +
>>  	/* start ports */
>>  	RTE_ETH_FOREACH_DEV(portid) {
>>  		if ((enabled_port_mask & (1 << portid)) == 0)
>> @@ -2576,6 +2853,18 @@ main(int32_t argc, char **argv)
>>  			sp4_init(&socket_ctx[socket_id], socket_id);
>>  			sp6_init(&socket_ctx[socket_id], socket_id);
>>  			rt_init(&socket_ctx[socket_id], socket_id);
>> +
>> +			/*
>> +			 * Event mode currently supports only inline protocol
>> +			 * sessions. If there are other types of sessions
>> +			 * configured then exit with error.
>> +			 */
>> +			if (eh_conf->mode == EH_PKT_TRANSFER_MODE_EVENT) {
>> +				ev_mode_sess_verify(
>> +						socket_ctx[socket_id].sa_in);
>> +				ev_mode_sess_verify(
>> +						socket_ctx[socket_id].sa_out);
>> +			}
>>  		}
>>  	}
>>
>> @@ -2583,10 +2872,54 @@ main(int32_t argc, char **argv)
>> 	
>>  	/* launch per-lcore init on every lcore */
>>  	rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
>> +
>>  	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>>  		if (rte_eal_wait_lcore(lcore_id) < 0)
>>  			return -1;
>>  	}
>>
>> +	/* Uninitialize eventmode components */
>> +	ret = eh_devs_uninit(eh_conf);
>> +	if (ret < 0)
>> +		rte_exit(EXIT_FAILURE, "eh_devs_uninit failed, err=%d\n", ret);
>> +
>> +	/* Free eventmode configuration memory */
>> +	eh_conf_uninit(eh_conf);
>> +
>> +	/* Destroy inline inbound and outbound sessions */
>> +	for (i = 0; i < NB_SOCKETS && i < rte_socket_count(); i++) {
>> +		socket_id = rte_socket_id_by_idx(i);
>> +		inline_sessions_free(socket_ctx[socket_id].sa_in);
>
> That causes a crash on 2 socket system with the config that uses
> lcores only from the first socket.
>

[Lukasz] I will fix it in V3. Thanks

>> +		inline_sessions_free(socket_ctx[socket_id].sa_out);
>> +	}
>> +
>> +	for (cdev_id = 0; cdev_id < rte_cryptodev_count(); cdev_id++) {
>> +		printf("Closing cryptodev %d...", cdev_id);
>> +		rte_cryptodev_stop(cdev_id);
>> +		rte_cryptodev_close(cdev_id);
>> +		printf(" Done\n");
>> +	}
>> +
>> +	RTE_ETH_FOREACH_DEV(portid) {
>> +		if ((enabled_port_mask & (1 << portid)) == 0)
>> +			continue;
>> +
>> +		printf("Closing port %d...", portid);
>> +		if (flow_info_tbl[portid].rx_def_flow) {
>> +			struct rte_flow_error err;
>> +
>> +			ret = rte_flow_destroy(portid,
>> +				flow_info_tbl[portid].rx_def_flow, &err);
>> +			if (ret)
>> +				RTE_LOG(ERR, IPSEC, "Failed to destroy flow "
>> +					" for port %u, err msg: %s\n", portid,
>> +					err.message);
>> +		}
>> +		rte_eth_dev_stop(portid);
>> +		rte_eth_dev_close(portid);
>> +		printf(" Done\n");
>> +	}
>> +	printf("Bye...\n");
>> +
>>  	return 0;
>>  }
>> diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
>> index 28ff07d..0539aec 100644
>> --- a/examples/ipsec-secgw/ipsec.h
>> +++ b/examples/ipsec-secgw/ipsec.h
>> @@ -153,6 +153,17 @@ struct ipsec_sa {
>>  	struct rte_security_session_conf sess_conf;
>>  } __rte_cache_aligned;
>>
>> +struct sa_ctx {
>> +	void *satbl; /* pointer to array of rte_ipsec_sa objects*/
>> +	struct ipsec_sa sa[IPSEC_SA_MAX_ENTRIES];
>> +	union {
>> +		struct {
>> +			struct rte_crypto_sym_xform a;
>> +			struct rte_crypto_sym_xform b;
>> +		};
>> +	} xf[IPSEC_SA_MAX_ENTRIES];
>> +};
>> +
>>  struct ipsec_mbuf_metadata {
>>  	struct ipsec_sa *sa;
>>  	struct rte_crypto_op cop;
>> diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
>> index c75a5a1..2ec3e17 100644
>> --- a/examples/ipsec-secgw/sa.c
>> +++ b/examples/ipsec-secgw/sa.c
>> @@ -781,17 +781,6 @@ print_one_sa_rule(const struct ipsec_sa *sa, int inbound)
>>  	printf("\n");
>>  }
>>
>> -struct sa_ctx {
>> -	void *satbl; /* pointer to array of rte_ipsec_sa objects*/
>> -	struct ipsec_sa sa[IPSEC_SA_MAX_ENTRIES];
>> -	union {
>> -		struct {
>> -			struct rte_crypto_sym_xform a;
>> -			struct rte_crypto_sym_xform b;
>> -		};
>> -	} xf[IPSEC_SA_MAX_ENTRIES];
>> -};
>> -
>>  static struct sa_ctx *
>>  sa_create(const char *name, int32_t socket_id)
>>  {
>> --
>> 2.7.4
>
Konstantin Ananyev Jan. 30, 2020, 11:13 a.m. UTC | #3
Hi Lukasz,

> >>
> >>  /*
> >>   * RX/TX HW offload capabilities to enable/use on ethernet ports.
> >> @@ -1185,7 +1205,7 @@ main_loop(__attribute__((unused)) void *dummy)
> >>  }
> >>
> >>  static int32_t
> >> -check_params(void)
> >> +check_params(struct eh_conf *eh_conf)
> >>  {
> >>  	uint8_t lcore;
> >>  	uint16_t portid;
> >> @@ -1220,6 +1240,14 @@ check_params(void)
> >>  			return -1;
> >>  		}
> >>  	}
> >> +
> >> +	if (eh_conf->mode == EH_PKT_TRANSFER_MODE_POLL) {
> >> +		if (schedule_type) {
> >> +			printf("error: option --schedule-type applies only to event mode\n");
> >> +			return -1;
> >> +		}
> >> +	}
> >
> > As a nit - might be better to keep check_params() intact,
> > and put this new check above into a separate function?
> > check_eh_conf() or so?
> 
> [Lukasz] I will put the check into new check_eh_conf() function.
> 
> > Another thing it seems a bit clumsy construction to have global var (scheduler_type)
> > just to figure out was particular option present on command line or not.
> > Probably simler way to avoid it - set initially em_conf->ext_params.sched_type to
> > some invalid value (-1 or so). Then after parse args you can check did its value
> > change or not.
> 
> [Lukasz] I will change it in V3.
> 
> > As alternative thought: wouldn't it be better to unite both  --transfer-mode
> > and --schedule-type options into one?
> > Then possible values for this unite option would be:
> > "poll"
> > "event" (expands to "event-ordered")
> > "event-ordered"
> > "event-atomic"
> > "event-parallel"
> > And this situation you are checking above simply wouldn't be possible.
> > Again probably would be easier/simpler for users.
> 
> [Lukasz] I would rather not combine event mode parameters into one for two reason:
> - to be consistent with poll where one configuration item is controlled with one option,
> - if we come up with a need to add a new event mode parameter in future then we
> we will need to split event-ordered back to --transfer-mode and --schedule-type
> to be consistent with how with provide event mode command line options.

I thought for future mods we can just keep adding new types here:
"event-xxx", "poll-yyy", etc.
But if you think separate ones is a better approach - I am fine. 
Konstantin
Konstantin Ananyev Jan. 30, 2020, 10:21 p.m. UTC | #4
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Thursday, January 30, 2020 11:13 AM
> To: Lukas Bartosik <lbartosik@marvell.com>; Anoob Joseph <anoobj@marvell.com>; Akhil Goyal <akhil.goyal@nxp.com>; Nicolau, Radu
> <radu.nicolau@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> Cc: 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: RE: [EXT] RE: [PATCH v2 09/12] examples/ipsec-secgw: add eventmode to ipsec-secgw
> 
> Hi Lukasz,
> 
> > >>
> > >>  /*
> > >>   * RX/TX HW offload capabilities to enable/use on ethernet ports.
> > >> @@ -1185,7 +1205,7 @@ main_loop(__attribute__((unused)) void *dummy)
> > >>  }
> > >>
> > >>  static int32_t
> > >> -check_params(void)
> > >> +check_params(struct eh_conf *eh_conf)
> > >>  {
> > >>  	uint8_t lcore;
> > >>  	uint16_t portid;
> > >> @@ -1220,6 +1240,14 @@ check_params(void)
> > >>  			return -1;
> > >>  		}
> > >>  	}
> > >> +
> > >> +	if (eh_conf->mode == EH_PKT_TRANSFER_MODE_POLL) {
> > >> +		if (schedule_type) {
> > >> +			printf("error: option --schedule-type applies only to event mode\n");
> > >> +			return -1;
> > >> +		}
> > >> +	}
> > >
> > > As a nit - might be better to keep check_params() intact,
> > > and put this new check above into a separate function?
> > > check_eh_conf() or so?
> >
> > [Lukasz] I will put the check into new check_eh_conf() function.
> >
> > > Another thing it seems a bit clumsy construction to have global var (scheduler_type)
> > > just to figure out was particular option present on command line or not.
> > > Probably simler way to avoid it - set initially em_conf->ext_params.sched_type to
> > > some invalid value (-1 or so). Then after parse args you can check did its value
> > > change or not.
> >
> > [Lukasz] I will change it in V3.
> >
> > > As alternative thought: wouldn't it be better to unite both  --transfer-mode
> > > and --schedule-type options into one?
> > > Then possible values for this unite option would be:
> > > "poll"
> > > "event" (expands to "event-ordered")
> > > "event-ordered"
> > > "event-atomic"
> > > "event-parallel"
> > > And this situation you are checking above simply wouldn't be possible.
> > > Again probably would be easier/simpler for users.
> >
> > [Lukasz] I would rather not combine event mode parameters into one for two reason:
> > - to be consistent with poll where one configuration item is controlled with one option,
> > - if we come up with a need to add a new event mode parameter in future then we
> > we will need to split event-ordered back to --transfer-mode and --schedule-type
> > to be consistent with how with provide event mode command line options.
> 
> I thought for future mods we can just keep adding new types here:
> "event-xxx", "poll-yyy", etc.
> But if you think separate ones is a better approach - I am fine.

Probably one extra suggestion - would it make sense to change name for
that option to have 'event' inside?
'--event-scheduler' or so.
Will probably make things a bit more clear.
Lukasz Bartosik Jan. 31, 2020, 1:09 a.m. UTC | #5
Hi Konstantin,

On 30.01.2020 23:21, Ananyev, Konstantin wrote:
> 
> 
>> -----Original Message-----
>> From: Ananyev, Konstantin
>> Sent: Thursday, January 30, 2020 11:13 AM
>> To: Lukas Bartosik <lbartosik@marvell.com>; Anoob Joseph <anoobj@marvell.com>; Akhil Goyal <akhil.goyal@nxp.com>; Nicolau, Radu
>> <radu.nicolau@intel.com>; Thomas Monjalon <thomas@monjalon.net>
>> Cc: 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: RE: [EXT] RE: [PATCH v2 09/12] examples/ipsec-secgw: add eventmode to ipsec-secgw
>>
>> Hi Lukasz,
>>
>>>>>
>>>>>  /*
>>>>>   * RX/TX HW offload capabilities to enable/use on ethernet ports.
>>>>> @@ -1185,7 +1205,7 @@ main_loop(__attribute__((unused)) void *dummy)
>>>>>  }
>>>>>
>>>>>  static int32_t
>>>>> -check_params(void)
>>>>> +check_params(struct eh_conf *eh_conf)
>>>>>  {
>>>>>  	uint8_t lcore;
>>>>>  	uint16_t portid;
>>>>> @@ -1220,6 +1240,14 @@ check_params(void)
>>>>>  			return -1;
>>>>>  		}
>>>>>  	}
>>>>> +
>>>>> +	if (eh_conf->mode == EH_PKT_TRANSFER_MODE_POLL) {
>>>>> +		if (schedule_type) {
>>>>> +			printf("error: option --schedule-type applies only to event mode\n");
>>>>> +			return -1;
>>>>> +		}
>>>>> +	}
>>>>
>>>> As a nit - might be better to keep check_params() intact,
>>>> and put this new check above into a separate function?
>>>> check_eh_conf() or so?
>>>
>>> [Lukasz] I will put the check into new check_eh_conf() function.
>>>
>>>> Another thing it seems a bit clumsy construction to have global var (scheduler_type)
>>>> just to figure out was particular option present on command line or not.
>>>> Probably simler way to avoid it - set initially em_conf->ext_params.sched_type to
>>>> some invalid value (-1 or so). Then after parse args you can check did its value
>>>> change or not.
>>>
>>> [Lukasz] I will change it in V3.
>>>
>>>> As alternative thought: wouldn't it be better to unite both  --transfer-mode
>>>> and --schedule-type options into one?
>>>> Then possible values for this unite option would be:
>>>> "poll"
>>>> "event" (expands to "event-ordered")
>>>> "event-ordered"
>>>> "event-atomic"
>>>> "event-parallel"
>>>> And this situation you are checking above simply wouldn't be possible.
>>>> Again probably would be easier/simpler for users.
>>>
>>> [Lukasz] I would rather not combine event mode parameters into one for two reason:
>>> - to be consistent with poll where one configuration item is controlled with one option,
>>> - if we come up with a need to add a new event mode parameter in future then we
>>> we will need to split event-ordered back to --transfer-mode and --schedule-type
>>> to be consistent with how with provide event mode command line options.
>>
>> I thought for future mods we can just keep adding new types here:
>> "event-xxx", "poll-yyy", etc.
>> But if you think separate ones is a better approach - I am fine.
> 
> Probably one extra suggestion - would it make sense to change name for
> that option to have 'event' inside?
> '--event-scheduler' or so.
> Will probably make things a bit more clear.
[Lukasz] I will rename option --schedule-type to --event-scheduler
Lukasz Bartosik Feb. 2, 2020, 11 p.m. UTC | #6
Hi Konstantin,

On 31.01.2020 02:09, Lukasz Bartosik wrote:
> Hi Konstantin,
> 
> On 30.01.2020 23:21, Ananyev, Konstantin wrote:
>>
>>
>>> -----Original Message-----
>>> From: Ananyev, Konstantin
>>> Sent: Thursday, January 30, 2020 11:13 AM
>>> To: Lukas Bartosik <lbartosik@marvell.com>; Anoob Joseph <anoobj@marvell.com>; Akhil Goyal <akhil.goyal@nxp.com>; Nicolau, Radu
>>> <radu.nicolau@intel.com>; Thomas Monjalon <thomas@monjalon.net>
>>> Cc: 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: RE: [EXT] RE: [PATCH v2 09/12] examples/ipsec-secgw: add eventmode to ipsec-secgw
>>>
>>> Hi Lukasz,
>>>
>>>>>>
>>>>>>  /*
>>>>>>   * RX/TX HW offload capabilities to enable/use on ethernet ports.
>>>>>> @@ -1185,7 +1205,7 @@ main_loop(__attribute__((unused)) void *dummy)
>>>>>>  }
>>>>>>
>>>>>>  static int32_t
>>>>>> -check_params(void)
>>>>>> +check_params(struct eh_conf *eh_conf)
>>>>>>  {
>>>>>>  	uint8_t lcore;
>>>>>>  	uint16_t portid;
>>>>>> @@ -1220,6 +1240,14 @@ check_params(void)
>>>>>>  			return -1;
>>>>>>  		}
>>>>>>  	}
>>>>>> +
>>>>>> +	if (eh_conf->mode == EH_PKT_TRANSFER_MODE_POLL) {
>>>>>> +		if (schedule_type) {
>>>>>> +			printf("error: option --schedule-type applies only to event mode\n");
>>>>>> +			return -1;
>>>>>> +		}
>>>>>> +	}
>>>>>
>>>>> As a nit - might be better to keep check_params() intact,
>>>>> and put this new check above into a separate function?
>>>>> check_eh_conf() or so?
>>>>
>>>> [Lukasz] I will put the check into new check_eh_conf() function.
>>>>
>>>>> Another thing it seems a bit clumsy construction to have global var (scheduler_type)
>>>>> just to figure out was particular option present on command line or not.
>>>>> Probably simler way to avoid it - set initially em_conf->ext_params.sched_type to
>>>>> some invalid value (-1 or so). Then after parse args you can check did its value
>>>>> change or not.
>>>>
>>>> [Lukasz] I will change it in V3.
>>>>
>>>>> As alternative thought: wouldn't it be better to unite both  --transfer-mode
>>>>> and --schedule-type options into one?
>>>>> Then possible values for this unite option would be:
>>>>> "poll"
>>>>> "event" (expands to "event-ordered")
>>>>> "event-ordered"
>>>>> "event-atomic"
>>>>> "event-parallel"
>>>>> And this situation you are checking above simply wouldn't be possible.
>>>>> Again probably would be easier/simpler for users.
>>>>
>>>> [Lukasz] I would rather not combine event mode parameters into one for two reason:
>>>> - to be consistent with poll where one configuration item is controlled with one option,
>>>> - if we come up with a need to add a new event mode parameter in future then we
>>>> we will need to split event-ordered back to --transfer-mode and --schedule-type
>>>> to be consistent with how with provide event mode command line options.
>>>
>>> I thought for future mods we can just keep adding new types here:
>>> "event-xxx", "poll-yyy", etc.
>>> But if you think separate ones is a better approach - I am fine.
>>
>> Probably one extra suggestion - would it make sense to change name for
>> that option to have 'event' inside?
>> '--event-scheduler' or so.
>> Will probably make things a bit more clear.
> [Lukasz] I will rename option --schedule-type to --event-scheduler
> 
[Lukasz] After reconsideration my proposal is to change option --schedule-type to --event-schedule-type. Are you ok with that ?
Konstantin Ananyev Feb. 3, 2020, 7:50 a.m. UTC | #7
Hi Lukasz,

> >>>>>>  /*
> >>>>>>   * RX/TX HW offload capabilities to enable/use on ethernet ports.
> >>>>>> @@ -1185,7 +1205,7 @@ main_loop(__attribute__((unused)) void *dummy)
> >>>>>>  }
> >>>>>>
> >>>>>>  static int32_t
> >>>>>> -check_params(void)
> >>>>>> +check_params(struct eh_conf *eh_conf)
> >>>>>>  {
> >>>>>>  	uint8_t lcore;
> >>>>>>  	uint16_t portid;
> >>>>>> @@ -1220,6 +1240,14 @@ check_params(void)
> >>>>>>  			return -1;
> >>>>>>  		}
> >>>>>>  	}
> >>>>>> +
> >>>>>> +	if (eh_conf->mode == EH_PKT_TRANSFER_MODE_POLL) {
> >>>>>> +		if (schedule_type) {
> >>>>>> +			printf("error: option --schedule-type applies only to event mode\n");
> >>>>>> +			return -1;
> >>>>>> +		}
> >>>>>> +	}
> >>>>>
> >>>>> As a nit - might be better to keep check_params() intact,
> >>>>> and put this new check above into a separate function?
> >>>>> check_eh_conf() or so?
> >>>>
> >>>> [Lukasz] I will put the check into new check_eh_conf() function.
> >>>>
> >>>>> Another thing it seems a bit clumsy construction to have global var (scheduler_type)
> >>>>> just to figure out was particular option present on command line or not.
> >>>>> Probably simler way to avoid it - set initially em_conf->ext_params.sched_type to
> >>>>> some invalid value (-1 or so). Then after parse args you can check did its value
> >>>>> change or not.
> >>>>
> >>>> [Lukasz] I will change it in V3.
> >>>>
> >>>>> As alternative thought: wouldn't it be better to unite both  --transfer-mode
> >>>>> and --schedule-type options into one?
> >>>>> Then possible values for this unite option would be:
> >>>>> "poll"
> >>>>> "event" (expands to "event-ordered")
> >>>>> "event-ordered"
> >>>>> "event-atomic"
> >>>>> "event-parallel"
> >>>>> And this situation you are checking above simply wouldn't be possible.
> >>>>> Again probably would be easier/simpler for users.
> >>>>
> >>>> [Lukasz] I would rather not combine event mode parameters into one for two reason:
> >>>> - to be consistent with poll where one configuration item is controlled with one option,
> >>>> - if we come up with a need to add a new event mode parameter in future then we
> >>>> we will need to split event-ordered back to --transfer-mode and --schedule-type
> >>>> to be consistent with how with provide event mode command line options.
> >>>
> >>> I thought for future mods we can just keep adding new types here:
> >>> "event-xxx", "poll-yyy", etc.
> >>> But if you think separate ones is a better approach - I am fine.
> >>
> >> Probably one extra suggestion - would it make sense to change name for
> >> that option to have 'event' inside?
> >> '--event-scheduler' or so.
> >> Will probably make things a bit more clear.
> > [Lukasz] I will rename option --schedule-type to --event-scheduler
> >
> [Lukasz] After reconsideration my proposal is to change option --schedule-type to --event-schedule-type. Are you ok with that ?

Yes, sounds ok to me.

Patch
diff mbox series

diff --git a/examples/ipsec-secgw/event_helper.c b/examples/ipsec-secgw/event_helper.c
index 9719ab4..54a98c9 100644
--- a/examples/ipsec-secgw/event_helper.c
+++ b/examples/ipsec-secgw/event_helper.c
@@ -966,6 +966,8 @@  eh_find_worker(uint32_t lcore_id, struct eh_conf *conf,
 	else
 		curr_conf.cap.burst = EH_RX_TYPE_NON_BURST;
 
+	curr_conf.cap.ipsec_mode = conf->ipsec_mode;
+
 	/* Parse the passed list and see if we have matching capabilities */
 
 	/* Initialize the pointer used to traverse the list */
@@ -1625,7 +1627,7 @@  eh_launch_worker(struct eh_conf *conf, struct eh_app_worker_params *app_wrkr,
 	}
 
 	/* Get eventmode conf */
-	em_conf = (struct eventmode_conf *)(conf->mode_params);
+	em_conf = conf->mode_params;
 
 	/* Get core ID */
 	lcore_id = rte_lcore_id();
diff --git a/examples/ipsec-secgw/event_helper.h b/examples/ipsec-secgw/event_helper.h
index 15a7bd6..cf5d346 100644
--- a/examples/ipsec-secgw/event_helper.h
+++ b/examples/ipsec-secgw/event_helper.h
@@ -74,6 +74,14 @@  enum eh_tx_types {
 	EH_TX_TYPE_NO_INTERNAL_PORT
 };
 
+/**
+ * Event mode ipsec mode types
+ */
+enum eh_ipsec_mode_types {
+	EH_IPSEC_MODE_TYPE_APP = 0,
+	EH_IPSEC_MODE_TYPE_DRIVER
+};
+
 /* Event dev params */
 struct eventdev_params {
 	uint8_t eventdev_id;
@@ -183,6 +191,10 @@  struct eh_conf {
 		 */
 	void *mode_params;
 		/**< Mode specific parameters */
+
+		/** Application specific params */
+	enum eh_ipsec_mode_types ipsec_mode;
+		/**< Mode of ipsec run */
 };
 
 /* Workers registered by the application */
@@ -194,6 +206,8 @@  struct eh_app_worker_params {
 			/**< Specify status of rx type burst */
 			uint64_t tx_internal_port : 1;
 			/**< Specify whether tx internal port is available */
+			uint64_t ipsec_mode : 1;
+			/**< Specify ipsec processing level */
 		};
 		uint64_t u64;
 	} cap;
diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
index d5e8fe5..f1cc3fb 100644
--- a/examples/ipsec-secgw/ipsec-secgw.c
+++ b/examples/ipsec-secgw/ipsec-secgw.c
@@ -2,6 +2,7 @@ 
  * Copyright(c) 2016 Intel Corporation
  */
 
+#include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdint.h>
@@ -14,6 +15,7 @@ 
 #include <sys/queue.h>
 #include <stdarg.h>
 #include <errno.h>
+#include <signal.h>
 #include <getopt.h>
 
 #include <rte_common.h>
@@ -41,12 +43,17 @@ 
 #include <rte_jhash.h>
 #include <rte_cryptodev.h>
 #include <rte_security.h>
+#include <rte_bitmap.h>
+#include <rte_eventdev.h>
 #include <rte_ip.h>
 #include <rte_ip_frag.h>
 
+#include "event_helper.h"
 #include "ipsec.h"
 #include "parser.h"
 
+volatile bool force_quit;
+
 #define RTE_LOGTYPE_IPSEC RTE_LOGTYPE_USER1
 
 #define MAX_JUMBO_PKT_LEN  9600
@@ -133,12 +140,20 @@  struct flow_info flow_info_tbl[RTE_MAX_ETHPORTS];
 #define CMD_LINE_OPT_CONFIG		"config"
 #define CMD_LINE_OPT_SINGLE_SA		"single-sa"
 #define CMD_LINE_OPT_CRYPTODEV_MASK	"cryptodev_mask"
+#define CMD_LINE_OPT_TRANSFER_MODE	"transfer-mode"
+#define CMD_LINE_OPT_SCHEDULE_TYPE	"schedule-type"
 #define CMD_LINE_OPT_RX_OFFLOAD		"rxoffload"
 #define CMD_LINE_OPT_TX_OFFLOAD		"txoffload"
 #define CMD_LINE_OPT_REASSEMBLE		"reassemble"
 #define CMD_LINE_OPT_MTU		"mtu"
 #define CMD_LINE_OPT_FRAG_TTL		"frag-ttl"
 
+#define CMD_LINE_ARG_EVENT	"event"
+#define CMD_LINE_ARG_POLL	"poll"
+#define CMD_LINE_ARG_ORDERED	"ordered"
+#define CMD_LINE_ARG_ATOMIC	"atomic"
+#define CMD_LINE_ARG_PARALLEL	"parallel"
+
 enum {
 	/* long options mapped to a short option */
 
@@ -149,6 +164,8 @@  enum {
 	CMD_LINE_OPT_CONFIG_NUM,
 	CMD_LINE_OPT_SINGLE_SA_NUM,
 	CMD_LINE_OPT_CRYPTODEV_MASK_NUM,
+	CMD_LINE_OPT_TRANSFER_MODE_NUM,
+	CMD_LINE_OPT_SCHEDULE_TYPE_NUM,
 	CMD_LINE_OPT_RX_OFFLOAD_NUM,
 	CMD_LINE_OPT_TX_OFFLOAD_NUM,
 	CMD_LINE_OPT_REASSEMBLE_NUM,
@@ -160,6 +177,8 @@  static const struct option lgopts[] = {
 	{CMD_LINE_OPT_CONFIG, 1, 0, CMD_LINE_OPT_CONFIG_NUM},
 	{CMD_LINE_OPT_SINGLE_SA, 1, 0, CMD_LINE_OPT_SINGLE_SA_NUM},
 	{CMD_LINE_OPT_CRYPTODEV_MASK, 1, 0, CMD_LINE_OPT_CRYPTODEV_MASK_NUM},
+	{CMD_LINE_OPT_TRANSFER_MODE, 1, 0, CMD_LINE_OPT_TRANSFER_MODE_NUM},
+	{CMD_LINE_OPT_SCHEDULE_TYPE, 1, 0, CMD_LINE_OPT_SCHEDULE_TYPE_NUM},
 	{CMD_LINE_OPT_RX_OFFLOAD, 1, 0, CMD_LINE_OPT_RX_OFFLOAD_NUM},
 	{CMD_LINE_OPT_TX_OFFLOAD, 1, 0, CMD_LINE_OPT_TX_OFFLOAD_NUM},
 	{CMD_LINE_OPT_REASSEMBLE, 1, 0, CMD_LINE_OPT_REASSEMBLE_NUM},
@@ -177,6 +196,7 @@  static int32_t numa_on = 1; /**< NUMA is enabled by default. */
 static uint32_t nb_lcores;
 static uint32_t single_sa;
 static uint32_t single_sa_idx;
+static uint32_t schedule_type;
 
 /*
  * RX/TX HW offload capabilities to enable/use on ethernet ports.
@@ -1185,7 +1205,7 @@  main_loop(__attribute__((unused)) void *dummy)
 }
 
 static int32_t
-check_params(void)
+check_params(struct eh_conf *eh_conf)
 {
 	uint8_t lcore;
 	uint16_t portid;
@@ -1220,6 +1240,14 @@  check_params(void)
 			return -1;
 		}
 	}
+
+	if (eh_conf->mode == EH_PKT_TRANSFER_MODE_POLL) {
+		if (schedule_type) {
+			printf("error: option --schedule-type applies only to event mode\n");
+			return -1;
+		}
+	}
+
 	return 0;
 }
 
@@ -1277,6 +1305,8 @@  print_usage(const char *prgname)
 		" --config (port,queue,lcore)[,(port,queue,lcore)]"
 		" [--single-sa SAIDX]"
 		" [--cryptodev_mask MASK]"
+		" [--transfer-mode MODE]"
+		" [--schedule-type TYPE]"
 		" [--" CMD_LINE_OPT_RX_OFFLOAD " RX_OFFLOAD_MASK]"
 		" [--" CMD_LINE_OPT_TX_OFFLOAD " TX_OFFLOAD_MASK]"
 		" [--" CMD_LINE_OPT_REASSEMBLE " REASSEMBLE_TABLE_SIZE]"
@@ -1298,6 +1328,14 @@  print_usage(const char *prgname)
 		"                     bypassing the SP\n"
 		"  --cryptodev_mask MASK: Hexadecimal bitmask of the crypto\n"
 		"                         devices to configure\n"
+		"  --transfer-mode MODE\n"
+		"               \"poll\"  : Packet transfer via polling (default)\n"
+		"               \"event\" : Packet transfer via event device\n"
+		"  --schedule-type TYPE queue schedule type, used only when\n"
+		"                       transfer mode is set to event\n"
+		"               \"ordered\"  : Ordered (default)\n"
+		"               \"atomic\"   : Atomic\n"
+		"               \"parallel\" : Parallel\n"
 		"  --" CMD_LINE_OPT_RX_OFFLOAD
 		": bitmask of the RX HW offload capabilities to enable/use\n"
 		"                         (DEV_RX_OFFLOAD_*)\n"
@@ -1432,8 +1470,45 @@  print_app_sa_prm(const struct app_sa_prm *prm)
 	printf("Frag TTL: %" PRIu64 " ns\n", frag_ttl_ns);
 }
 
+static int
+parse_transfer_mode(struct eh_conf *conf, const char *optarg)
+{
+	if (!strcmp(CMD_LINE_ARG_POLL, optarg))
+		conf->mode = EH_PKT_TRANSFER_MODE_POLL;
+	else if (!strcmp(CMD_LINE_ARG_EVENT, optarg))
+		conf->mode = EH_PKT_TRANSFER_MODE_EVENT;
+	else {
+		printf("Unsupported packet transfer mode\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int
+parse_schedule_type(struct eh_conf *conf, const char *optarg)
+{
+	struct eventmode_conf *em_conf = NULL;
+
+	/* Get eventmode conf */
+	em_conf = conf->mode_params;
+
+	if (!strcmp(CMD_LINE_ARG_ORDERED, optarg))
+		em_conf->ext_params.sched_type = RTE_SCHED_TYPE_ORDERED;
+	else if (!strcmp(CMD_LINE_ARG_ATOMIC, optarg))
+		em_conf->ext_params.sched_type = RTE_SCHED_TYPE_ATOMIC;
+	else if (!strcmp(CMD_LINE_ARG_PARALLEL, optarg))
+		em_conf->ext_params.sched_type = RTE_SCHED_TYPE_PARALLEL;
+	else {
+		printf("Unsupported queue schedule type\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int32_t
-parse_args(int32_t argc, char **argv)
+parse_args(int32_t argc, char **argv, struct eh_conf *eh_conf)
 {
 	int opt;
 	int64_t ret;
@@ -1522,6 +1597,7 @@  parse_args(int32_t argc, char **argv)
 			/* else */
 			single_sa = 1;
 			single_sa_idx = ret;
+			eh_conf->ipsec_mode = EH_IPSEC_MODE_TYPE_DRIVER;
 			printf("Configured with single SA index %u\n",
 					single_sa_idx);
 			break;
@@ -1536,6 +1612,26 @@  parse_args(int32_t argc, char **argv)
 			/* else */
 			enabled_cryptodev_mask = ret;
 			break;
+
+		case CMD_LINE_OPT_TRANSFER_MODE_NUM:
+			ret = parse_transfer_mode(eh_conf, optarg);
+			if (ret < 0) {
+				printf("Invalid packet transfer mode\n");
+				print_usage(prgname);
+				return -1;
+			}
+			break;
+
+		case CMD_LINE_OPT_SCHEDULE_TYPE_NUM:
+			ret = parse_schedule_type(eh_conf, optarg);
+			if (ret < 0) {
+				printf("Invalid queue schedule type\n");
+				print_usage(prgname);
+				return -1;
+			}
+			schedule_type = 1;
+			break;
+
 		case CMD_LINE_OPT_RX_OFFLOAD_NUM:
 			ret = parse_mask(optarg, &dev_rx_offload);
 			if (ret != 0) {
@@ -2450,16 +2546,176 @@  create_default_ipsec_flow(uint16_t port_id, uint64_t rx_offloads)
 		port_id);
 }
 
+static struct eh_conf *
+eh_conf_init(void)
+{
+	struct eventmode_conf *em_conf = NULL;
+	struct eh_conf *conf = NULL;
+	unsigned int eth_core_id;
+	uint32_t nb_bytes;
+	void *mem = NULL;
+
+	/* Allocate memory for config */
+	conf = calloc(1, sizeof(struct eh_conf));
+	if (conf == NULL) {
+		printf("Failed to allocate memory for eventmode helper conf");
+		goto err;
+	}
+
+	/* Set default conf */
+
+	/* Packet transfer mode: poll */
+	conf->mode = EH_PKT_TRANSFER_MODE_POLL;
+	conf->ipsec_mode = EH_IPSEC_MODE_TYPE_APP;
+
+	/* Keep all ethernet ports enabled by default */
+	conf->eth_portmask = -1;
+
+	/* Allocate memory for event mode params */
+	conf->mode_params = calloc(1, sizeof(struct eventmode_conf));
+	if (conf->mode_params == NULL) {
+		printf("Failed to allocate memory for event mode params");
+		goto err;
+	}
+
+	/* Get eventmode conf */
+	em_conf = conf->mode_params;
+
+	/* Allocate and initialize bitmap for eth cores */
+	nb_bytes = rte_bitmap_get_memory_footprint(RTE_MAX_LCORE);
+	if (!nb_bytes) {
+		printf("Failed to get bitmap footprint");
+		goto err;
+	}
+
+	mem = rte_zmalloc("event-helper-ethcore-bitmap", nb_bytes,
+			  RTE_CACHE_LINE_SIZE);
+	if (!mem) {
+		printf("Failed to allocate memory for eth cores bitmap\n");
+		goto err;
+	}
+
+	em_conf->eth_core_mask = rte_bitmap_init(RTE_MAX_LCORE, mem, nb_bytes);
+	if (!em_conf->eth_core_mask) {
+		printf("Failed to initialize bitmap");
+		goto err;
+	}
+
+	/* Schedule type: ordered */
+	em_conf->ext_params.sched_type = RTE_SCHED_TYPE_ORDERED;
+
+	/* Set two cores as eth cores for Rx & Tx */
+
+	/* Use first core other than master core as Rx core */
+	eth_core_id = rte_get_next_lcore(0,	/* curr core */
+					 1,	/* skip master core */
+					 0	/* wrap */);
+
+	rte_bitmap_set(em_conf->eth_core_mask, eth_core_id);
+
+	/* Use next core as Tx core */
+	eth_core_id = rte_get_next_lcore(eth_core_id,	/* curr core */
+					 1,		/* skip master core */
+					 0		/* wrap */);
+
+	rte_bitmap_set(em_conf->eth_core_mask, eth_core_id);
+
+	return conf;
+err:
+	rte_free(mem);
+	free(em_conf);
+	free(conf);
+	return NULL;
+}
+
+static void
+eh_conf_uninit(struct eh_conf *conf)
+{
+	struct eventmode_conf *em_conf = NULL;
+
+	/* Get eventmode conf */
+	em_conf = conf->mode_params;
+
+	/* Free evenmode configuration memory */
+	rte_free(em_conf->eth_core_mask);
+	free(em_conf);
+	free(conf);
+}
+
+static void
+signal_handler(int signum)
+{
+	if (signum == SIGINT || signum == SIGTERM) {
+		printf("\n\nSignal %d received, preparing to exit...\n",
+				signum);
+		force_quit = true;
+	}
+}
+
+static void
+inline_sessions_free(struct sa_ctx *sa_ctx)
+{
+	struct rte_ipsec_session *ips;
+	struct ipsec_sa *sa;
+	int32_t i, ret;
+
+	for (i = 0; i < IPSEC_SA_MAX_ENTRIES; i++) {
+
+		sa = &sa_ctx->sa[i];
+		if (!sa->spi)
+			continue;
+
+		ips = ipsec_get_primary_session(sa);
+		if (ips->type != RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL &&
+		    ips->type != RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO)
+			continue;
+
+		ret = rte_security_session_destroy(
+				rte_eth_dev_get_sec_ctx(sa->portid),
+				ips->security.ses);
+		if (ret)
+			RTE_LOG(ERR, IPSEC, "Failed to destroy security "
+					    "session type %d, spi %d\n",
+					    ips->type, sa->spi);
+	}
+}
+
+static void
+ev_mode_sess_verify(struct sa_ctx *sa_ctx)
+{
+	struct rte_ipsec_session *ips;
+	struct ipsec_sa *sa;
+	int32_t i;
+
+	if (!sa_ctx)
+		return;
+
+	for (i = 0; i < IPSEC_SA_MAX_ENTRIES; i++) {
+
+		sa = &sa_ctx->sa[i];
+		if (!sa->spi)
+			continue;
+
+		ips = ipsec_get_primary_session(sa);
+		if (ips->type != RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL)
+			rte_exit(EXIT_FAILURE, "Event mode supports only "
+				 "inline protocol sessions\n");
+	}
+
+}
+
 int32_t
 main(int32_t argc, char **argv)
 {
 	int32_t ret;
 	uint32_t lcore_id;
+	uint32_t cdev_id;
 	uint32_t i;
 	uint8_t socket_id;
 	uint16_t portid;
 	uint64_t req_rx_offloads[RTE_MAX_ETHPORTS];
 	uint64_t req_tx_offloads[RTE_MAX_ETHPORTS];
+	struct eh_conf *eh_conf = NULL;
 	size_t sess_sz;
 
 	/* init EAL */
@@ -2469,8 +2725,17 @@  main(int32_t argc, char **argv)
 	argc -= ret;
 	argv += ret;
 
+	force_quit = false;
+	signal(SIGINT, signal_handler);
+	signal(SIGTERM, signal_handler);
+
+	/* initialize event helper configuration */
+	eh_conf = eh_conf_init();
+	if (eh_conf == NULL)
+		rte_exit(EXIT_FAILURE, "Failed to init event helper config");
+
 	/* parse application arguments (after the EAL ones) */
-	ret = parse_args(argc, argv);
+	ret = parse_args(argc, argv, eh_conf);
 	if (ret < 0)
 		rte_exit(EXIT_FAILURE, "Invalid parameters\n");
 
@@ -2487,7 +2752,7 @@  main(int32_t argc, char **argv)
 		rte_exit(EXIT_FAILURE, "Invalid unprotected portmask 0x%x\n",
 				unprotected_port_mask);
 
-	if (check_params() < 0)
+	if (check_params(eh_conf) < 0)
 		rte_exit(EXIT_FAILURE, "check_params failed\n");
 
 	ret = init_lcore_rx_queues();
@@ -2529,6 +2794,18 @@  main(int32_t argc, char **argv)
 
 	cryptodevs_init();
 
+	/*
+	 * Set the enabled port mask in helper config for use by helper
+	 * sub-system. This will be used while initializing devices using
+	 * helper sub-system.
+	 */
+	eh_conf->eth_portmask = enabled_port_mask;
+
+	/* Initialize eventmode components */
+	ret = eh_devs_init(eh_conf);
+	if (ret < 0)
+		rte_exit(EXIT_FAILURE, "eh_devs_init failed, err=%d\n", ret);
+
 	/* start ports */
 	RTE_ETH_FOREACH_DEV(portid) {
 		if ((enabled_port_mask & (1 << portid)) == 0)
@@ -2576,6 +2853,18 @@  main(int32_t argc, char **argv)
 			sp4_init(&socket_ctx[socket_id], socket_id);
 			sp6_init(&socket_ctx[socket_id], socket_id);
 			rt_init(&socket_ctx[socket_id], socket_id);
+
+			/*
+			 * Event mode currently supports only inline protocol
+			 * sessions. If there are other types of sessions
+			 * configured then exit with error.
+			 */
+			if (eh_conf->mode == EH_PKT_TRANSFER_MODE_EVENT) {
+				ev_mode_sess_verify(
+						socket_ctx[socket_id].sa_in);
+				ev_mode_sess_verify(
+						socket_ctx[socket_id].sa_out);
+			}
 		}
 	}
 
@@ -2583,10 +2872,54 @@  main(int32_t argc, char **argv)
 
 	/* launch per-lcore init on every lcore */
 	rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
+
 	RTE_LCORE_FOREACH_SLAVE(lcore_id) {
 		if (rte_eal_wait_lcore(lcore_id) < 0)
 			return -1;
 	}
 
+	/* Uninitialize eventmode components */
+	ret = eh_devs_uninit(eh_conf);
+	if (ret < 0)
+		rte_exit(EXIT_FAILURE, "eh_devs_uninit failed, err=%d\n", ret);
+
+	/* Free eventmode configuration memory */
+	eh_conf_uninit(eh_conf);
+
+	/* Destroy inline inbound and outbound sessions */
+	for (i = 0; i < NB_SOCKETS && i < rte_socket_count(); i++) {
+		socket_id = rte_socket_id_by_idx(i);
+		inline_sessions_free(socket_ctx[socket_id].sa_in);
+		inline_sessions_free(socket_ctx[socket_id].sa_out);
+	}
+
+	for (cdev_id = 0; cdev_id < rte_cryptodev_count(); cdev_id++) {
+		printf("Closing cryptodev %d...", cdev_id);
+		rte_cryptodev_stop(cdev_id);
+		rte_cryptodev_close(cdev_id);
+		printf(" Done\n");
+	}
+
+	RTE_ETH_FOREACH_DEV(portid) {
+		if ((enabled_port_mask & (1 << portid)) == 0)
+			continue;
+
+		printf("Closing port %d...", portid);
+		if (flow_info_tbl[portid].rx_def_flow) {
+			struct rte_flow_error err;
+
+			ret = rte_flow_destroy(portid,
+				flow_info_tbl[portid].rx_def_flow, &err);
+			if (ret)
+				RTE_LOG(ERR, IPSEC, "Failed to destroy flow "
+					" for port %u, err msg: %s\n", portid,
+					err.message);
+		}
+		rte_eth_dev_stop(portid);
+		rte_eth_dev_close(portid);
+		printf(" Done\n");
+	}
+	printf("Bye...\n");
+
 	return 0;
 }
diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
index 28ff07d..0539aec 100644
--- a/examples/ipsec-secgw/ipsec.h
+++ b/examples/ipsec-secgw/ipsec.h
@@ -153,6 +153,17 @@  struct ipsec_sa {
 	struct rte_security_session_conf sess_conf;
 } __rte_cache_aligned;
 
+struct sa_ctx {
+	void *satbl; /* pointer to array of rte_ipsec_sa objects*/
+	struct ipsec_sa sa[IPSEC_SA_MAX_ENTRIES];
+	union {
+		struct {
+			struct rte_crypto_sym_xform a;
+			struct rte_crypto_sym_xform b;
+		};
+	} xf[IPSEC_SA_MAX_ENTRIES];
+};
+
 struct ipsec_mbuf_metadata {
 	struct ipsec_sa *sa;
 	struct rte_crypto_op cop;
diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
index c75a5a1..2ec3e17 100644
--- a/examples/ipsec-secgw/sa.c
+++ b/examples/ipsec-secgw/sa.c
@@ -781,17 +781,6 @@  print_one_sa_rule(const struct ipsec_sa *sa, int inbound)
 	printf("\n");
 }
 
-struct sa_ctx {
-	void *satbl; /* pointer to array of rte_ipsec_sa objects*/
-	struct ipsec_sa sa[IPSEC_SA_MAX_ENTRIES];
-	union {
-		struct {
-			struct rte_crypto_sym_xform a;
-			struct rte_crypto_sym_xform b;
-		};
-	} xf[IPSEC_SA_MAX_ENTRIES];
-};
-
 static struct sa_ctx *
 sa_create(const char *name, int32_t socket_id)
 {