Re: [dpdk-dev] [EXT] RE: [PATCH v4 12/15] examples/ipsec-secgw: add app mode worker

[Lukas Bartosik <lbartosik@marvell.com>]

Hi Akhil,

Please see my answer below.

Thanks,
Lukasz

On 27.02.2020 13:07, Akhil Goyal wrote:
>>
>> Hi Lukasz,
>>
>>>>
>>>> Is it not possible to use the existing functions for finding routes, checking
>>> packet types and checking security policies.
>>>> It will be very difficult to manage two separate functions for same work. I
>> can
>>> see that the pkt->data_offs
>>>> Are not required to be updated in the inline case, but can we split the existing
>>> functions in two so that they can be
>>>> Called in the appropriate cases.
>>>>
>>>> As you have said in the cover note as well to add lookaside protocol support.
>> I
>>> also tried adding it, and it will get very
>>>> Difficult to manage separate functions for separate code paths.
>>>>
>>>
>>> [Lukasz] This was also Konstantin's comment during review of one of previous
>>> revisions.
>>> The prepare_one_packet() and prepare_tx_pkt() do much more than we need
>>> and for performance reasons
>>> we crafted new functions. For example, process_ipsec_get_pkt_type function
>>> returns nlp and whether
>>> packet type is plain or IPsec. That's all. Prepare_one_packet() process packets
>> in
>>> chunks and does much more -
>>> it adjusts mbuf and packet length then it demultiplex packets into plain and
>> IPsec
>>> flows and finally does
>>> inline checks. This is similar for update_mac_addrs() vs prepare_tx_pkt() and
>>> check_sp() vs inbound_sp_sa()
>>> that prepare_tx_pkt() and inbound_sp_sa() do more that we need in event
>> mode.
>>>
>>> I understand your concern from the perspective of code maintenance but on
>> the
>>> other hand we are concerned with performance.
>>> The current code is not optimized to support multiple mode processing
>>> introduced with rte_security. We can work on a common
>>> routines once we have other modes also added, so that we can come up with
>> a
>>> better solution than what we have today.
>>>
>>
>> Yes that is correct, but we should split the existing functions so that the part
>> which is common
>> In both mode should stay common and we do not have duplicate code in the app.
>>
>> I believe we should take care of this when we add lookaside cases. We shall
>> remove all duplicate
>> Code. Ideally it should be part of this patchset. But we can postpone it to the
>> lookaside case addition.
>>
>>
> 
> I believe the route(4/6)_pkts and route(4/6)_pkt can be made uniform quite easily.
> Now you can call either send_single_pkt() or rte_event_eth_tx_adapter_enqueue() from
> the caller of route4_pkts.
> I don’t think this will impact the performance at all.
> Instead of having 3 for loops, now there will be only 2 and nothing else is getting changed for
> anybody. In fact we can reduce 1 more, if we can call send pkts from inside the route4_pkts.
> I think that can also be done, but it may increase the lookup duration as there may be cache miss.
> But that need to be experimented. What say??
> 
> static inline void
> route4_pkts(struct rt_ctx *rt_ctx, struct rte_mbuf *pkts[], uint32_t *hop[],
>                 uint8_t nb_pkts)
> {
>         uint32_t dst_ip;
>         uint16_t i, offset;
> 
>         if (nb_pkts == 0)
>                 return;
> 
>         /* Need to do an LPM lookup for non-inline packets. Inline packets will
>          * have port ID in the SA
>          */
> 
>         for (i = 0; i < nb_pkts; 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 = *rte_pktmbuf_mtod_offset(pkts[i],
>                                         uint32_t *, offset);
>                         dst_ip = rte_be_to_cpu_32(dst_ip);
>                         if (rte_lpm_lookup((struct rte_lpm *)rt_ctx, dst_ip, hop[i]))
>                                 rte_pktmbuf_free(pkts[i]);
>                 } else {
>                         *hop[i] = get_hop_for_offload_pkt(pkts[i], 0);
>                 }
>         }
> }
> 

[Lukasz] Thank you for your suggestion. Looking at the proposed change I have major concern related
to performance. Current rout4_pkts uses rte_lpm_lookup_bulk() which can benefit from SIMD instructions.
Replacing it with rte_lpm_lookup might introduce substantial performance degradation. I will start 
experimenting with processing functions (routing packets, checking packet type, checking sp policies) to make
them as much common as possible between poll and event modes. As agreed the plan is to make processing functions common
with the addition of lookaside event mode. In the meantime I will send V5 event mode patches which address your other comments.