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

[Anoob Joseph <anoob.joseph@caviumnetworks.com>]

Hi,


On 12/11/2017 03:56 PM, Radu Nicolau wrote:
> Hi,
>
>
> On 12/6/2017 11:08 AM, Anoob wrote:
>> Hi Akhil,
>>
>> On 12/04/2017 01:19 PM, Akhil Goyal wrote:
>>> 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.
>> This will call for an ABI change. And LPM library has multiple 
>> variants for v4 & v6 lookups. We will need to modify all such 
>> instances. I've CCed Bruce for his opinion on this matter. If 
>> maintainers can decide on how to address this properly, I can plan my 
>> next steps accordingly.
> Maybe this alternative approach will help: change the 
> get_hop_for_offload_packet to return -1 for v6 and clear 
> RTE_LPM_LOOKUP_SUCCESS flag for v4 errors. This will be on the error 
> path so the extra code to check the pkt type will have no performance 
> impact, and the route function can be cleaner and we can lose the 
> extra if in the v4 one.
That should be fine I guess. So the get_hop_for_offload_packet will have 
one more argument to specify whether it is ipv4 or ipv6, right?

I'll revise the patch with this suggestion.
>>> Sergio can comment on this.
>>>
>>> Duplicating code for get_hop_for_offload_packet may not be a good idea.
>>>
>>> -Akhil
>>>
>>
>