Re: [PATCH v2 1/2] examples/ipsec-secgw: fix 1st pkt dropped for inline crypto

[Akhil Goyal <akhil.goyal@nxp.com>]

Hi Konstantin,

On 3/26/2019 5:59 PM, Ananyev, Konstantin wrote:
> Hi Akhil,
>
>>>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] examples/ipsec-secgw: fix 1st
>>>>>> pkt dropped for inline crypto
>>>>>>
>>>>>> Hi Bernard,
>>>>>>
>>>>>> On 3/7/2019 8:27 PM, Bernard Iremonger wrote:
>>>>>>> Inline crypto installs a flow rule in the NIC. This flow rule must
>>>>>>> be installed before the first inbound packet is received.
>>>>>>>
>>>>>>> The create_session() function installs the flow rule,
>>>>>>> create_session() has been refactored into create_inline_session()
>>>>>>> and create_lookaside_session(). The create_inline_session() function
>>>>>>> uses the socket_ctx data and is now called at initialisation in
>>>>>>> sa_add_rules().
>>>>>> why do we need a separate function for session creation for inline
>>>>>> and lookaside cases?
>>>>>> Why can't we initialize the sessions on sa_init in both the cases?
>>>>> For the create_inline_session(struct socket_ctx *skt_ctx, struct
>>>>> ipsec_sa *sa) function, all of the required data is available in the in the
>>>> skt_ctx variable.
>>>>> The skt_ctx variable is already setup when sa_init() is called.
>>>>>
>>>>> For the create_lookaside_session(struct ipsec_ctx *ipsec_ctx, struct
>>>>> ipsec_sa *sa) function, the required data is available in the ipsec_ctx
>>>> variable.
>>>>> The ipsec_ctx variable is not setup when sa_init() is called.
>>>>> It is setup in the main_loop() function  when the variable qconf is setup.
>>>>> The main_loop() function is called after the sa_init() function is called.
>>>>>
>>>>> I hope this  answers your question
>>>> Whatever information that is required for session creation is available before
>>>> we call the main loop() in both the cases.
>>>> My point is both the sessions(inline/lookaside) can be init at the same
>>>> position, we do not need to have a separate path for them.
>>>> If it is not possible in sa_init(), it may be somewhere else before the actual
>>>> data path is started.
>>>>
>>>> The problem with inline processing is that, h/w need to know the SA before
>>>> the first packet is received. So we cannot init the session on receive of first
>>>> packet. However there is no such limitation in case of lookaside, it can be
>>>> initialized anywhere.
>>>>
>>>> -Akhil
>>> This patch is intended to fix the bug in the inline processing, which is that  the flow rule must be installed, before the first packet is
>> received while leaving the lookaside processing as it was originally.
>>> It is not intended to refactor the lookaside processing.
>> The fix provided should not make the code complex and difficult to
>> understand when we have the option available to fix that in a simpler
>> way. Application is already complex enough(4 action types) and by adding
>> separate code for session init at two different places will be difficult
>> to maintain in future and will reduce the code readability.
> It can be changed to do initialization for both security and crypto sessions at sa_init()
> time, but it would cause much more changes in the code:
> the idea is for each SA go through all available crypto-devices, check their capabilities,
> and if they match, we'll invoke init_session() for that SA and device.
> That way at the end of sa_init() we'll have crypto-sessions that can be used on any attached
> crypto-dev.
Agreed, my point is we should have same code path for all action types 
as much as possible to make the user comfortable to port their 
applications over DPDK.
> As a drawback -  each session might occupy more memory.
That is still there for the inline cases.
> Though as I said, it would require more changes (and testing) in init code.
> So my thought to have this patch as it is and then add these changes as a separate patch series (phase 2).
> What do you think?
The code changes are still there in this patch as well. And the testing 
cycle has not started yet for this release.

5 files changed, 246 insertions(+), 180 deletions(-)

If we intend to defer it, we will have similar code movement again. So IMO, it is good that we do the changes in one go.
This is an application code and we can have it merged in RC2 as well.

> Konstantin
>