[dpdk-dev] [PATCH v3] examples/ipsec-secgw: fix usage of incorrect port
Anoob Joseph
anoob.joseph at caviumnetworks.com
Mon Dec 11 11:38:13 CET 2017
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
>>>
>>
>
More information about the dev
mailing list