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

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

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