Re: [dpdk-dev] [PATCH v5 01/10] examples/ipsec-secgw: allow user to disable some RX/TX offloads

[Akhil Goyal <akhil.goyal@nxp.com>]

On 12/28/2018 9:03 PM, Konstantin Ananyev wrote:
> Right now ipsec-secgw always enables TX offloads
> (DEV_TX_OFFLOAD_MULTI_SEGS, DEV_TX_OFFLOAD_SECURITY),
> even when they are not requested by the config.
> That causes many PMD to choose full-featured TX function,
> which in many cases is much slower then one without offloads.
> That patch adds ability for the user to disable unneeded HW offloads.
> If DEV_TX_OFFLOAD_IPV4_CKSUM is disabled by user, then
> SW version of ip cksum calculation is used.
> That allows to use vector TX function, when inline-ipsec is not
> requested.
>
> Signed-off-by: Remy Horton <remy.horton@intel.com>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Acked-by: Radu Nicolau <radu.nicolau@intel.com>
> ---
>   doc/guides/sample_app_ug/ipsec_secgw.rst |  12 +++
>   examples/ipsec-secgw/ipsec-secgw.c       | 126 ++++++++++++++++++++---
>   examples/ipsec-secgw/ipsec.h             |   6 ++
>   examples/ipsec-secgw/sa.c                |  35 +++++++
>   4 files changed, 164 insertions(+), 15 deletions(-)
>
> diff --git a/doc/guides/sample_app_ug/ipsec_secgw.rst b/doc/guides/sample_app_ug/ipsec_secgw.rst
> index 4869a011d..244bde2de 100644
> --- a/doc/guides/sample_app_ug/ipsec_secgw.rst
> +++ b/doc/guides/sample_app_ug/ipsec_secgw.rst
> @@ -95,6 +95,8 @@ The application has a number of command line options::
>                           -p PORTMASK -P -u PORTMASK -j FRAMESIZE
>                           --config (port,queue,lcore)[,(port,queue,lcore]
>                           --single-sa SAIDX
> +                        --rxoffload MASK
> +                        --txoffload MASK
>                           -f CONFIG_FILE_PATH
>   
>   Where:
> @@ -119,6 +121,16 @@ Where:
>       on both Inbound and Outbound. This option is meant for debugging/performance
>       purposes.
>   
> +*   ``--rxoffload MASK``: RX HW offload capabilities to enable/use on this port
> +    (bitmask of DEV_RX_OFFLOAD_* values). It is an optional parameter and
> +    allows user to disable some of the RX HW offload capabilities.
> +    By default all HW RX offloads are enabled.
> +
> +*   ``--txoffload MASK``: TX HW offload capabilities to enable/use on this port
> +    (bitmask of DEV_TX_OFFLOAD_* values). It is an optional parameter and
> +    allows user to disable some of the TX HW offload capabilities.
> +    By default all HW TX offloads are enabled.
> +
by default all should not be enabled. It should remain as it was before 
your patchset.
It would be good if you can add a link to ethdev section where it 
explain different offloads available.

If anyone needs to change the default behavior, a patch need to be 
submitted to add that.
>   *   ``-f CONFIG_FILE_PATH``: the full path of text-based file containing all
>       configuration items for running the application (See Configuration file
>       syntax section below). ``-f CONFIG_FILE_PATH`` **must** be specified.
> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
> index 1bc0b5b50..f5db3da0c 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.c
> +++ b/examples/ipsec-secgw/ipsec-secgw.c
> @@ -124,6 +124,8 @@ struct ethaddr_info ethaddr_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_RX_OFFLOAD		"rxoffload"
> +#define CMD_LINE_OPT_TX_OFFLOAD		"txoffload"
>   
>   enum {
>   	/* long options mapped to a short option */
> @@ -135,12 +137,16 @@ enum {
>   	CMD_LINE_OPT_CONFIG_NUM,
>   	CMD_LINE_OPT_SINGLE_SA_NUM,
>   	CMD_LINE_OPT_CRYPTODEV_MASK_NUM,
> +	CMD_LINE_OPT_RX_OFFLOAD_NUM,
> +	CMD_LINE_OPT_TX_OFFLOAD_NUM,
>   };
>   
>   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_RX_OFFLOAD, 1, 0, CMD_LINE_OPT_RX_OFFLOAD_NUM},
> +	{CMD_LINE_OPT_TX_OFFLOAD, 1, 0, CMD_LINE_OPT_TX_OFFLOAD_NUM},
>   	{NULL, 0, 0, 0}
>   };
>   
> @@ -155,6 +161,13 @@ static uint32_t single_sa;
>   static uint32_t single_sa_idx;
>   static uint32_t frame_size;
>   
> +/*
> + * RX/TX HW offload capabilities to enable/use on ethernet ports.
> + * By default all capabilities are enabled.
> + */
> +static uint64_t dev_rx_offload = UINT64_MAX;
> +static uint64_t dev_tx_offload = UINT64_MAX;
> +
>   struct lcore_rx_queue {
>   	uint16_t port_id;
>   	uint8_t queue_id;
> @@ -208,8 +221,6 @@ static struct rte_eth_conf port_conf = {
>   	},
>   	.txmode = {
>   		.mq_mode = ETH_MQ_TX_NONE,
> -		.offloads = (DEV_TX_OFFLOAD_IPV4_CKSUM |
> -			     DEV_TX_OFFLOAD_MULTI_SEGS),
>   	},
>   };
>   
> @@ -315,7 +326,8 @@ prepare_traffic(struct rte_mbuf **pkts, struct ipsec_traffic *t,
>   }
>   
>   static inline void
> -prepare_tx_pkt(struct rte_mbuf *pkt, uint16_t port)
> +prepare_tx_pkt(struct rte_mbuf *pkt, uint16_t port,
> +		const struct lcore_conf *qconf)
>   {
>   	struct ip *ip;
>   	struct ether_hdr *ethhdr;
> @@ -325,14 +337,19 @@ prepare_tx_pkt(struct rte_mbuf *pkt, uint16_t port)
>   	ethhdr = (struct ether_hdr *)rte_pktmbuf_prepend(pkt, ETHER_HDR_LEN);
>   
>   	if (ip->ip_v == IPVERSION) {
> -		pkt->ol_flags |= PKT_TX_IP_CKSUM | PKT_TX_IPV4;
> +		pkt->ol_flags |= qconf->outbound.ipv4_offloads;
>   		pkt->l3_len = sizeof(struct ip);
>   		pkt->l2_len = ETHER_HDR_LEN;
>   
>   		ip->ip_sum = 0;
> +
> +		/* calculate IPv4 cksum in SW */
> +		if ((pkt->ol_flags & PKT_TX_IP_CKSUM) == 0)
> +			ip->ip_sum = rte_ipv4_cksum((struct ipv4_hdr *)ip);
> +
something is not correct, on my hardware I still see rte_ipv4_cksum 
getting called.
This result in around 2% drop due to this patch.
>   		ethhdr->ether_type = rte_cpu_to_be_16(ETHER_TYPE_IPv4);
>   	} else {
> -		pkt->ol_flags |= PKT_TX_IPV6;
> +		pkt->ol_flags |= qconf->outbound.ipv6_offloads;
>   		pkt->l3_len = sizeof(struct ip6_hdr);
>   		pkt->l2_len = ETHER_HDR_LEN;
>   
> @@ -346,18 +363,19 @@ prepare_tx_pkt(struct rte_mbuf *pkt, uint16_t port)
>   }
>   
>   static inline void
> -prepare_tx_burst(struct rte_mbuf *pkts[], uint16_t nb_pkts, uint16_t port)
> +prepare_tx_burst(struct rte_mbuf *pkts[], uint16_t nb_pkts, uint16_t port,
> +		const struct lcore_conf *qconf)
>   {
>   	int32_t i;
>   	const int32_t prefetch_offset = 2;
>   
>   	for (i = 0; i < (nb_pkts - prefetch_offset); i++) {
>   		rte_mbuf_prefetch_part2(pkts[i + prefetch_offset]);
> -		prepare_tx_pkt(pkts[i], port);
> +		prepare_tx_pkt(pkts[i], port, qconf);
>   	}
>   	/* Process left packets */
>   	for (; i < nb_pkts; i++)
> -		prepare_tx_pkt(pkts[i], port);
> +		prepare_tx_pkt(pkts[i], port, qconf);
>   }
>   
>   /* Send burst of packets on an output interface */
> @@ -371,7 +389,7 @@ send_burst(struct lcore_conf *qconf, uint16_t n, uint16_t port)
>   	queueid = qconf->tx_queue_id[port];
>   	m_table = (struct rte_mbuf **)qconf->tx_mbufs[port].m_table;
>   
> -	prepare_tx_burst(m_table, n, port);
> +	prepare_tx_burst(m_table, n, port, qconf);
>   
>   	ret = rte_eth_tx_burst(port, queueid, m_table, n);
>   	if (unlikely(ret < n)) {
> @@ -954,6 +972,8 @@ print_usage(const char *prgname)
>   		" --config (port,queue,lcore)[,(port,queue,lcore)]"
>   		" [--single-sa SAIDX]"
>   		" [--cryptodev_mask MASK]"
> +		" [--" CMD_LINE_OPT_RX_OFFLOAD " RX_OFFLOAD_MASK]"
> +		" [--" CMD_LINE_OPT_TX_OFFLOAD " TX_OFFLOAD_MASK]"
>   		"\n\n"
>   		"  -p PORTMASK: Hexadecimal bitmask of ports to configure\n"
>   		"  -P : Enable promiscuous mode\n"
> @@ -966,10 +986,31 @@ print_usage(const char *prgname)
>   		"                     bypassing the SP\n"
>   		"  --cryptodev_mask MASK: Hexadecimal bitmask of the crypto\n"
>   		"                         devices to configure\n"
> +		"  --" CMD_LINE_OPT_RX_OFFLOAD
> +		": bitmask of the RX HW offload capabilities to enable/use\n"
> +		"                         (DEV_RX_OFFLOAD_*)\n"
> +		"  --" CMD_LINE_OPT_TX_OFFLOAD
> +		": bitmask of the TX HW offload capabilities to enable/use\n"
> +		"                         (DEV_TX_OFFLOAD_*)\n"
>   		"\n",
>   		prgname);
>   }
>   
> +static int
> +parse_mask(const char *str, uint64_t *val)
> +{
> +	char *end;
> +	unsigned long t;
> +
> +	errno = 0;
> +	t = strtoul(str, &end, 0);
> +	if (errno != 0 || end[0] != 0)
> +		return -EINVAL;
> +
> +	*val = t;
> +	return 0;
> +}
> +
>   static int32_t
>   parse_portmask(const char *portmask)
>   {
> @@ -1156,6 +1197,24 @@ parse_args(int32_t argc, char **argv)
>   			/* else */
>   			enabled_cryptodev_mask = ret;
>   			break;
> +		case CMD_LINE_OPT_RX_OFFLOAD_NUM:
> +			ret = parse_mask(optarg, &dev_rx_offload);
> +			if (ret != 0) {
> +				printf("Invalid argument for \'%s\': %s\n",
> +					CMD_LINE_OPT_RX_OFFLOAD, optarg);
> +				print_usage(prgname);
> +				return -1;
> +			}
> +			break;
> +		case CMD_LINE_OPT_TX_OFFLOAD_NUM:
> +			ret = parse_mask(optarg, &dev_tx_offload);
> +			if (ret != 0) {
> +				printf("Invalid argument for \'%s\': %s\n",
> +					CMD_LINE_OPT_TX_OFFLOAD, optarg);
> +				print_usage(prgname);
> +				return -1;
> +			}
> +			break;
>   		default:
>   			print_usage(prgname);
>   			return -1;
> @@ -1543,7 +1602,7 @@ cryptodevs_init(void)
>   }
>   
>   static void
> -port_init(uint16_t portid)
> +port_init(uint16_t portid, uint64_t req_rx_offloads, uint64_t req_tx_offloads)
>   {
>   	struct rte_eth_dev_info dev_info;
>   	struct rte_eth_txconf *txconf;
> @@ -1556,6 +1615,10 @@ port_init(uint16_t portid)
>   
>   	rte_eth_dev_info_get(portid, &dev_info);
>   
> +	/* limit allowed HW offloafs, as user requested */
> +	dev_info.rx_offload_capa &= dev_rx_offload;
> +	dev_info.tx_offload_capa &= dev_tx_offload;
> +
>   	printf("Configuring device port %u:\n", portid);
>   
>   	rte_eth_macaddr_get(portid, &ethaddr);
> @@ -1584,14 +1647,38 @@ port_init(uint16_t portid)
>   		local_port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
>   	}
>   
> -	if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_SECURITY)
> -		local_port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_SECURITY;
> -	if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_SECURITY)
> -		local_port_conf.txmode.offloads |= DEV_TX_OFFLOAD_SECURITY;
> +	local_port_conf.rxmode.offloads |= req_rx_offloads;
> +	local_port_conf.txmode.offloads |= req_tx_offloads;
> +
> +	/* Check that all required capabilities are supported */
> +	if ((local_port_conf.rxmode.offloads & dev_info.rx_offload_capa) !=
> +			local_port_conf.rxmode.offloads)
> +		rte_exit(EXIT_FAILURE,
> +			"Error: port %u required RX offloads: 0x%" PRIx64
> +			", avaialbe RX offloads: 0x%" PRIx64 "\n",
> +			portid, local_port_conf.rxmode.offloads,
> +			dev_info.rx_offload_capa);
> +
> +	if ((local_port_conf.txmode.offloads & dev_info.tx_offload_capa) !=
> +			local_port_conf.txmode.offloads)
> +		rte_exit(EXIT_FAILURE,
> +			"Error: port %u required TX offloads: 0x%" PRIx64
> +			", avaialbe TX offloads: 0x%" PRIx64 "\n",
> +			portid, local_port_conf.txmode.offloads,
> +			dev_info.tx_offload_capa);
> +
>   	if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_MBUF_FAST_FREE)
>   		local_port_conf.txmode.offloads |=
>   			DEV_TX_OFFLOAD_MBUF_FAST_FREE;
>   
> +	if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_IPV4_CKSUM)
> +		local_port_conf.txmode.offloads |= DEV_TX_OFFLOAD_IPV4_CKSUM;
> +
> +	printf("port %u configurng rx_offloads=0x%" PRIx64
> +		", tx_offloads=0x%" PRIx64 "\n",
> +		portid, local_port_conf.rxmode.offloads,
> +		local_port_conf.txmode.offloads);
> +
>   	local_port_conf.rx_adv_conf.rss_conf.rss_hf &=
>   		dev_info.flow_type_rss_offloads;
>   	if (local_port_conf.rx_adv_conf.rss_conf.rss_hf !=
> @@ -1639,6 +1726,13 @@ port_init(uint16_t portid)
>   
>   		qconf = &lcore_conf[lcore_id];
>   		qconf->tx_queue_id[portid] = tx_queueid;
> +
> +		/* Pre-populate pkt offloads based on capabilities */
> +		qconf->outbound.ipv4_offloads = PKT_TX_IPV4;
> +		qconf->outbound.ipv6_offloads = PKT_TX_IPV6;
> +		if (req_tx_offloads & DEV_TX_OFFLOAD_IPV4_CKSUM)
> +			qconf->outbound.ipv4_offloads |= PKT_TX_IP_CKSUM;
I believe this is the issue, this should not be req_tx_offload. 
req_tx_offloads cannot have DEV_TX_OFFLOAD_IPV4_CKSUM set for lookaside 
mode.
> +
>   		tx_queueid++;
>   
>   		/* init RX queues */
> @@ -1749,6 +1843,7 @@ main(int32_t argc, char **argv)
>   	uint32_t lcore_id;
>   	uint8_t socket_id;
>   	uint16_t portid;
> +	uint64_t req_rx_offloads, req_tx_offloads;
>   
>   	/* init EAL */
>   	ret = rte_eal_init(argc, argv);
> @@ -1804,7 +1899,8 @@ main(int32_t argc, char **argv)
>   		if ((enabled_port_mask & (1 << portid)) == 0)
>   			continue;
>   
> -		port_init(portid);
> +		sa_check_offloads(portid, &req_rx_offloads, &req_tx_offloads);
> +		port_init(portid, req_rx_offloads, req_tx_offloads);
>   	}
>   
>   	cryptodevs_init();
> diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
> index c998c8076..9b1586f52 100644
> --- a/examples/ipsec-secgw/ipsec.h
> +++ b/examples/ipsec-secgw/ipsec.h
> @@ -146,6 +146,8 @@ struct ipsec_ctx {
>   	struct rte_mempool *session_pool;
>   	struct rte_mbuf *ol_pkts[MAX_PKT_BURST] __rte_aligned(sizeof(void *));
>   	uint16_t ol_pkts_cnt;
> +	uint64_t ipv4_offloads;
> +	uint64_t ipv6_offloads;
>   };
>   
>   struct cdev_key {
> @@ -239,4 +241,8 @@ sa_init(struct socket_ctx *ctx, int32_t socket_id);
>   void
>   rt_init(struct socket_ctx *ctx, int32_t socket_id);
>   
> +int
> +sa_check_offloads(uint16_t port_id, uint64_t *rx_offloads,
> +		uint64_t *tx_offloads);
> +
>   #endif /* __IPSEC_H__ */
> diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
> index d2d3550a4..f6271bc60 100644
> --- a/examples/ipsec-secgw/sa.c
> +++ b/examples/ipsec-secgw/sa.c
> @@ -1017,3 +1017,38 @@ outbound_sa_lookup(struct sa_ctx *sa_ctx, uint32_t sa_idx[],
>   	for (i = 0; i < nb_pkts; i++)
>   		sa[i] = &sa_ctx->sa[sa_idx[i]];
>   }
> +
> +/*
> + * Select HW offloads to be used.
> + */
> +int
> +sa_check_offloads(uint16_t port_id, uint64_t *rx_offloads,
> +		uint64_t *tx_offloads)
> +{
> +	struct ipsec_sa *rule;
> +	uint32_t idx_sa;
> +
> +	*rx_offloads = 0;
> +	*tx_offloads = 0;
> +
> +	/* Check for inbound rules that use offloads and use this port */
> +	for (idx_sa = 0; idx_sa < nb_sa_in; idx_sa++) {
> +		rule = &sa_in[idx_sa];
> +		if ((rule->type == RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO ||
> +				rule->type ==
> +				RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL)
> +				&& rule->portid == port_id)
> +			*rx_offloads |= DEV_RX_OFFLOAD_SECURITY;
> +	}
> +
> +	/* Check for outbound rules that use offloads and use this port */
> +	for (idx_sa = 0; idx_sa < nb_sa_out; idx_sa++) {
> +		rule = &sa_out[idx_sa];
> +		if ((rule->type == RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO ||
> +				rule->type ==
> +				RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL)
> +				&& rule->portid == port_id)
> +			*tx_offloads |= DEV_TX_OFFLOAD_SECURITY;
> +	}
> +	return 0;
> +}