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

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

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.
> Sergio can comment on this.
>
> Duplicating code for get_hop_for_offload_packet may not be a good idea.
>
> -Akhil
>