On 19.04.2017 17:02, Verma, Shally wrote:
>
>
> -----Original Message-----
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Verma,
> Shally
> Sent: 19 April 2017 19:30
> To: Dmitry Eremin-Solenikov <dmitry.ereminsoleni...@linaro.org>; Shally Verma
> <shally.ve...@gmail.com>; lng-odp@lists.linaro.org
> Cc: Challa, Mahipal <mahipal.cha...@cavium.com>; Masood, Faisal
> <faisal.mas...@cavium.com>; Narayana, Prasad Athreya
> <prasadathreya.naray...@cavium.com>
> Subject: Re: [lng-odp] [RFC, API-NEXT v2 1/1] comp:compression interface
>
>
>
> -----Original Message-----
> From: Dmitry Eremin-Solenikov [mailto:dmitry.ereminsoleni...@linaro.org]
> Sent: 19 April 2017 17:36
> To: Shally Verma <shally.ve...@gmail.com>; lng-odp@lists.linaro.org
> Cc: Challa, Mahipal <mahipal.cha...@cavium.com>; Masood, Faisal
> <faisal.mas...@cavium.com>; Narayana, Prasad Athreya
> <prasadathreya.naray...@cavium.com>; Verma, Shally <shally.ve...@cavium.com>
> Subject: Re: [lng-odp] [RFC, API-NEXT v2 1/1] comp:compression interface
>
> On 19.04.2017 13:00, Shally Verma wrote:
>> An API set to add compression/decompression support in ODP interface.
>>
>> Signed-off-by: Shally Verma <sve...@cavium.com>
>> Signed-off-by: Mahipal Challa <mcha...@cavium.com>
>> ---
>> include/odp/api/spec/comp.h | 748
>> ++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 748 insertions(+)
>>
>> diff --git a/include/odp/api/spec/comp.h b/include/odp/api/spec/comp.h
>> new file mode 100644 index 0000000..65feacf
>> --- /dev/null
>> +++ b/include/odp/api/spec/comp.h
>> @@ -0,0 +1,748 @@
>> +/*
>> + */
>> +
>> +/**
>> + * @file
>> + *
>> + * ODP Compression
>> + */
>> +
>> +#ifndef ODP_API_COMP_H_
>> +#define ODP_API_COMP_H_
>> +#include <odp/visibility_begin.h>
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>> +/** @defgroup odp_compression ODP COMP
>> + * ODP Compression defines interface to compress/decompress and
>> +authenticate
>> + * data.
>> + *
>> + * Compression here implicilty refer to both compression and decompression.
>> + * Example, compression algo 'deflate' mean both 'deflate' and 'inflate'.
>> + *
>> + * if opcode = ODP_COMP_COMPRESS, then it will Compress,
>> + * if opcode = ODP_COMP_DECOMPRESS, then it will Decompress.
>> + *
>> + * Current version of Interface allow Compression
>> +ONLY,Authentication ONLY or
>> + * both Compression + Auth ONLY sessions.
>> + *
>> + * Macros, enums, types and operations to utilise compression.
>> + * @{
>> + */
>> +
>> +/**
>> + * @def ODP_COMP_SESSION_INVALID
>> + * Invalid session handle
>> + */
>> +
>> +/**
>> + * @typedef odp_comp_session_t (platform dependent)
>> + * Comp API opaque session handle
>> + */
>> +
>> +/**
>> + * @typedef odp_comp_compl_t
>> +* Compression API completion event (platform dependent) */
>> +
>> +/**
>> + * Compression API operation mode
>> + */
>> +typedef enum {
>> + /** Synchronous, return results immediately */
>> + ODP_COMP_SYNC,
>> + /** Asynchronous, return results via queue event */
>> + ODP_COMP_ASYNC,
>> +} odp_comp_op_mode_t;
>> +
>> +/**
>> + * Comp API operation type.
>> + * Ignored for Authentication ONLY.
>> + *
>> + */
>> +typedef enum {
>> + /** Compress and/or Compute digest */
>> + ODP_COMP_OP_COMPRESS,
>> + /** Decompress and/or Compute digest */
>> + ODP_COMP_OP_DECOMPRESS,
>> +} odp_comp_op_t;
>> +
>> +/**
>> + * Comp API authentication algorithm
>> + *
>> + */
>> +typedef enum {
>> + /** No authentication algorithm specified */
>> + ODP_COMP_AUTH_ALG_NULL,
>> + /** ODP_COMP_AUTH_ALG_SHA1*/
>> + ODP_COMP_AUTH_ALG_SHA1,
>> + /** ODP_COMP_AUTH_ALG_SHA256*/
>> + ODP_COMP_AUTH_ALG_SHA256
>> +} odp_comp_auth_alg_t;
>
> Why do you need special comp_auth_alg instead of just odp_auth_alg_t?
> Shally - To differentiate that these algorithm are specifically part of
> compression interface as crypto also has them enumerated.
> Correction - Crypto doesn't enumerate them "as is" in current api-next as I
> see it.
So, what is particularly 'compression' in them, which is not 'crypto'?
>
>> +/**
>> +* Comp API enumeration for preferred compression level/speed.
>> +*
>> +* trade-off between speed and compression ratio.
>> +*
>> +* Please note this enumeration is only a peferential selection
>> +* but may not guarantee operation to committed level depending
>> +* on implementation support. Ex. SPEED_FASTEST and SPEED_FAST may
>> +* give same result on some platforms.
>> +*
>> +*/
>> +typedef enum {
>> + /* Use implementaion default between ratio and speed */
>> + ODP_COMP_SPEED_DEFAULT = 0,
>> + /** fastest speed, lowest compression */
>> + ODP_COMP_SPEED_FASTEST,
>> + /** fast speed, lower compression */
>> + ODP_COMP_SPEED_FAST,
>> + /** medium speed, medium compression */
>> + ODP_COMP_SPEED_MED,
>> + /** slowest speed, maximum compression */
>> + ODP_COMP_SPEED_SLOWEST,
>> +
>> +} odp_comp_speed_t;
>
> It might be easier to specify this as unsigned integer and
>
> Shally-These levels (in its current stage) are specific to ZLIB RFC 1950 and
> added so that user can exercise these ZLIB algorithm parameters during zlib
> compression.
> Though they can be used in generic sense to alter and avail speed levels of
> other algorithms but depending on implementation, granularity may/may not be
> achievable. Thus it is marked as 'preferable' parameter only.
Exactly. Thus the amount of levels should be an algorithm capability.
You should not tie compression to ZLIB.
> "max" speed level specific to exact algorithm implementation acquired through
> capabilities.
>
> Shally -As said these speed level don't mean to indicate per algorithm speed.
> If required, we may consider to add maximum achievable ratio/time values per
> algorithm, which will be more of an absolute numbers but different from these
> speed enumeration.
If so, then why do we have them in generic(!) structure in the first
place? And why are they called ODP_COMP_SPEED_smth, rather than just
ODP_ZLIB_smth?
>
>> +
>> +/**
>> + * Comp API enumeration for encoding type
>> + *
>> + */
>> +typedef enum {
>> + /** use implementation default to choose between compression codes */
>> + ODP_COMP_CODE_DEFAULT = 0,
>> + /** use fixed huffman codes */
>> + ODP_COMP_CODE_FIX_HUFFMAN,
>> + /** use dynamic huffman coding */
>> + ODP_COMP_CODE_DYN_HUFFMAN,
>> +} odp_comp_code_t;
>
> Isn't this too specific?
>
> Shally - yes. These are zlib/deflate standard specific and these all can be
> negotiated / chosen by upper layer app.
Then this should go to protocol capabilities/settings. Or one can add
protocol-specific part to parameters. But the enum name (and parameters
structure) should be clear that it applies only to this-and-that algorithms.
>> +
>> +/**
>> + * Authentication algorithms in a bit field structure
>> + *
>> + */
>> +typedef union odp_comp_auth_algos_t {
>> + /** Authentication algorithms*/
>> + struct {
>> + /** ODP_AUTH_ALG_NULL*/
>> + uint32_t null : 1;
>> +
>> + /** SHA-1*/
>> + uint32_t sha1 : 1;
>> +
>> + /** SHA-2 with 256 bits of Message Digest*/
>> + uint32_t sha256 : 1;
>> +
>> + } bit;
>> +
>> + /** All bits of the bit field structure
>> + *
>> + * This field can be used to set/clear all flags, or bitwise
>> + * operations over the entire structure.
>> + */
>> + uint32_t all_bits;
>> +} odp_comp_auth_algos_t;
>
> Any real benefit over odp_crypto_auth_algos_t?
>
> Shally - Just that cavium ZIP hardware perform both in one shot providing
> faster operations. So, we see its use for application using only compression
> interface and need SHA as well.
> Also, don't think crypto provide sha-only support.
Our ZIP can do this and can not do SHA3-512, sure. But compression API
should not be tied to Cavium only hardware. So Cavium imlementation can
return just SHA-1 and SHA-256 bits when asked for supported algorithms
through compression API. But I see little point in re-enumerating
algorithms here just because one hardware instance doesn't support other
algorithms.
>> +/**
>> + * Authentication algorithm capabilities
>> + *
>> + */
>> +typedef struct odp_comp_auth_alg_capability_t {
>> + /** key size in bytes */
>> + uint32_t key_size;
>> +
>> + /** Digest length in bytes */
>> + uint32_t digest_len;
>
> Ideally this cap should go to crypto interface.
>
> Shally - Compression is kept independent of crypto and i.e. the reason we
> added compression specific data types. By keeping crypto references into
> compression, will create dependency which is not a preferred scenario.
This is nice point. I understand that you would have implementation
separate from crypto. But types should be the same.
Now about digest_len. I'd prefer it in the crypto API because it is a
crypto property, even if you have separate implementation.
>> +
>> + /** Optional NULL-terminated string description
>> + * message (platform-dependent).
>> + */
>> + void *description;
>> +
>> + /** Optional Null-terminated string
>> + * identifier of the algo (platform-dependent)
>> + */
>> + void *name;
>
> Why do you need those two fields?
> Shally - these are optional, added for diagnostic purpose.
Then you can just drop them and have diagnostic names inside an
application, rather than in compression API. Notice: crypto (after which
you've modelled your API) does not have such diagnotics, does it?
>> +/**
>> + * Comp API data range specifier
>> + *
>> + */
>> +typedef struct odp_comp_data_range_t {
>> + /** Offset from beginning of input */
>> + uint32_t offset;
>> +
>> + /** Length of data to operate on */
>> + uint32_t length;
>> +} odp_comp_data_range_t;
>
> Please use odp_crypto_data_range_t instead.
>
> Shally - as explained above.
>
> Maybe it would be a good idea to have generic 'odp_data_range_t' instead,
> which would be used by compression and crypto interfaces.
> Shally - ya. This sounds a better idea. As more interfaces are added using
> common datatypes, it may be thought of generalizing them. SHA is one of them.
>
Then please go ahead and introduce odp_data_range_t, porting crypto to
use it.
>> +
>> +/**
>> + * Comp API key structure
>> + *
>> + */
>> +typedef struct odp_comp_auth_key_t {
>> + /** Key */
>> + uint8_t *key;
>> +
>> + /** Key length in bytes */
>> + uint32_t length;
>> +} odp_comp_auth_key_t;
>
> Just odp_crypto_key_t please.
> Shally - Again we would not like to bind dependency on crypto for compression
> usage.
It is not a dependency. It is just a structure name. No need in
duplicating definitions for the same thing.
>
>> +
>> + /**
>> + * Comp API session creation parameters
>> + *
>> + */
>> +typedef struct odp_comp_session_param_t {
>> + /** Compress vs. Decompress operation */
>> + odp_comp_op_t op;
>> +
>> + /** Sync vs Async
>> + *
>> + * When mode = ODP_COMP_SYNC, odp_comp_operation() behaves
>> + * synchronously. Operation results will be available as the call
>> + * returns.
>> + *
>> + * When mode = ODP_COMP_ASYNC, odp_comp_operation() behaves
>> + * asynchronously. Operation results will be notified via
>> + * ODP_COMP_COMPL_EVENT enqueued into completion queue.
>> + *
>> + * Before passing, User should ensure given
>> + * mode is supported by implementation via call to
>> + * odp_comp_capability_query().
>> + *
>> + * Set mode to ODP_COMP_ASYNC, if
>> + * odp_comp_capability_t:async is set to true
>> + *
>> + *
>> + * Set mode to ODP_COMP_SYNC, if
>> + * odp_comp_capability_t:sync is set to true
>> + *
>> + */
>> + odp_comp_op_mode_t mode;
>
> As said before, please switch to IPsec-like interface, where there are
> separate functions for SYNC and ASYNC modes.
>
> Shally - Ok. But I have a question here.
> Once session is created in sync/async mode, API would always work in that
> mode which is equivalent to having two separate calls.
> My intention was to keep API count minimal until there is huge change in API
> signature and definitions.
> Since in current scenario, API carry almost same signature and meaning so why
> we prefer having two than one? Is it because of code design consistency?
Because then there is no way for the implementation to return 'posted'
if it was asked to work in SYNC mode and vice versa.
>> +
>> + /** Compression algorithm
>> + *
>> + * Use odp_comp_capability() for supported algorithms.
>> + */
>> + odp_comp_alg_t comp_alg;
>> +
>> + /** Compression preferred encoding mode
>> + *
>> + */
>> + odp_comp_code_t comp_code;
>> +
>> + /** Compression preferred speed
>> + *
>> + */
>> + odp_comp_speed_t comp_speed;
>> +
>> + /** Authentication algorithm.
>> + *
>> + * if enabled along with non-null compression algo, follows a fixed
>> + * ordering. it will be applied on plaintext i.e.
>> + * before data is compressed for compression session, and
>> + * after it is decompressed for decompression session.
>> + *
>> + * This may later be enhanced to allow re-ordering subject to
>> + * explicitly defined use-case.
>> + */
>> + odp_comp_auth_alg_t auth_alg;
>> +
>> + /** auth parameters during session creation*/
>> + odp_comp_auth_key_t auth_key;
>> +
>> + /** Async mode completion event queue
>> + *
>> + * When mode = ODP_COMP_ASYNC, User should wait on ODP_COMP_COMPL_EVENT
>> + * on this queue.
>> + *
>> + * By default, implementation enques completion events in-order-of
>> + * request submission and thus queue is considered ordered.
>> + *
>> + * Please note, behavior could be changed or enhanced
>> + * to queue event in-order-of their completion to enable
>> + * performance-oriented application to leverage hw offered parallelism.
>> + * However, This will be subject to application requirement and more
>> + * explicit defined use-case.
>> + *
>> + */
>> + odp_queue_t compl_queue;
>> +
>> + /** Pool used by user for input/output data
>> + *
>> + * used by implementation to have user pool information.
>> + * Helps to map input/output data pointer to appropriate type
>> + * and other pool related operations.
>> + *
>> + */
>> + odp_pool_t pool;
>
> What is the usage of this pool? Your commentary isn't easy to parse.
>
> Shally - This is kept as placeholder for future extension to
> odp_comp_operation(). Currently interface expects separate input and output
> packets. However, if required interface could be extended to support
> auto-allocation of output buffer (as in crypto). Considering such case in
> mind, we kept user pool as input during session creation.
Then you can add necessary pools later, when there is need for them.
>
>> +} odp_comp_session_param_t;
>> +
>> +/**
>> + * Comp API operation parameters.
>> + * Called to process each data unit.
>> + *
>> + */
>> +typedef struct odp_comp_op_param_t {
>> + /** Session handle from creation */
>> + odp_comp_session_t session;
>> +
>> + /** User context */
>> + void *ctx;
>> +
>> + /** Boolean indicating End of data
>> + * for stateless compressions (ex ipcomp), it should always be 'true'
>> + *
>> + * true : last chunk
>> + * false: more to follow
>> + *
>> + * for compression+authentication, digest will be available after
>> + * last chunk is processed completely.
>> + */
>> + odp_bool_t last;
>
> Maybe it'd be better named 'flush' instead of 'last'?
> Shally - 'last' indicate last chunk of data and needed to indicate
> end-of-operation. It does not have similar meaning as 'flush' as data will
> get flushed regular basis per call of operation.
For stateless there is no 'last' packet, but rather a request to 'flush'
all before processing of this packet ends, isn't it?
Can you continue sending more data after 'last' packet was sent in
stateful mode?
>> +
>> + /** Input packet*/
>> + odp_packet_t in_data;
>> +
>> + /** input data range w.r.t in_data*/
>> + odp_comp_data_range_t in_data_range;
>> +
>> + /** Output packet.
>> + * Stores processed output.
>> + *
>> + * For Compression+Authentication session,
>> + * output buffer will store both data and digest(with digest appended at
>> + * end-of-data). User should pass packet of sufficiently large size
>> + * to store digest.
>> + *
>> + * Auto-allocation of output buffer( i.e. when out_data set to
>> + * ODP_PACKET_INVALID) and/or in-place operation support
>> + * to be considered later based on explicit defined use-case.
>> + *
>> + */
>> + odp_packet_t out_data;
>
> If you do not allow auto allocation of output packet, I don't see why would
> you need a pool in odp_comp_session_param_t.
>
> Shally - Yes. This is feature to-be considered at this point.
Then either please add API now, or drop it, so that we won't have API
with mixed state 'this is unused now, but can and will be used in
future. maybe'.
>> +
>> + /** output data range w.r.t out_data, where
>> + *
>> + * At input, length specifies length of available output buffer, and
>> + * offset marks beginning of output buffer.
>> + *
>> + * At output, length is updated to length of data written in output, and
>> + * offset marks start of the data.
>> + *
>> + * For Compression+Authentication session, Both data and digest are
>> + * written to same output buffer (with digest written at the end of
>> + * data), so data length = out_data_range.len-digest_size.
>
> I'd suggest to have separate data range for digest algo. It is more future
> proof.
>
>
> Shally - keeping single output buffer for each operation makes logic
> consistent.
> output buffer will carry compressed data for compression only session, digest
> for authentication only and compressed+digest both for compression+auth
> session.
What about possible compression protocols that would prefer to have some
data between compressed data and auth value?
>> + *
>> + */
>> + odp_comp_data_range_t out_data_range;
>> +
>> + /** dictionary - Optional Pointer to dictionary as passed by user.
>> + * User should pass dictionary when interface is in idle state i.e.
>> + * no operation is in progress.
>> + */
>> + uint8_t *dict;
>
> What is the format of the dictionary? Also the requirement of 'idle'
>
> Shally - dictionary is just byte-stream of characters carrying pattern
> matching to data that follows.
Not that it is specified in the API. Or the format of that byte stream.
Which algorithms would use this dict? Or in other words: would any
algorithm make use of this dictionary?
> Injecting dictionary in-middle-of operation leads to erroneous results, thus
> there should be no compression/decompression operation in progress.
>
> state seems quite troublesome. Maybe it should go to odp_comp_session_param_t
> ?
> Shally - There are application use cases in compressions, where dictionary
> may be injected/refreshed on a regular basis. This does not need session
> destroy-creation cycle.
Could you please be more specific on this? How do you plan to make sure
that there are no parallel calls in other threads using this session?
Also if the dictionary is a state property, not a packet operation
property, dict updating should go to a separate call.
>> +
>> + /** Length of dictionary */
>> + uint32_t dict_len;
>> +
>> +} odp_comp_op_param_t;
>> +
>> +/**
>> + * Comp API per operation result
>> + *
>> + */
>> +typedef struct odp_comp_op_result_t {
>> + /** User context from request */
>> + void *ctx;
>> +
>> + /** Operation Return Code */
>> + odp_comp_err_t err;
>> +
>> + /** pointer to output buffer. Valid when odp_comp_err_t is
>> + * ODP_COMP_ERR_NONE
>> + *
>> + * Contains digest for authentication ONLY operations,
>> + * processed output for compression/decompression operations, and
>> + * both data and digest for Compression+Authentication Operations.
>> + *
>> + */
>> + odp_packet_t out_data;
>> +
>> + /** length of data written to output packet.
>> + * include both data+digest_len, for compression+authentication
>> + * session
>> + */
>> + odp_comp_data_range_t data_range;
>> +} odp_comp_op_result_t;
>
> So, here we have 1 packet per request. Would we like to support multiple
> packets per request instead?
> Shally - Do you mean multiple packets where each packet to be compressed
> independently?
Yes. Burst packets processing. When you get several packets from
interface, pass them to compression in one chunk and then to output
iface also in a chunk.
> Besides, there's document in google docs
> https://docs.google.com/document/d/1aaEo8oTvZvm2096GJzl165L571tcsKqfcaD5yypiiCA/edit?usp=sharing
> It gives an overview of current odp comp interface use case and usage. It
> should help explain and answer some of the points above.
> Please let me know if there're any issues in accessing it.
The document is a design document. API should be understandable and
usable without additional documents. If necessary you should add
necessary data to API documentation.
--
With best wishes
Dmitry
