Re: [dpdk-dev] [PATCH] security: remove experimental tag

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

> > >> -----Original Message-----
> > >> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> > >> Sent: Tuesday, November 13, 2018 11:28 AM
> > >> To: dev@dpdk.org
> > >> Cc: thomas@monjalon.net; Ananyev, Konstantin
> > >> <konstantin.ananyev@intel.com>; jerin.jacob@caviumnetworks.com;
> > >> anoob.joseph@caviumnetworks.com; Nicolau, Radu
> > >> <radu.nicolau@intel.com>; Doherty, Declan
> > >> <declan.doherty@intel.com>; Hemant Agrawal
> > >> <hemant.agrawal@nxp.com>; Akhil Goyal <akhil.goyal@nxp.com>
> > >> Subject: [PATCH] security: remove experimental tag
> > >>
> > >> rte_security has been experimental since DPDK 17.11 release.
> > >> Now the library has matured and expermental tag is removed in this
> > >> patch.
> > > I agree that it is present for a while in dpdk.org, but as I can see
> > > we still have unimplemented API here.
> > > Which makes me doubt that it is ok to remove experimental tag from it.
> > > Konstantin
> > 3 vendors(Intel/Cavium/NXP) have tested their PMDs on security and
> > made the changes that they need.
> > Which APIs are missing?
> 
> What I am aware about:
> a) rte_security_ops. get_userdata
> [Akhil] I believe Cavium added some patches in ipsec-secgw app for its usage and I believe they do have implementation for that.

ipsec-secgw has some code that refers it, but at present moment there is no PMD in dpdk.org that supports it
(at least I can't find any).

> Also I
> cannot see any changes in rte_security for its support in PMDs.

Might be, but wouldn't you expect function to be at least implemented
to call it 'mature'?

> 
> b) RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL
> 
> [Akhil] Cavium supports it.

Might be, but again it is not in dpdk.org right now
(AFAIK it is planned for 19.02).

> 
> c) rte_security_capability.ol_flags:
>     RTE_SECURITY_PDCP_ORDERING_CAP
>     RTE_SECURITY_PDCP_DUP_DETECT_CAP
> 
> [Akhil] PDCP is not currently supported by any of the vendors except NXP and NXP do not support these capabilities.
> For this also, I don’t see any change in the library. It would be only PMD which needs to support it.
> 
>     RTE_SECURITY_TX_HW_TRAILER_OFFLOAD
>     RTE_SECURITY_RX_HW_TRAILER_OFFLOAD
> 
> [Akhil] Same here, these are all PMD capabilities which do not require any change in rte_security.

Without real implementation, how can we be certain about it?
Might be while implementing feature X we would realize that something else is needed.
Another question - what users who build their products on top of rte_security
have to do?
Should they include support for all these unimplemented capabilities into their
code or not?
Considering the fact, that right now there is no way for them to test/try it.

> 
> >I believe addition of protocols is not an issue  even if we remove
> >experimental tag.
> 
> After another thought - it is probably unfair to keep whole lib as experimental because few things are missing.
> But I think things that are unimplemented (or related to them) need to stay in 'experimental' state.
> 
> [Akhil] I do not foresee any changes in library, so I believe experimental is not required. Please correct me if this is incorrect understanding.

The only change I am personally plan to do in 19.02 -
add opaque userdata field into rte_security_session:
struct rte_security_session {
 	void *sess_private_data;
 	/**< Private session material */
+	uint64_t userdata;
+	/**< Opaque user defined data */
 };

Might be in future extra changes would be needed to pass ipsec
sqn/replay_window data between HW/SW.
Not aware about any other changes.
Though these future changes is not my main concern.
After all we have a defined process for making changes into non-experimental API.
I just don't see how we can consider API that has un-implemented parts as a 'mature'.
Probably we have different views on what experimental/mature API means.
From my perspective to name it a 'mature' it needs to at least to be implemented and
tested (proved working),  plus stable enough(not major changes coming).
Konstantin