[dpdk-dev] [PATCH v3] examples/ipsec-secgw: fix usage of incorrect port
Anoob Joseph
anoob.joseph at caviumnetworks.com
Wed Nov 29 05:21:36 CET 2017
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
More information about the dev
mailing list