[dpdk-dev] cryptodev: mark experimental state

Message ID 1448473135-19604-1-git-send-email-thomas.monjalon@6wind.com (mailing list archive)
State Accepted, archived
Headers

Commit Message

Thomas Monjalon Nov. 25, 2015, 5:38 p.m. UTC
  The crypto API is in an early state.
It requires more discussions and experiments to declare it stable,
as discussed in http://dpdk.org/ml/archives/dev/2015-November/028634.html

A documentation section will be required in the guides.

Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
 MAINTAINERS                            |  2 +-
 config/common_bsdapp                   |  1 +
 config/common_linuxapp                 |  1 +
 doc/guides/contributing/versioning.rst |  1 +
 doc/guides/rel_notes/release_2_2.rst   | 10 ++++++++++
 lib/librte_cryptodev/rte_cryptodev.h   |  3 +++
 6 files changed, 17 insertions(+), 1 deletion(-)
  

Comments

Thomas Monjalon Nov. 25, 2015, 8:59 p.m. UTC | #1
2015-11-25 18:38, Thomas Monjalon:
> The crypto API is in an early state.
> It requires more discussions and experiments to declare it stable,
> as discussed in http://dpdk.org/ml/archives/dev/2015-November/028634.html
> 
> A documentation section will be required in the guides.
> 
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Applied
  
Panu Matilainen Nov. 26, 2015, 7:39 a.m. UTC | #2
On 11/25/2015 07:38 PM, Thomas Monjalon wrote:
> The crypto API is in an early state.
> It requires more discussions and experiments to declare it stable,
> as discussed in http://dpdk.org/ml/archives/dev/2015-November/028634.html
>
> A documentation section will be required in the guides.
>
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> ---
[...]
> --- a/config/common_bsdapp
> +++ b/config/common_bsdapp
> @@ -310,6 +310,7 @@ CONFIG_RTE_PMD_PACKET_PREFETCH=y
>
>   #
>   # Compile generic crypto device library
> +# EXPERIMENTAL: API may change without prior notice
>   #
>   CONFIG_RTE_LIBRTE_CRYPTODEV=y
>   CONFIG_RTE_LIBRTE_CRYPTODEV_DEBUG=n
> diff --git a/config/common_linuxapp b/config/common_linuxapp
> index eaad8d6..2866986 100644
> --- a/config/common_linuxapp
> +++ b/config/common_linuxapp
> @@ -319,6 +319,7 @@ CONFIG_RTE_PMD_PACKET_PREFETCH=y
>
>   #
>   # Compile generic crypto device library
> +# EXPERIMENTAL: API may change without prior notice
>   #
>   CONFIG_RTE_LIBRTE_CRYPTODEV=y
>   CONFIG_RTE_LIBRTE_CRYPTODEV_DEBUG=n
[...]

I think an experimental library which declares itself exempt from the 
ABI policy should not be compiled by default. That way anybody wanting 
to try it out will be forced to notice the experimental status.

More generally / longer term, perhaps there should be a 
CONFIG_RTE_EXPERIMENTAL which wraps all experimental features and 
defaults to off.

	- Panu -
  
Panu Matilainen Nov. 26, 2015, 8 a.m. UTC | #3
On 11/26/2015 09:39 AM, Panu Matilainen wrote:
> On 11/25/2015 07:38 PM, Thomas Monjalon wrote:
>> The crypto API is in an early state.
>> It requires more discussions and experiments to declare it stable,
>> as discussed in http://dpdk.org/ml/archives/dev/2015-November/028634.html
>>
>> A documentation section will be required in the guides.
>>
>> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
>> ---
> [...]
>> --- a/config/common_bsdapp
>> +++ b/config/common_bsdapp
>> @@ -310,6 +310,7 @@ CONFIG_RTE_PMD_PACKET_PREFETCH=y
>>
>>   #
>>   # Compile generic crypto device library
>> +# EXPERIMENTAL: API may change without prior notice
>>   #
>>   CONFIG_RTE_LIBRTE_CRYPTODEV=y
>>   CONFIG_RTE_LIBRTE_CRYPTODEV_DEBUG=n
>> diff --git a/config/common_linuxapp b/config/common_linuxapp
>> index eaad8d6..2866986 100644
>> --- a/config/common_linuxapp
>> +++ b/config/common_linuxapp
>> @@ -319,6 +319,7 @@ CONFIG_RTE_PMD_PACKET_PREFETCH=y
>>
>>   #
>>   # Compile generic crypto device library
>> +# EXPERIMENTAL: API may change without prior notice
>>   #
>>   CONFIG_RTE_LIBRTE_CRYPTODEV=y
>>   CONFIG_RTE_LIBRTE_CRYPTODEV_DEBUG=n
> [...]
>
> I think an experimental library which declares itself exempt from the
> ABI policy should not be compiled by default. That way anybody wanting
> to try it out will be forced to notice the experimental status.
>
> More generally / longer term, perhaps there should be a
> CONFIG_RTE_EXPERIMENTAL which wraps all experimental features and
> defaults to off.

