> -----Original Message-----
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Github ODP bot
> Sent: Friday, August 04, 2017 4:00 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH API-NEXT v8 1/1] comp: compression spec
> 
> From: Shally Verma <shally.ve...@cavium.com>
> 
> Signed-off-by: Shally Verma <shally.ve...@cavium.com>
> Signed-off-by: Mahipal Challa <mahipal.cha...@cavium.com> Cc
> prasadathreya.naray...@cavium.com
> ---
> /** Email created from pull request 102 (1234sv:api-next)
>  ** https://github.com/Linaro/odp/pull/102
>  ** Patch: https://github.com/Linaro/odp/pull/102.patch
>  ** Base sha: 8390f890d4bd2babb63a24f7b15d2f4763e44050
>  ** Merge commit sha: fbdff8c82a19f5b640ae299204b3bb1bbbefdccb
>  **/
>  include/odp/api/spec/comp.h | 815
> ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 815 insertions(+)
>  create mode 100644 include/odp/api/spec/comp.h
> 
> diff --git a/include/odp/api/spec/comp.h b/include/odp/api/spec/comp.h
> new file mode 100644
> index 00000000..2956094c
> --- /dev/null
> +++ b/include/odp/api/spec/comp.h
> @@ -0,0 +1,815 @@
> +/* Copyright (c) 2013, Linaro Limited

Year 2017

> +
> +/**
> + * Comp API hash algorithm
> + *
> + */
> +typedef enum {
> +     /** ODP_COMP_HASH_ALG_NONE*/

This kind of comment is not very helpful. Each enumeration needs explanation - 
like odp_comp_alg_t under.

> +     ODP_COMP_HASH_ALG_NONE,
> +     /** ODP_COMP_HASH_ALG_SHA1*/
> +     ODP_COMP_HASH_ALG_SHA1,
> +     /**  ODP_COMP_HASH_ALG_SHA256*/
> +     ODP_COMP_HASH_ALG_SHA256
> +} odp_comp_hash_alg_t;
> +
> +/**
> + * Comp API compression algorithm
> + *
> + */
> +typedef enum {
> +     /** No algorithm specified.
> +      * Means no compression, output == input.
> +      * if provided, no operation (compression/decompression or
> hash)
> +      * applied on input. Added for testing purpose.
> +      */
> +     ODP_COMP_ALG_NULL,
> +     /** DEFLATE - RFC1951 */
> +     ODP_COMP_ALG_DEFLATE,
> +     /** ZLIB - RFC1950 */
> +     ODP_COMP_ALG_ZLIB,
> +     /** LZS */
> +     ODP_COMP_ALG_LZS
> +} odp_comp_alg_t;
> +
> +
> +/**
> + * Hash algorithms in a bit field structure
> + *
> + */
> +typedef union odp_comp_hash_algos_t {
> +     /** hash algorithms */
> +     struct {
> +             /** SHA-1 */
> +             uint32_t sha1  : 1;
> +
> +             /** SHA with 256 bits of Message Digest */
> +             uint32_t sha256 : 1;


Need to be more explicit in algorithm definition: SHA-1, SHA-256, ... algorithm 
(SHA-2 would also do, but we use SHA-256 in crypto API since it seems to be 
used by standards).

Actually, these explanations should go under enum definitions and then just 
refer to those enums here - like odp_comp_algos_t under.


> +
> +     } 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_hash_algos_t;
> +
> +/**
> + * Comp algorithms in a bit field structure
> + *
> + */
> +typedef union odp_comp_algos_t {
> +     /** Compression algorithms */
> +     struct {
> +             /** ODP_COMP_ALG_NULL */
> +             uint32_t null       : 1;
> +
> +             /** ODP_COMP_ALG_DEFLATE */
> +             uint32_t deflate    : 1;
> +
> +             /** ODP_COMP_ALG_ZLIB */
> +             uint32_t zlib       : 1;
> +
> +             /** ODP_COMP_ALG_LZS */
> +             uint32_t lzs        :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_algos_t;
> +
> +/**
> + * Compression Interface Capabilities
> + *
> + */
> +typedef struct odp_comp_capability_t {
> +     /** Maximum number of  sessions */
> +     uint32_t max_sessions;
> +
> +     /** Supported compression algorithms */
> +     odp_comp_algos_t comp_algs;

No need to save one character => comp_algos

> +
> +     /** Supported hash algorithms. */
> +     odp_comp_hash_algos_t hash_algs;

hash_algos

> +
> +     /* sync/async mode of operation support.
> +      * Implementation should support atleast one of the mode.
> +      */


"mode" field definition missing on this line ?


> +
> +     /** Support type for synchronous operation mode
> (ODP_COMP_SYNC).
> +      *  User should set odp_comp_session_param_t:mode based on
> +      *  support level as indicated by this param.
> +      */
> +     odp_support_t sync;
> +
> +     /** Support type for asynchronous operation mode
> (ODP_COMP_ASYNC).
> +      *  User should set odp_comp_session_param_t:mode param based
> on
> +      *  support level as indicated by this param.
> +      */
> +     odp_support_t async;
> +} odp_comp_capability_t;
> +
> +/**
> + * Hash algorithm capabilities
> + *
> + */
> +typedef struct odp_comp_hash_alg_capability_t {
> +     /** Digest length in bytes */
> +     uint32_t digest_len;
> +} odp_comp_hash_alg_capability_t;
> +
> +/**
> + * Compression algorithm capabilities structure for each algorithm.
> + *
> + */
> +typedef struct odp_comp_alg_capability_t {
> +     /** Enumeration indicating alg support for dictionary load */
> +     odp_support_t support_dict;
> +
> +     /** Optional Maximum length of dictionary supported
> +      *   by implementation of the algorithm.
> +      *
> +      *   Invalid if support_dict == ODP_SUPPORT_NO.
> +      *
> +      *   Implementation use dictionary of length less than or equal
> to value
> +      *   indicated by dict_len. if set to 0 and if support_dict ==
> +      *   ODP_SUPPORT_YES, then implementation will use dictionary
> length
> +      *   less than or equal to user input length in
> odp_comp_set_dict()
> +      *   and update used dictionary length at output of the call.
> +      *
> +      */
> +     uint32_t dict_len;
> +
> +     /* Maximum compression level supported by implementation of
> this algo.
> +      *  Indicates number of compression levels supported by
> implementation,
> +      *
> +      * where,
> +      *
> +      * 1 means fastest compression i.e. output produced at
> +      * best possible speed at the expense of compression quality,
> and
> +      *
> +      * max_level means best compression i.e.output produced is best
> possible
> +      * compressed content at the expense of speed.
> +      *
> +      * Example, if max_level = 4 , it means algorithm supports four
> levels
> +      * of compression from value 1 up to 4. User can set this value
> from
> +      * 1 (fastest compression) to 4 (best compression).
> +      * See RFC1950 for an example explanation to level.
> +      *
> +      * Value 0 mean implementation use its default value.
> +      *
> +      */
> +     uint32_t max_level;
> +
> +     /* Supported hash algorithms */
> +     odp_comp_hash_algos_t hash_alg;
> +} odp_comp_alg_capability_t;
> +
> +/**
> + * Comp API dictionary type
> + *
> + */
> +typedef struct odp_comp_dict_t {
> +     /** pointer to character array */
> +     uint8_t *buf;

Is the data input only? If it is, then this should be const data pointer.

What is expected about the format of the data? Null terminated strings? If it's 
strings, then e.g. const char * would be better.


> +     /** length of the dictionary. */
> +     uint32_t len;
> +} odp_comp_dict_t;
> +
> +/**
> + * Comp API algorithm specific parameters
> + *
> + */
> +typedef struct odp_comp_alg_param_t {
> +     struct comp_alg_def_param {
> +             /** compression level where
> +              * ODP_COMP_LEVEL_MIN <= level <= ODP_COMP_LEVEL_MAX
> +              */
> +             odp_comp_level_t level;
> +             /** huffman code to use */
> +             odp_comp_huffman_code_t comp_code;
> +     } deflate;

Doxygen comment missing? Is this filled in only if algorithms is ALG_DEFLATE ?


> +     struct comp_alg_zlib_param {
> +                     /** deflate algo params */
> +                     struct comp_alg_def_param def;
> +     } zlib;

Same here.

> +} odp_comp_alg_param_t;
> +
> +/**
> + * Comp API data range specifier
> + *
> + */
> +typedef union odp_comp_data_t {
> +     struct {
> +             /** packet */
> +             odp_packet_t packet;
> +
> +             /** packet data range to operate on  */
> +             odp_packet_data_range_t data_range;


Barry have indicated use case for defining multiple ranges per packet. How that 
will be handled?


> +     } pkt;
> +} odp_comp_data_t;


Packet is quite deep in parameter structures. E.g. in compress call

param.input.pkt.packet            = pkt;
param.input.pkt.data_range.offset = 0;
param.input.pkt.data_range.len    = len;

odp_comp_compress(&param, &result)



I'd pull up the packet handle(s) from param, and maybe have separate calls for 
full packets and ranges:

int odp_comp_packet(const odp_packet_t pkt_in[],
                    odp_packet_t pkt_out[],
                    const odp_comp_op_param_t param[],
                    int num_pkt);

typedef struct odp_comp_packet_range_t {
        int num_range;

        struct {
                uint32_t offset;
                uint32_t len;
        } range[];

} odp_comp_packet_range_t;

int odp_comp_packet_range(const odp_packet_t pkt_in[],
                          const odp_comp_packet_range_t range_in[],
                          odp_packet_t pkt_out[],
                          // also range_out needed ??
                          const odp_comp_op_param_t param[],
                          int num_pkt);


Then later on, direct memory address input/output would look something like 
this:

int odp_comp_mem(uint8_t *ptr_in[],
                 uint32_t len_in[],
                 uint8_t *ptr_out[],
                 uint32_t *len_out[],
                 const odp_comp_op_param_t param[],
                 int num_ptr);


> +
> + /**
> + * 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_compress()/odp_comp_decomp()
> +      * should be called.
> +      *
> +      * When mode = ODP_COMP_ASYNC, odp_comp_compress_enq()/
> +      * odp_comp_decomp_enq() should be called.
> +      *
> +      * Use odp_comp_capability() for supported mode.
> +      *
> +      */
> +     odp_comp_op_mode_t mode;
> +
> +     /** Compression algorithm
> +      *
> +      *  Use odp_comp_capability() for supported algorithms.
> +      */
> +     odp_comp_alg_t comp_alg;
> +
> +     /** Hash algorithm
> +      *
> +      *  Use odp_comp_alg_capability() for supported hash algo for
> +      *  compression algo given as comp_alg. Implementation should
> not
> +      *  support hash only operation on data. output should always
> contain
> +      *  data + hash.
> +      *
> +      */
> +     odp_comp_hash_alg_t hash_alg;
> +
> +     /** parameters specific to compression */
> +     odp_comp_alg_param_t alg_param;
> +
> +     /** Async mode completion event queue
> +      *
> +      * When mode = ODP_COMP_ASYNC, user should wait on
> ODP_EVENT_PACKET
> +      * with subtype ODP_EVENT_PACKET_COMP 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;
> +} 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, where
> +      *
> +      *   true : last chunk
> +      *
> +      *   false: more to follow
> +      *
> +      * If set to true, indicates this is the last chunk of
> +      * data. After processing of last chunk of data is complete
> i.e.
> +      * call returned with any error code except
> ODP_COMP_ERR_OUT_OF_SPACE,
> +      * implementation should move algorithm to stateless mode
> +      * for next of batch of operation i.e. reset history,
> +      * insert 'End of Block' marker into compressed data stream(if
> +      * supported by algo).See deflate/zlib for interpretation of
> +      * stateless/stateful.
> +      *
> +      * For stateless compressions (ex ipcomp), last should be set
> to 'true'
> +      * for every input packet processing call.
> +      *
> +      * For compression + hash, digest will be available after
> +      * last chunk is processed completely. In case of
> +      * ODP_COMP_ERR_OUT_OF_SPACE, application should keep on
> calling
> +      * odp_comp_xxx() API with more output buffer unless call
> returns
> +      * with ODP_COMP_ERR_NONE or other failure code except
> +      *  ODP_COMP_ERR_OUT_OF_SPACE.
> +      */
> +     odp_bool_t last;
> +
> +     /** Input data */
> +     odp_comp_data_t input;

I'd move this out of the struct, since it's not operation parameter but the 
target of the operation. From call to call, everything else may remain 
constant, but the target is not constant.

> +
> +     /** placeholder for output data.
> +      *
> +      * For Compression/Decompression+hash session,
> +      * output  will store both data and digest(with digest appended
> at
> +      * end-of-data). User should pass packet of sufficiently large
> size
> +      * to store digest.
> +      *
> +      */
> +     odp_comp_data_t output;

Same here.

> +} 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.Valid when odp_comp_err_t is
> +      * ODP_COMP_ERR_NONE or ODP_COMP_ERR_OUT_OF_SPACE
> +      *
> +      * Contain data after compression/decompression operation,
> +      * or data + digest for compression/decompression + hash
> operation.
> +      *
> +      */
> +     odp_comp_data_t output;


This would also go out of the struct.


> +} odp_comp_op_result_t;
> +

-Petri


Reply via email to