On Mon, Apr 08, 2019 at 10:41:29AM +0000, Ananyev, Konstantin wrote: > > Hi Yongseok, > > > Outbound ESP sequence number should be incremented atomically and refeenced > > indirectly. Otherwise, concurrent access by multiple threads will break > > coherency. > > I think MT mode per SA is not supported right now by ipsec-secgw. > From > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdoc.dpdk.org%2Fguides%2Fsample_app_ug%2Fipsec_secgw.html&data=02%7C01%7Cyskoh%40mellanox.com%7C92fcdccccff446f9f93708d6bc0ec532%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636903168953772804&sdata=8l1b79cFf7Jodlk%2Bup3b8uRUUSU2IyglmFxhmXqsbCI%3D&reserved=0: > 49.2. Constraints > ... > Each SA must be handle by a unique lcore (1 RX queue per port).
I wasn't aware of this limitation. If it is already documented, I'll take back my patch. Actually, I have done some performance testing with this example and it worked fine with single SA in each direction. That's why I thought it was supported. > Also the changes you proposed wouldn't handle inbound case. No seq number check for inbound direction in this example code.. > Anyway, if you still like to have atomic sqn updates for outbound, > then probably better to make it configurable > (otherwise it would introduce unnecessary slowdown). > There is '-a' (atomic) command-line option. > Right now it works for librte_ipsec mode only, but I suppose it wouldn't > be big deal to make legacy mode to take it into account too. > [...] > > diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c > > index a7298a30c2..adf6ac3f9a 100644 > > --- a/examples/ipsec-secgw/sa.c > > +++ b/examples/ipsec-secgw/sa.c > > @@ -795,7 +795,7 @@ sa_add_rules(struct sa_ctx *sa_ctx, const struct > > ipsec_sa entries[], > > return -EINVAL; > > } > > *sa = entries[i]; > > - sa->seq = 0; > > + rte_atomic64_set(&sa->seq, -1); > > I think initial value should remain zero. The reason was because there's no rte_atomic64_return_add() but rte_atomic64_add_return(). Would be better to have it with __sync_fetch_and_add() later. Anyway, I'm taking back this patch like I mentioned. Thanks, Yongseok