On a related note, librte_mbuf_offload cannot be built if 
CONFIG_RTE_LIBRTE_CRYPTODEV is disabled. Which seems to suggest its (at 
least currently) so tightly couple to cryptodev that perhaps it too 
should be marked experimental and default to off.

	- Panu -
  
Thomas Monjalon Nov. 26, 2015, 10:08 a.m. UTC | #4
2015-11-26 10:00, Panu Matilainen:
> On 11/26/2015 09:39 AM, Panu Matilainen wrote:
> > On 11/25/2015 07:38 PM, Thomas Monjalon wrote:
> >> --- a/config/common_linuxapp
> >> +++ b/config/common_linuxapp
> >> @@ -319,6 +319,7 @@ CONFIG_RTE_PMD_PACKET_PREFETCH=y
> >>
> >>   #
> >>   # Compile generic crypto device library
> >> +# EXPERIMENTAL: API may change without prior notice
> >>   #
> >>   CONFIG_RTE_LIBRTE_CRYPTODEV=y
> >>   CONFIG_RTE_LIBRTE_CRYPTODEV_DEBUG=n
> > [...]
> >
> > I think an experimental library which declares itself exempt from the
> > ABI policy should not be compiled by default. That way anybody wanting
> > to try it out will be forced to notice the experimental status.
> >
> > More generally / longer term, perhaps there should be a
> > CONFIG_RTE_EXPERIMENTAL which wraps all experimental features and
> > defaults to off.
> 
> On a related note, librte_mbuf_offload cannot be built if 
> CONFIG_RTE_LIBRTE_CRYPTODEV is disabled. Which seems to suggest its (at 
> least currently) so tightly couple to cryptodev that perhaps it too 
> should be marked experimental and default to off.

I think you are right.
Declan, what is your opinion?
  
Doherty, Declan Nov. 26, 2015, 1:51 p.m. UTC | #5
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, November 26, 2015 10:09 AM
> To: Panu Matilainen; Doherty, Declan
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] cryptodev: mark experimental state
> 
> 2015-11-26 10:00, Panu Matilainen:
> > On 11/26/2015 09:39 AM, Panu Matilainen wrote:
> > > On 11/25/2015 07:38 PM, Thomas Monjalon wrote:
> > >> --- a/config/common_linuxapp
> > >> +++ b/config/common_linuxapp
> > >> @@ -319,6 +319,7 @@ CONFIG_RTE_PMD_PACKET_PREFETCH=y
> > >>
> > >>   #
> > >>   # Compile generic crypto device library
> > >> +# EXPERIMENTAL: API may change without prior notice
> > >>   #
> > >>   CONFIG_RTE_LIBRTE_CRYPTODEV=y
> > >>   CONFIG_RTE_LIBRTE_CRYPTODEV_DEBUG=n
> > > [...]
> > >
> > > I think an experimental library which declares itself exempt from the
> > > ABI policy should not be compiled by default. That way anybody wanting
> > > to try it out will be forced to notice the experimental status.
> > >
> > > More generally / longer term, perhaps there should be a
> > > CONFIG_RTE_EXPERIMENTAL which wraps all experimental features and
> > > defaults to off.
> >
> > On a related note, librte_mbuf_offload cannot be built if
> > CONFIG_RTE_LIBRTE_CRYPTODEV is disabled. Which seems to suggest its (at
> > least currently) so tightly couple to cryptodev that perhaps it too
> > should be marked experimental and default to off.
> 
> I think you are right.
> Declan, what is your opinion?


Hey Thomas, yes librte_mbuf_offload should also be set as experimental, it's 
probably one of the areas which will most likely change in the future.
 
