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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Wednesday, January 2, 2019 1:43 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: Horton, Remy <remy.horton@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v5 01/10] examples/ipsec-secgw: allow user to disable some RX/TX offloads
> 
> 
> 
> 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 remains as it was before the patchset.
By default we initialize dev_rx_offload/dev_tx_offloads to (uint64_t)-1;
Then: 
dev_info.rx_offload_capa &= dev_rx_offload;
to disable unwanted capabilities.
Then:
if ((local_port_conf.rxmode.offloads & dev_info.rx_offload_capa)
to check that all required capabilities are supported.

> 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.

My bad - screwed part of Remy's changes while introducing offloads options.
Will fix it in v6.
Konstantin