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 26.02.2020 07:04, 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.
> 
> 
>>
>>>> +	return 1;
>>>
>>> It will be better to use some MACROS while returning
>>> Like
>>> #define PKT_FORWARD   1
>>> #define PKT_DROPPED     0
>>> #define PKT_POSTED       2  /*may be for lookaside cases */
>>>
> 
> I think you missed this comment.
> 

[Lukasz] Thank you for pointing out that I missed the comment. I will use macros when returning instead of magic numbers.

>>>> +
>>>> +drop_pkt_and_exit:
>>>> +	RTE_LOG(ERR, IPSEC, "Outbound packet dropped\n");
>>>> +	rte_pktmbuf_free(pkt);
>>>> +	ev->mbuf = NULL;
>>>> +	return 0;
>>>> +}
>>>> +