From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anoob Joseph Subject: Re: [PATCH v3] examples/ipsec-secgw: fix usage of incorrect port Date: Wed, 29 Nov 2017 09:51:36 +0530 Message-ID: References: <1510673823-24475-1-git-send-email-anoob.joseph@caviumnetworks.com> <1510738915-14712-1-git-send-email-anoob.joseph@caviumnetworks.com> <0349861e-de98-92b5-8b6f-7ab944dd45bf@nxp.com> <5d0bed52-ad0e-df65-158e-4e62b79fe754@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: Narayana Prasad , Jerin Jacob , dev@dpdk.org To: Akhil Goyal , Declan Doherty , Sergio Gonzalez Monroy , Radu Nicolau Return-path: Received: from NAM02-SN1-obe.outbound.protection.outlook.com (mail-sn1nam02on0056.outbound.protection.outlook.com [104.47.36.56]) by dpdk.org (Postfix) with ESMTP id 04FA8378B for ; Wed, 29 Nov 2017 05:21:57 +0100 (CET) In-Reply-To: <5d0bed52-ad0e-df65-158e-4e62b79fe754@nxp.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Akhil, On 24-11-2017 16:19, Akhil Goyal wrote: > Hi Anoob, > > On 11/24/2017 3:28 PM, Anoob wrote: >>>>   static inline void >>>>   route4_pkts(struct rt_ctx *rt_ctx, struct rte_mbuf *pkts[], >>>> uint8_t nb_pkts) >>>>   { >>>>       uint32_t hop[MAX_PKT_BURST * 2]; >>>>       uint32_t dst_ip[MAX_PKT_BURST * 2]; >>>> +    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 ip, ip_dst); >>>> -        dst_ip[i] = *rte_pktmbuf_mtod_offset(pkts[i], >>>> -                uint32_t *, offset); >>>> -        dst_ip[i] = rte_be_to_cpu_32(dst_ip[i]); >>>> +        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 ip, ip_dst); >>>> +            dst_ip[lpm_pkts] = *rte_pktmbuf_mtod_offset(pkts[i], >>>> +                    uint32_t *, offset); >>>> +            dst_ip[lpm_pkts] = rte_be_to_cpu_32(dst_ip[lpm_pkts]); >>>> +            lpm_pkts++; >>>> +        } >>>>       } >>>>   -    rte_lpm_lookup_bulk((struct rte_lpm *)rt_ctx, dst_ip, hop, >>>> nb_pkts); >>>> +    rte_lpm_lookup_bulk((struct rte_lpm *)rt_ctx, dst_ip, hop, >>>> lpm_pkts); >>>> + >>>> +    lpm_pkts = 0; >>>>         for (i = 0; i < nb_pkts; i++) { >>>> -        if ((hop[i] & RTE_LPM_LOOKUP_SUCCESS) == 0) { >>>> +        if (pkts[i]->ol_flags & PKT_TX_SEC_OFFLOAD) { >>>> +            /* 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 & RTE_LPM_LOOKUP_SUCCESS) == 0) >>>> +                pkt_hop = -1; >>>> +        } >>>> + >>> I believe the following check is redundant for non inline case. I >>> believe get_hop_for_offload_pkt can also set the >>> RTE_LPM_LOOKUP_SUCCESS if route is success and take the (pkt_hop & >>> RTE_LPM_LOOKUP_SUCCESS) == 0 check outside the if else block and >>> free the packet if it is unsuccessful. >>> >>> Same comment for route6_pkts. Checking with -1 may not be a good >>> idea if we have a flag available for the same. >>> Others can comment. >> The problem is ipv4 & ipv6 LPM lookups return different error values, >> but we are using a single routine to get the hop for offload packets. >> The flag(RTE_LPM_LOOKUP_SUCCESS) is only for ipv4 lookups. For ipv6, >> error is -1. If we need a cleaner solution, we can have ipv4 & ipv6 >> variants of "get_hop_for_offload_pkt". But that would be repetition >> of some code. > > my concern over this patch is that there is an addition of an extra > check in the non inline case and we can get rid of that with some > changes in the code(lib/app). Regarding route6_pkts, the code looks > cleaner than route4_pkts If we have ipv4 and ipv6 variants of the "get_hop_for_offload_packet" function, the code would look much cleaner. Shall I update the patch with such a change and send v4? > > > -Akhil Thanks, Anoob