[dpdk-dev] [EXT] RE: [PATCH] examples/ipsec-secgw: fix dropping of initial IPsec pkts

[Lukas Bartosik [C]]

Hi Konstantin,

Do you have any thoughts how both cases could be covered:
1. Inline not applied to inbound IPsec pkts for short duration 
of time after rte_eth_dev_start() but before sa_init() is executed (which creates SAs).
2. SAs not surviving rte_eth_dev_start() on ixgbe driver.

Thanks,
Lukasz

On 16.04.2020 14:28, Lukas Bartosik [C] wrote:
> Hi Konstantin,
> 
> Please see my answer below.
> 
> Thanks,
> Lukasz
> 
> On 16.04.2020 01:47, Ananyev, Konstantin wrote:
>> External Email
>>
>> ----------------------------------------------------------------------
>>
>>
>> Hi Lukasz,
>>  
>>> Hi Konstantin,
>>>
>>> In this patch I moved the sa_init() before rte_eth_dev_start() in order to avoid dropping
>>> of IPsec pkts when a traffic flows and the ipsec-secgw application is started.
>>>
>>> However I remember that during review of event mode patches you mentioned that
>>> moving sa_init() before rte_eth_dev_start() is an issue for one of
>>> the Intel drivers.
>>
>> Yes, I think so.
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__mails.dpdk.org_archives_dev_2019-2DDecember_153908.html&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=SchRHhE7GLCjEY4i2a1byjC_FpWgRLtq4-kLvKp3_84&m=w3xh94Ox4xIhabfE-nD2VbEWbh2JTmiscVMb6pJZcYo&s=9rDtRPGK2QBDAcY8VQf0HQzXINtQzucwIxU7DB2ND5s&e= 
>> Moving that piece of code (dev_start) after sa_init() breaks ixgbe inline-crypto support.
>> As I understand, because configured ipsec flows don't persist dev_start().
>> At least for ixgbe PMD.
>> Any reason why to move that code at all?
>>
> 
> [Lukasz] We're observing issue in inline mode. When traffic flows and ipsec-secgw application is started then
> for short period of time inline is not applied by HW and IPsec packets reach the application. This is because
> sa_init() (which creates security associations SAs for HW) is executed after rte_eth_dev_start().
> That's the reason I moved the code. And that movement fixes the issue because now SAs are already
> created when eth ports are started.
> 
> Would it be possible to fix the ixgbe so that SAs would survive rte_eth_dev_start() ?
> Do you have any other idea how we could cover both cases ?
> 
>>  > Is this still the case ?
>>
>> AFAIK, yes. 
>> Thanks for bringing it to attention.
>> Konstantin
>>
>>
>>>
>>> Thanks,
>>> Lukasz
>>>
>>> On 08.04.2020 13:32, Lukasz Bartosik wrote:
>>>> In inline event mode when traffic flows and the ipsec-secgw
>>>> app is started then for short period of time IPsec packets
>>>> arrive at application without being decrypted and are dropped
>>>> by the application. This happens because eth ports are started
>>>> before creation of inline sessions and IPsec flows. This fix
>>>> rearranges the code in such a way that eth ports are always
>>>> started after creation of inline sessions and IPsec flows.
>>>>
>>>> Change-Id: Ifddc446082fb2897f81559517f90e1ee603e13f3
>>>> Signed-off-by: Lukasz Bartosik <lbartosik at marvell.com>
>>>> ---
>>>>  examples/ipsec-secgw/event_helper.c | 26 --------------------------
>>>>  examples/ipsec-secgw/ipsec-secgw.c  | 26 +++++++++++++-------------
>>>>  2 files changed, 13 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/examples/ipsec-secgw/event_helper.c b/examples/ipsec-secgw/event_helper.c
>>>> index 076f1f2..da861e4 100644
>>>> --- a/examples/ipsec-secgw/event_helper.c
>>>> +++ b/examples/ipsec-secgw/event_helper.c
>>>> @@ -1526,7 +1526,6 @@ int32_t
>>>>  eh_devs_init(struct eh_conf *conf)
>>>>  {
>>>>  	struct eventmode_conf *em_conf;
>>>> -	uint16_t port_id;
>>>>  	int ret;
>>>>
>>>>  	if (conf == NULL) {
>>>> @@ -1558,16 +1557,6 @@ eh_devs_init(struct eh_conf *conf)
>>>>  	/* Display the current configuration */
>>>>  	eh_display_conf(conf);
>>>>
>>>> -	/* Stop eth devices before setting up adapter */
>>>> -	RTE_ETH_FOREACH_DEV(port_id) {
>>>> -
>>>> -		/* Use only the ports enabled */
>>>> -		if ((conf->eth_portmask & (1 << port_id)) == 0)
>>>> -			continue;
>>>> -
>>>> -		rte_eth_dev_stop(port_id);
>>>> -	}
>>>> -
>>>>  	/* Setup eventdev */
>>>>  	ret = eh_initialize_eventdev(em_conf);
>>>>  	if (ret < 0) {
>>>> @@ -1589,21 +1578,6 @@ eh_devs_init(struct eh_conf *conf)
>>>>  		return ret;
>>>>  	}
>>>>
>>>> -	/* Start eth devices after setting up adapter */
>>>> -	RTE_ETH_FOREACH_DEV(port_id) {
>>>> -
>>>> -		/* Use only the ports enabled */
>>>> -		if ((conf->eth_portmask & (1 << port_id)) == 0)
>>>> -			continue;
>>>> -
>>>> -		ret = rte_eth_dev_start(port_id);
>>>> -		if (ret < 0) {
>>>> -			EH_LOG_ERR("Failed to start eth dev %d, %d",
>>>> -				   port_id, ret);
>>>> -			return ret;
>>>> -		}
>>>> -	}
>>>> -
>>>>  	return 0;
>>>>  }
>>>>
>>>> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
>>>> index 5fde4f7..e03bd89 100644
>>>> --- a/examples/ipsec-secgw/ipsec-secgw.c
>>>> +++ b/examples/ipsec-secgw/ipsec-secgw.c
>>>> @@ -2829,6 +2829,19 @@ main(int32_t argc, char **argv)
>>>>  	if (ret < 0)
>>>>  		rte_exit(EXIT_FAILURE, "eh_devs_init failed, err=%d\n", ret);
>>>>
>>>> +	/* Replicate each context per socket */
>>>> +	for (i = 0; i < NB_SOCKETS && i < rte_socket_count(); i++) {
>>>> +		socket_id = rte_socket_id_by_idx(i);
>>>> +		if ((socket_ctx[socket_id].mbuf_pool != NULL) &&
>>>> +			(socket_ctx[socket_id].sa_in == NULL) &&
>>>> +			(socket_ctx[socket_id].sa_out == NULL)) {
>>>> +			sa_init(&socket_ctx[socket_id], socket_id);
>>>> +			sp4_init(&socket_ctx[socket_id], socket_id);
>>>> +			sp6_init(&socket_ctx[socket_id], socket_id);
>>>> +			rt_init(&socket_ctx[socket_id], socket_id);
>>>> +		}
>>>> +	}
>>>> +
>>>>  	/* start ports */
>>>>  	RTE_ETH_FOREACH_DEV(portid) {
>>>>  		if ((enabled_port_mask & (1 << portid)) == 0)
>>>> @@ -2866,19 +2879,6 @@ main(int32_t argc, char **argv)
>>>>  			rte_exit(EXIT_FAILURE, "failed at reassemble init");
>>>>  	}
>>>>
>>>> -	/* Replicate each context per socket */
>>>> -	for (i = 0; i < NB_SOCKETS && i < rte_socket_count(); i++) {
>>>> -		socket_id = rte_socket_id_by_idx(i);
>>>> -		if ((socket_ctx[socket_id].mbuf_pool != NULL) &&
>>>> -			(socket_ctx[socket_id].sa_in == NULL) &&
>>>> -			(socket_ctx[socket_id].sa_out == NULL)) {
>>>> -			sa_init(&socket_ctx[socket_id], socket_id);
>>>> -			sp4_init(&socket_ctx[socket_id], socket_id);
>>>> -			sp6_init(&socket_ctx[socket_id], socket_id);
>>>> -			rt_init(&socket_ctx[socket_id], socket_id);
>>>> -		}
>>>> -	}
>>>> -
>>>>  	check_all_ports_link_status(enabled_port_mask);
>>>>
>>>>  	/* launch per-lcore init on every lcore */
>>> >