> > > > 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? > > [Anoob] Yes, when SAs are not yet created and device is started then HW > treats IPsec packets as clear packets and does not apply inline to > them. The IPsec packets arrive at application level. This points at a serious > hardware issue as a packet intended for IPsec has reached > application without being IPsec processed. Application would check if session > is created and since the session is available at the time of > processing the packet (in the application), it would look like a hardware > issue when it is just a false positive.
I still not sure why is that big deal, but ok, can we then just do it in a different way for poll vs event mode for now: for event mode do dev_start() after sa_init(), for poll mode leave things as it (dev_start(), then sa_init()). Would that address the issue? > > > > 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. > > [Anoob] I believe the spec talks about the behavior during start/stop and > initialization. > > "Flow rules are not maintained between successive port initializations. An > application exiting without releasing them and restarting must re- > create them from scratch." > > "PMDs, not applications, are responsible for maintaining flow rules > configuration when stopping and restarting a port or performing other > actions which may affect them." But it also says: - 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. > > From the above two, following is what I could conclude, > 1. Across initializations rte_flow rules are not expected to be retained. > 2. Across restarts (dev start/stop), PMDs have to ensure rte_flow rules > survive. Ok, I get your opinion, but I would like more input from the rest of the community. In particular from PMD/flow/security designers/maintainers/users. Should PMD be responsible for keeping/restoring all flows rules config accross dev_start/dev_stop? If yes, does anyone see any implications with that in terms of: - increased memory footprint - time dev_start would take - time flow_rule_add/del will take - possible storing of auth/crypto keys inside the PMD BTW, I assume that octeontx2 does keep ipsec (all?) flow rules across dev_start/dev_stop? If so, does HW can keep it, or PMD restores it at start? Or you just rely on table shared between HW and SW? > I haven't looked at the ixgbe PMD yet, but technically start/stop should only > be about whether the device is running. Not only. It does HW reset/init, starts RX/TX queues, selects RX/TX functions, applies/restores settings to HW, etc. > I believe there are > other settings also which need to be retained between start/stop. > > From https://doc.dpdk.org/api/rte__ethdev_8h.html > > Please note that some configuration is not stored between calls to > rte_eth_dev_stop()/rte_eth_dev_start(). The following configuration will > be retained: > > - MTU > - flow control settings > - receive mode configuration (promiscuous mode, all-multicast mode, > hardware checksum mode, RSS/VMDQ settings etc.) > - VLAN filtering configuration > - default MAC address > - MAC addresses supplied to MAC address array > - flow director filtering mode (but not filtering rules) > - NIC queue statistics mappings > > Flow director refers to rte_flow, right? Yes, and it says mode, not rules. > > I looked through PG for rte_flow, and it seems a bit conversional here (at > > least > > for me): > > https://urldefense.proofpoint.com/v2/url?u=https- > > 3A__doc.dpdk.org_guides_prog-5Fguide_rte- > > 5Fflow.html&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs > > 2n6B- > > WYLn1v9SyTMrT5EQqh2TU&m=pUa6XHzwVA2IP7UEe5b98hosHmHjxlLOmn3IxH > > hZQAA&s=2BM4iySNyS0svzG26B0PiG8hQoAT2clAtkxiJrytdaI&e= (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? > > [Anoob] I do agree that the above two points can be misleading. But it > actually describes the usage of rte_flow API. > > 1. "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." > DPDK rte_flow spec doesn't provide an API to check if a rule is already > created. Application is configuring hardware resources using rte_flow > and it has to keep track of what is configured. > > 2. "PMDs, not applications, are responsible for maintaining flow rules > configuration when stopping and restarting a port or performing > other actions which may affect them." > In this case, we are talking about the state of the hardware. Once the rules > are configured, it should survive across stop-start. Application is > not required to destroy and create rte_flows in such case. > > In first case, we are talking about rte_flow objects & rules (like session in > case of rte_security) and in the second we are talking about > hardware state. > > I think this patch can be delayed till ixgbe can be fixed. Having it in 20.08 > timeframe should be fine. > > > > > 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=SchR > > HhE7GLC > > > jEY4i2a1byjC_FpWgRLtq4- > > > kLvKp3_84&m=w3xh94Ox4xIhabfE- > > nD2VbEWbh2JTmiscVMb6pJZcYo&s=9rDtRPGK2QBD > > > AcY8VQf0HQzXINtQzucwIxU7DB2ND5s&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 <lbarto...@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 */ > > > >>> >