On the issue of turning off experimental libraries in the build by default, my
preference would be not to turn them off unless the library has external
dependencies, otherwise the possibility of patches being submitted which 
could break an experimental library will be much higher. In my opinion the
fewer build configurations developers have to test against the better.
  
Panu Matilainen Nov. 27, 2015, 1:09 p.m. UTC | #6
On 11/26/2015 03:51 PM, Doherty, Declan wrote:
>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>> Sent: Thursday, November 26, 2015 10:09 AM
>> To: Panu Matilainen; Doherty, Declan
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] cryptodev: mark experimental state
>>
>> 2015-11-26 10:00, Panu Matilainen:
>>> On 11/26/2015 09:39 AM, Panu Matilainen wrote:
>>>> On 11/25/2015 07:38 PM, Thomas Monjalon wrote:
>>>>> --- a/config/common_linuxapp
>>>>> +++ b/config/common_linuxapp
>>>>> @@ -319,6 +319,7 @@ CONFIG_RTE_PMD_PACKET_PREFETCH=y
>>>>>
>>>>>    #
>>>>>    # Compile generic crypto device library
>>>>> +# EXPERIMENTAL: API may change without prior notice
>>>>>    #
>>>>>    CONFIG_RTE_LIBRTE_CRYPTODEV=y
>>>>>    CONFIG_RTE_LIBRTE_CRYPTODEV_DEBUG=n
>>>> [...]
>>>>
>>>> I think an experimental library which declares itself exempt from the
>>>> ABI policy should not be compiled by default. That way anybody wanting
>>>> to try it out will be forced to notice the experimental status.
>>>>
>>>> More generally / longer term, perhaps there should be a
>>>> CONFIG_RTE_EXPERIMENTAL which wraps all experimental features and
>>>> defaults to off.
>>>
>>> On a related note, librte_mbuf_offload cannot be built if
>>> CONFIG_RTE_LIBRTE_CRYPTODEV is disabled. Which seems to suggest its (at
>>> least currently) so tightly couple to cryptodev that perhaps it too
>>> should be marked experimental and default to off.
>>
>> I think you are right.
>> Declan, what is your opinion?
>
>
> Hey Thomas, yes librte_mbuf_offload should also be set as experimental, it's
> probably one of the areas which will most likely change in the future.
>
> On the issue of turning off experimental libraries in the build by default, my
> preference would be not to turn them off unless the library has external
> dependencies, otherwise the possibility of patches being submitted which
> could break an experimental library will be much higher. In my opinion the
> fewer build configurations developers have to test against the better.

What I'm more worried about is users and developers starting to rely on 
it while still in experimental state, a single comment in the header is 
really easy to miss.

So I'd like to see *some* mechanism which forces users and developers to 
acknowledge the fact that they're dealing with experimental work. 
Defaulting to off is one possibility, another one would be wrapping 
experimental APIs behind a define which you have to set to be able to 
use the API, eg:

#if defined(I_KNOW_THIS_IS_EXPERIMENTAL_AND_MAY_EAT_BABIES)
[...]
#endif

	- Panu -
  
Thomas Monjalon Nov. 27, 2015, 1:30 p.m. UTC | #7
2015-11-27 15:09, Panu Matilainen:
> On 11/26/2015 03:51 PM, Doherty, Declan wrote:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> >> 2015-11-26 10:00, Panu Matilainen:
> >>> On 11/26/2015 09:39 AM, Panu Matilainen wrote:
> >>>> I think an experimental library which declares itself exempt from the
> >>>> ABI policy should not be compiled by default. That way anybody wanting
> >>>> to try it out will be forced to notice the experimental status.
> >>>>
> >>>> More generally / longer term, perhaps there should be a
> >>>> CONFIG_RTE_EXPERIMENTAL which wraps all experimental features and
> >>>> defaults to off.
> >>>
> >>> On a related note, librte_mbuf_offload cannot be built if
> >>> CONFIG_RTE_LIBRTE_CRYPTODEV is disabled. Which seems to suggest its (at
> >>> least currently) so tightly couple to cryptodev that perhaps it too
> >>> should be marked experimental and default to off.
> >>
> >> I think you are right.
> >> Declan, what is your opinion?
> >
> >
> > Hey Thomas, yes librte_mbuf_offload should also be set as experimental, it's
> > probably one of the areas which will most likely change in the future.
> >
> > On the issue of turning off experimental libraries in the build by default, my
> > preference would be not to turn them off unless the library has external
> > dependencies, otherwise the possibility of patches being submitted which
> > could break an experimental library will be much higher. In my opinion the
> > fewer build configurations developers have to test against the better.
> 
> What I'm more worried about is users and developers starting to rely on 
> it while still in experimental state, a single comment in the header is 
> really easy to miss.

