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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Lukas,

Apologies for delay with response.
 
> 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.

No, right now I don't have an idea how both can be satisfied.
About 1) - why do you consider that as problem?
As I understand when inline device is started by not properly programmed yet,
these packets will be treated by device as non-ipsec ones (just plain-text packets).
Then later in ipsec-secgw RX path they will probably be just dropped.
So no harm here, no?

About 2) - looking at ixgbe code, I think it can be changed to make ipsec flows
to survive device start/stop (though most likely not in 20.05 timeframe).
So I am not sure should we do it at all.
Obviously for ixgbe (with 1K max SA) it is not a problem to save extra info
about HW flows and restore them after stop/start.
Though for modern devices that might have millions of flows maintaining
such info in PMD might become a significant overhead.  
As generic question - how PMD should behave in such situation?
Should it support flows survival through dev_start/dev_stop,
or should it be app responsibility to reprogram flows after dev-start/dev_stop.
I looked through PG for rte_flow, and it seems a bit conversional here (at least for me): 
https://doc.dpdk.org/guides/prog_guide/rte_flow.html (11.7):
Form one side:
- DPDK does not keep track of flow rules definitions or flow rule objects automatically.
  Applications may keep track of the former and must keep track of the latter.
  PMDs may also do it for internal needs, however this must not be relied on by applications.
But a bit below:
- PMDs, not applications, are responsible for maintaining flow rules configuration
   when stopping and restarting a port or performing other actions which may affect them.
  They can only be destroyed explicitly by applications.

Would like to know what rte_flow maintainers and other interested parties think
about it?  

Konstantin

> 
> 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@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 */
> >>> >