From: Anoob Joseph <anoob.joseph@caviumnetworks.com>
To: Radu Nicolau <radu.nicolau@intel.com>,
Akhil Goyal <akhil.goyal@nxp.com>,
Declan Doherty <declan.doherty@intel.com>,
Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>,
Bruce Richardson <bruce.richardson@intel.com>
Cc: anoob.joseph@caviumnetworks.com,
Narayana Prasad <narayanaprasad.athreya@caviumnetworks.com>,
Jerin Jacob <jerin.jacob@caviumnetworks.com>,
dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v3] examples/ipsec-secgw: fix usage of incorrect port
Date: Mon, 11 Dec 2017 16:08:13 +0530 [thread overview]
Message-ID: <1293e784-b4e5-23a5-caca-63cdaac22e5a@caviumnetworks.com> (raw)
In-Reply-To: <5e4c92fd-1fa1-648c-0c9e-b6b282c232e7@intel.com>
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
>>>
>>
>
next prev parent reply other threads:[~2017-12-11 10:38 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-13 16:13 [dpdk-dev] [PATCH] " Anoob Joseph
2017-11-13 17:23 ` Radu Nicolau
2017-11-13 19:24 ` Anoob Joseph
2017-11-14 12:01 ` Nicolau, Radu
2017-11-14 15:37 ` [dpdk-dev] [PATCH v2] " Anoob Joseph
2017-11-14 16:16 ` Radu Nicolau
2017-11-15 9:41 ` [dpdk-dev] [PATCH v3] " Anoob Joseph
2017-11-24 9:28 ` Akhil Goyal
2017-11-24 9:58 ` Anoob
2017-11-24 10:49 ` Akhil Goyal
2017-11-29 4:21 ` Anoob Joseph
2017-12-04 7:49 ` Akhil Goyal
2017-12-06 11:08 ` Anoob
2017-12-11 10:26 ` Radu Nicolau
2017-12-11 10:38 ` Anoob Joseph [this message]
2017-12-11 15:35 ` [dpdk-dev] [PATCH v4] " Anoob Joseph
2017-12-12 6:54 ` Anoob Joseph
2017-12-12 7:34 ` Akhil Goyal
2017-12-12 8:32 ` [dpdk-dev] [PATCH v5] " Anoob Joseph
2017-12-12 11:27 ` Radu Nicolau
2017-12-14 9:01 ` De Lara Guarch, Pablo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1293e784-b4e5-23a5-caca-63cdaac22e5a@caviumnetworks.com \
--to=anoob.joseph@caviumnetworks.com \
--cc=akhil.goyal@nxp.com \
--cc=bruce.richardson@intel.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=jerin.jacob@caviumnetworks.com \
--cc=narayanaprasad.athreya@caviumnetworks.com \
--cc=radu.nicolau@intel.com \
--cc=sergio.gonzalez.monroy@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).