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