Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: fix coherency of ESP sequence number

[Yongseok Koh <yskoh@mellanox.com>]

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