There are some comments in the config, the header file, doxygen and the release notes.
When using a feature, you have to read the header or the doc.
So would it be better advertised by adding a comment in the doxygen section of
some of the mandatory functions or structures?

> So I'd like to see *some* mechanism which forces users and developers to 
> acknowledge the fact that they're dealing with experimental work. 
> Defaulting to off is one possibility, another one would be wrapping 
> experimental APIs behind a define which you have to set to be able to 
> use the API, eg:
> 
> #if defined(I_KNOW_THIS_IS_EXPERIMENTAL_AND_MAY_EAT_BABIES)
> [...]
> #endif

Are you sure about the babies? ;)
  

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 7f3d967..460245b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -219,7 +219,7 @@  M: Thomas Monjalon <thomas.monjalon@6wind.com>
 F: lib/librte_ether/
 F: scripts/test-null.sh
 
-Crypto API
+Crypto API - EXPERIMENTAL
 M: Declan Doherty <declan.doherty@intel.com>
 F: lib/librte_cryptodev/
 F: app/test/test_cryptodev*
diff --git a/config/common_bsdapp b/config/common_bsdapp
index 56020b6..3286481 100644
--- a/config/common_bsdapp
+++ b/config/common_bsdapp
@@ -310,6 +310,7 @@  CONFIG_RTE_PMD_PACKET_PREFETCH=y
 
 #
 # Compile generic crypto device library
+# EXPERIMENTAL: API may change without prior notice
 #
 CONFIG_RTE_LIBRTE_CRYPTODEV=y
 CONFIG_RTE_LIBRTE_CRYPTODEV_DEBUG=n
diff --git a/config/common_linuxapp b/config/common_linuxapp
index eaad8d6..2866986 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -319,6 +319,7 @@  CONFIG_RTE_PMD_PACKET_PREFETCH=y
 
 #
 # Compile generic crypto device library
+# EXPERIMENTAL: API may change without prior notice
 #
 CONFIG_RTE_LIBRTE_CRYPTODEV=y
 CONFIG_RTE_LIBRTE_CRYPTODEV_DEBUG=n
diff --git a/doc/guides/contributing/versioning.rst b/doc/guides/contributing/versioning.rst
index 8a739dd..653c7d0 100644
--- a/doc/guides/contributing/versioning.rst
+++ b/doc/guides/contributing/versioning.rst
@@ -13,6 +13,7 @@  General Guidelines
 ------------------
 
 #. Whenever possible, ABI should be preserved
+#. The libraries marked in experimental state may change without constraint.
 #. The addition of symbols is generally not problematic
 #. The modification of symbols can generally be managed with versioning
 #. The removal of symbols generally is an ABI break and requires bumping of the
diff --git a/doc/guides/rel_notes/release_2_2.rst b/doc/guides/rel_notes/release_2_2.rst
index 8c77768..d0a9955 100644
--- a/doc/guides/rel_notes/release_2_2.rst
+++ b/doc/guides/rel_notes/release_2_2.rst
@@ -17,6 +17,16 @@  New Features
 
 * **Added keepalive support to EAL and example application.**
 
+* **Added experimental cryptodev API**
+
+  The cryptographic processing of packet is provided as a preview
+  with two drivers for:
+
+  * Intel QuickAssist devices
+  * Intel AES-NI multi-buffer library
+
+  Due to its experimental state, the API may change without prior notice.
+
 * **Added ethdev API to support IEEE1588.**
 
   Added functions to read, write and adjust system time in the NIC.
diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
index 04bade7..aa9f785 100644
--- a/lib/librte_cryptodev/rte_cryptodev.h
+++ b/lib/librte_cryptodev/rte_cryptodev.h
@@ -39,6 +39,9 @@ 
  *
  * Defines RTE Crypto Device APIs for the provisioning of cipher and
  * authentication operations.
+ *
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
  */
 
 #ifdef __cplusplus