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

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

Hi Anoob,
On 11/29/2017 9:51 AM, Anoob Joseph wrote:
> 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?

I believe we shall get rid of "RTE_LPM_LOOKUP_SUCCESS" from the 
rte_lpm_lookup_bulk(), we shall have similar error flags for v4 and v6 
APIs. Either we can have RTE_LPM_LOOKUP_SUCCESS or -1 as check for errors.
Sergio can comment on this.

Duplicating code for get_hop_for_offload_packet may not be a good idea.

-Akhil