Re: [dpdk-dev] [PATCH v2] examples/ipsec-secgw: fix usage of incorrect port

[Radu Nicolau <radu.nicolau@intel.com>]

On 11/14/2017 3:37 PM, Anoob Joseph wrote:
> When security offload is enabled, the packet should be forwarded on the
> port configured in the SA. Security session will be configured on that
> port only, and sending the packet on other ports could result in
> unencrypted packets being sent out.
>
> This would have performance improvements too, as the per packet LPM
> lookup would be avoided for IPsec packets, in inline mode.
>
> Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload")
>
> Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
> ---
> v2:
> * Updated documentation with the change in behavior for outbound inline
>    offloaded packets.
>
>   doc/guides/sample_app_ug/ipsec_secgw.rst | 10 +++-
>   examples/ipsec-secgw/ipsec-secgw.c       | 92 +++++++++++++++++++++++++++-----
>   2 files changed, 87 insertions(+), 15 deletions(-)
>
> diff --git a/doc/guides/sample_app_ug/ipsec_secgw.rst b/doc/guides/sample_app_ug/ipsec_secgw.rst
> index d6cfdbf..d04e153 100644
> --- a/doc/guides/sample_app_ug/ipsec_secgw.rst
> +++ b/doc/guides/sample_app_ug/ipsec_secgw.rst
> @@ -61,6 +61,12 @@ In case of complete protocol offload, the processing of headers(ESP and outer
>   IP header) is done by the hardware and the application does not need to
>   add/remove them during outbound/inbound processing.
>   
> +For inline offloaded outbound traffic, the application need not do the LPM
> +lookup for routing, as the port on which the packet has to be forwarded, will
extra 
comma......................................................................................................................^here
And maybe change need not to will not, to reflect the actual behavior.
> <snip>
>   
> @@ -619,26 +660,49 @@ route6_pkts(struct rt_ctx *rt_ctx, struct rte_mbuf *pkts[], uint8_t nb_pkts)
>   	int32_t hop[MAX_PKT_BURST * 2];
>   	uint8_t dst_ip[MAX_PKT_BURST * 2][16];
>   	uint8_t *ip6_dst;
> +	int32_t pkt_hop = 0;
>   	uint16_t i, offset;
> +	uint16_t lpm_pkts = 0;
>   
>   	if (nb_pkts == 0)
>   		return;
>   
> +	/* Need to do an LPM lookup for non-offload packets. Offload packets
> +	 * will have port ID in the SA
> +	 */
> +
>   	for (i = 0; i < nb_pkts; i++) {
> -		offset = offsetof(struct ip6_hdr, ip6_dst);
> -		ip6_dst = rte_pktmbuf_mtod_offset(pkts[i], uint8_t *, offset);
> -		memcpy(&dst_ip[i][0], ip6_dst, 16);
> +		if (!(pkts[i]->ol_flags & PKT_TX_SEC_OFFLOAD)) {
> +			/* Security offload not enabled. So an LPM lookup is
> +			 * required to get the hop
> +			 */
> +			offset = offsetof(struct ip6_hdr, ip6_dst);
> +			ip6_dst = rte_pktmbuf_mtod_offset(pkts[i], uint8_t *,
> +					offset);
> +			memcpy(&dst_ip[lpm_pkts][0], ip6_dst, 16);
> +			lpm_pkts++;
> +		}
>   	}
>   
> -	rte_lpm6_lookup_bulk_func((struct rte_lpm6 *)rt_ctx, dst_ip,
> -			hop, nb_pkts);
> +	rte_lpm6_lookup_bulk_func((struct rte_lpm6 *)rt_ctx, dst_ip, hop,
> +			lpm_pkts);
> +
> +	lpm_pkts = 0;
>   
>   	for (i = 0; i < nb_pkts; i++) {
> -		if (hop[i] == -1) {
> +		if ((pkts[i]->ol_flags & PKT_TX_SEC_OFFLOAD) == 0) {
The if condition is wrong here.
> +			/* Read hop from the SA */
> +			pkt_hop = get_hop_for_offload_pkt(pkts[i]);
> +		} else {
> +			/* Need to use hop returned by lookup */
> +			pkt_hop = hop[lpm_pkts++];
> +		}
> +
> +		if (pkt_hop == -1) {
>   			rte_pktmbuf_free(pkts[i]);
>   			continue;
>   		}
> -		send_single_packet(pkts[i], hop[i] & 0xff);
> +		send_single_packet(pkts[i], pkt_hop & 0xff);
>   	}
>   }
>