Shally Verma(1234sv) replied on github web page:
include/odp/api/spec/comp.h
line 30
@@ -0,0 +1,873 @@
+/* Copyright (c) 2017, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP Compression
+ */
+
+#ifndef ODP_API_COMP_H_
+#define ODP_API_COMP_H_
+
+#include <odp/visibility_begin.h>
+#include <odp/api/support.h>
+#include <odp/api/packet.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/** @defgroup odp_compression ODP COMP
+ * ODP Compression is an API set to do compression+hash or decompression+hash
+ * operations on data. Hash is calculated on plaintext.
+ *
+ * if opcode = ODP_COMP_COMPRESS, then it will apply hash and then compress,
+ * if opcode = ODP_COMP_DECOMPRESS, then it will decompress and then apply
+ * hash.
Comment:
" This should be specified at operation documentation" means which place? user
guide Or ?
Do you mean I should remove this description from here? Or
"+ * if opcode = ODP_COMP_COMPRESS, then it will apply hash and then compress,
+ * if opcode = ODP_COMP_DECOMPRESS, then it will decompress and then apply
+ * hash."
> Shally Verma(1234sv) wrote:
> First, async call can reuse sync call spec by saying: "... processes packets
> otherwise identically to odp_comp_op(), but outputs resulting packets as
> ODP_EVENT_PACKET events ...". Then the remaining text can concentrate on what
> is unique on async vs sync operation.
>
> I think OUT_OF_SPACE spec might need re-thinking:
> 1) user sees a partial packet (error on a received packet)
> 2) implementation waits that user calls op_enq() again
> 3) what input packet should be given on the new call? I think the previous
> operation already consumed the input packet which contains the input data for
> operation. Did implementation store it?
> 4) how long implementation waits for new output packet to arrive from user,
> what if user don't send new output packets.
>
> Async spec (and also sync) spec would be more robust if implementation would
> allocate output packets.
>
>> Shally Verma(1234sv) wrote:
>> Application does not allocate buffers but packets.
>>
>> Does the partially processed packet contain valid data or not == does
>> application keep or discard the packet?
>>
>> Is there any way for application to prepare large enough packet in advance?
>> E.g. a per algo capability of worst case decompression ratio? Should
>> "pkt_out = INVALID" be supported? ==> implementation allocates large enough
>> packet.
>>
>> Single packet handle should **not** be defined as const.
>>
>> There is out_data_range defined in both op_param_t and op_result_t. Which
>> one is updated on output? May be operation output comments should be removed
>> from op_param_t documentation.
>>> Shally Verma(1234sv) wrote:
>>> Simply say that:
>>> This operation does de/-compression in synchronous mode (ODP_COMP_SYNC).
>>>> Shally Verma(1234sv) wrote:
>>>> "implementation should update length of dictionary used at output."
>>>> This is too vague: either length is updated or it's not. In which cases
>>>> the update would be done and what that would mean? Also, I assume that
>>>> implementation copies the data, so that application is free to overwrite
>>>> the memory after the call. That (copies data) should be mentioned also.
>>>>
>>>> Is there a benefit on output param here, especially if it updates only the
>>>> length and not the pointer. Maybe API would be cleaner, if also *dict is
>>>> input only param (and thus const*). Function may return e.g. number of
>>>> bytes copied/used.
>>>>> Shally Verma(1234sv) wrote:
>>>>> This can be simply
>>>>>
>>>>> odp_comp_session_t odp_comp_session_create(const odp_comp_session_param_t
>>>>> *param)
>>>>>
>>>>> Extra error codes may be returned through odp_errno(). Usually, we have
>>>>> not defined error codes, since it's implementation defines what all
>>>>> things can possibly go wrong, and typically application logic is just
>>>>> interested on success vs fail - not so much why we failed.
>>>>>
>>>>> This function prototype is copy-paste from crypto API, but all other APIs
>>>>> are the type I suggested above.
>>>>>
>>>>> Note: const pointer for "param"
>>>>>> Shally Verma(1234sv) wrote:
>>>>>> It would be good to define default values for odp_comp_session_param_t
>>>>>> (at the type documentation). Otherwise, application needs to always set
>>>>>> every param.
>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>> Can this return more than one hash_alg_capa (== digest_len) per
>>>>>>> hash_alg? Currently, application cannot select digest length. So, what
>>>>>>> application should do if this returns more than one capa.
>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>> Is array of capabilities needed with comp algorithms? It would be
>>>>>>>> needed if user would need to select between multiple discrete options
>>>>>>>> - e.g. if only some dict_len values would make sense (1kB, 4kB, 16kB),
>>>>>>>> but nothing in between.
>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>> "where length specifies,
>>>>>>>>> length of available packet buffer at input, and
>>>>>>>>> length of data written at output"
>>>>>>>>>
>>>>>>>>> This specification should be clarified. "Input" refers refers to
>>>>>>>>> out_data_range.length as operation input param and "output" refers
>>>>>>>>> to the same as operation output parameter.
>>>>>>>>>
>>>>>>>>> In addition to length, offset may be updated at operation output (at
>>>>>>>>> least when new packet was allocated). That should be mentioned also.
>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>> This last chuck:
>>>>>>>>>> "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. "
>>>>>>>>>>
>>>>>>>>>> Documents operation return / error codes rather than "last" field
>>>>>>>>>> usage. So, I'd remove it from here.
>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>> If only option for a stateless algorithm is true, then we should
>>>>>>>>>>> document that the value is ignored.
>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>> Typo: "enques" -> enqueues"
>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>> Remove this block text since it's speculation of what could be
>>>>>>>>>>>>> specified.
>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>> Typo: "... given as comp_alg" -> "... given as comp_algo"
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> There are two SHOULDs, but I think those are not needed:
>>>>>>>>>>>>>> 1) comp_algo is always set. It should be e.g. ALG_NULL by
>>>>>>>>>>>>>> default. We have defined that NULL does not have hash
>>>>>>>>>>>>>> capability. It would be also possible to leave NULL + hash to be
>>>>>>>>>>>>>> implementation defined capability. When we say that ALG_NULL is
>>>>>>>>>>>>>> only for test, it should be enough to discourage people from
>>>>>>>>>>>>>> misusing this API for hashing.
>>>>>>>>>>>>>> 2) What this means: "output should always contain data + hash" ?
>>>>>>>>>>>>>> If we enable hashing, hash is always calculated. It's not
>>>>>>>>>>>>>> optional for implementation to calculate it - right.
>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>> Field name should match the type == "hash_algos" (many
>>>>>>>>>>>>>>> algorithms in a bit field). Underneath "hash_algo" is used with
>>>>>>>>>>>>>>> enum odp_comp_hash_alg_t (one algo).
>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>> I did comment the same a month back:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Is the data input only? If it is, then this should be "const
>>>>>>>>>>>>>>>> uint8_t *buf"
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> What is expected about the format of the data? Null terminated
>>>>>>>>>>>>>>>> strings? If it's strings, then e.g. "const char *str" would be
>>>>>>>>>>>>>>>> better.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Also the type documentation should be updated:
>>>>>>>>>>>>>>>> * This is not dictionary type, but pointer into dictionary
>>>>>>>>>>>>>>>> data - right?
>>>>>>>>>>>>>>>> * "Consists of pointer to byte buffer. length of dictionary
>>>>>>>>>>>>>>>> indicated by length parameter. ", this needs clarification.
>>>>>>>>>>>>>>>> What is expected about the data behind the pointer?
>>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>>> @psavol Hmm. I thought that the user should use this value.
>>>>>>>>>>>>>>>>> And `odp_comp_level_t` should be dropped from the spec.
>>>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>>>> How this relates to odp_comp_level_t enumeration ? Shouldn't
>>>>>>>>>>>>>>>>>> user use those levels and not a number from 0 to this one ?
>>>>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>>>>> This is description of how de/-compression operation work.
>>>>>>>>>>>>>>>>>>> This should be specified at operation documentation, and
>>>>>>>>>>>>>>>>>>> optimally only there. Otherwise, we are in danger of this
>>>>>>>>>>>>>>>>>>> spec getting deprecated (== in conflict with operation
>>>>>>>>>>>>>>>>>>> documentation) without noticing.
>>>>>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>>>>>> True, all types need to be odp_ prefixed. Also this type
>>>>>>>>>>>>>>>>>>>> would be better to typedef and pull out (from inside of
>>>>>>>>>>>>>>>>>>>> algo_param_t) since it's used in two places already.
>>>>>>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>>>>>>> As a side note: it might be logical, we might want to add
>>>>>>>>>>>>>>>>>>>>> `odp_support_t` field to notify user if implementation
>>>>>>>>>>>>>>>>>>>>> supports stateful compression. I can easily assume that
>>>>>>>>>>>>>>>>>>>>> there might be implementations implementing only
>>>>>>>>>>>>>>>>>>>>> stateless compression.
>>>>>>>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>>>>>>>> odp_comp_alg_deflate_param, please.
>>>>>>>>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>>>>>>>>> Please check the see list. It mentions non-existing
>>>>>>>>>>>>>>>>>>>>>>> functions.
>>>>>>>>>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>>>>>>>>>> no result here
>>>>>>>>>>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>> s/Else/Otherwise/
>>>>>>>>>>>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>> ... an error....
>>>>>>>>>>>>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>> ... with valid pkt_out and pkt_in being
>>>>>>>>>>>>>>>>>>>>>>>>>>> ODP_PACKET_INVALID...
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>> It is an error to call this api when session was
>>>>>>>>>>>>>>>>>>>>>>>>>>>> created in ODP_COMP_ASYNC mode.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> As said, single and multi versions are possible,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> but not as proposed in this patch. Both versions
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> should have the same param structure and
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> specification. Only difference would be pkt_in[]
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> vs pkt_in, pkt_out[] vs pkt_out, param[] vs.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> param ...
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Then I propose to add a new API like
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> odp_comp_compress_multi(const odp_packet_t
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> pkt_in[], odp_packet_t pkt_out[], const
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> odp_comp_compress_param_t param[], int num_pkt);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> which takes of handling multiple packets and
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> retain current version for single packet
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> handling.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Shally
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> @bala-manoharan
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Results for both sync and async through result
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> function:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> int odp_comp_result(odp_comp_packet_result_t
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> *result, odp_packet_t packet);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> OR have the results as output parameter.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Results function is needed any way for the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> async operation.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Shally Verma(1234sv) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> int odp_comp_compress(const odp_packet_t
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> pkt_in[], odp_packet_t pkt_out[], const
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> odp_comp_compress_param_t param[], int
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> num_pkt);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Single packet version may be also added, but
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> is not necessary since couple of additional
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> if-clauses does not significant overhead when
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> compare CPU cycles of the entire comp
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> operation.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> int odp_comp_compress_single(odp_packet_t
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> pkt_in, odp_packet_t pkt_out, const
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> odp_comp_compress_param_t param);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> The two prototypes would be the same except []
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> and num_pkt.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
https://github.com/Linaro/odp/pull/156#discussion_r137716448
updated_at 2017-09-08 06:22:42
