Re: [dpdk-dev] [PATCH v2] doc/compress: clarify error handling on data-plane

[Shally Verma <shallyv@marvell.com>]

HI Fiona

> -----Original Message-----
> From: Fiona Trahe <fiona.trahe@intel.com>
> Sent: Tuesday, April 9, 2019 8:26 PM
> To: dev@dpdk.org
> Cc: akhil.goyal@nxp.com; Ashish Gupta <ashishg@marvell.com>;
> lee.daly@intel.com; Sunila Sahu <ssahu@marvell.com>; Shally Verma
> <shallyv@marvell.com>; Fiona Trahe <fiona.trahe@intel.com>;
> stable@dpdk.org
> Subject: [PATCH v2] doc/compress: clarify error handling on data-plane
> 
> Fixed some typos and clarified which errors should be returned when and
> why on the enqueue and dequeue APIs.
> 
> Fixes: a584d3bea902 ("doc: add compressdev library guide")
> cc: stable@dpdk.org
> 
> Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> ---
> v2 changes:
>  - changed "0 or undefined" to just "undefined" as 0 is superfluous.
> 
> 
>  doc/guides/prog_guide/compressdev.rst | 46
> ++++++++++++++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/compressdev.rst
> b/doc/guides/prog_guide/compressdev.rst
> index ad9703753..c700dd103 100644
> --- a/doc/guides/prog_guide/compressdev.rst
> +++ b/doc/guides/prog_guide/compressdev.rst
> @@ -201,7 +201,7 @@ for stateful processing of ops.
>  Operation Status
>  ~~~~~~~~~~~~~~~~
>  Each operation carries a status information updated by PMD after it is
> processed.
> -following are currently supported status:
> +Following are currently supported:
> 
>  - RTE_COMP_OP_STATUS_SUCCESS,
>      Operation is successfully completed @@ -227,14 +227,54 @@ following are
> currently supported status:
>      is not an error case. Output data up to op.produced can be used and
>      next op in the stream should continue on from op.consumed+1.
> 
> +Operation status after enqueue / dequeue
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +Some of the above values will only arise in the op after an
> +``rte_compressdev_enqueue_burst()``, some only after an
> +``rte_compressdev_dequeue_burst()``. 

[Shally] I think this could be simply stated as "some of the above values may arise in the op after ``rte_compressdev_enqueue_burst()``" because except STATUS_NOT_PROCESSED, I don't see any other error code that cannot be returned from enqueue. 
Like STATUS_ERROR, can be returned by both enqueue() and dequeue(). 

>For optimal performance on the
> +data-plane an application is not expected to check the ``op.status`` of
> +all ops after both enqueue and dequeue, it should be sufficient to only
> +check after dequeue.

[Shally] Ack

> To facilitate this optimisation, most errors which
> +may reasonably be expected to occur in a production environment will be
> returned by the PMD on the ``dequeue``.

[Shally] This statement should be moved after the next lines with bit of rephrasing

> +op.status may hold the following values after dequeue:
> +
> +- RTE_COMP_OP_STATUS_SUCCESS
> +- RTE_COMP_OP_STATUS_ERROR
> +- RTE_COMP_OP_STATUS_OUT_OF_SPACE_TERMINATED
> +- RTE_COMP_OP_STATUS_OUT_OF_SPACE_RECOVERABLE
> +
> +There are some exceptions whereby errors can occur on the ``enqueue``.

[Shally] this can be stated as  "To facilitate optimization in applications running in production environment, 
PMD should update and return op.status on dequeue unless in exceptional scenarios whereby error can occur on "enqueue"

> +For any error which can occur in a production environment and can be
> +successful after a retry with the same op the PMD may return the error
> +on the enqueue. 

[Shally] This statement looks redundant. If an op can be successful with retry, then no need for PMD to set an error on op. it can be left as OP_STATUS_NOT_PROCESSED.
Also, In normal scenario, if nb_enqd < actual passed, then app naturally attempt re-enqueue of an op.


So if less than the full burst is enqueued there's no
> +need for the application to check op.status - the application can
> +simply retry in a later enqueue and expect success. 
>Though the application
> is not expected to check for these, the values are as follows:

[Shally] these two statements can be combined as "if less than full burst is enqueued, then app shouldn't need to check for any error and simply should retry. However, if needed 
Then app can check for following errors:"

> +
> +- RTE_COMP_OP_STATUS_NOT_PROCESSED  - could occur if a hardware
> device's queue is full, after a dequeue a retry of the enqueue can be
> successful.
> +
> +- RTE_COMP_OP_STATUS_ERROR - could occur due to out-of-memory or
> other transient condition which could clear after a time.
> +
> +Other errors may also occur on an ``enqueue``, but they are only
> +expected to arise during development.
>As a retry with the same op won't
> +be successful, 

[Shally] Rephrasing could be "Or, any other irrecoverable error , example INVALID_ARGS or STATUS_ERROR.
In such case, the same op wont be successful"

>if a performant application wants to avoid checking
> +op.status on the enqueue it should ensure these never arise in a
> +production environment, e.g. by checking device capabilities and validating
> input parameters before sending operations. Examples are:
> +
> +- RTE_COMP_OP_STATUS_INVALID_ARGS
> +- RTE_COMP_OP_STATUS_ERROR (if due to a condition which is not
> +transient)
> +- RTE_COMP_OP_STATUS_INVALID_STATE
> +
> +If an application doesn't safeguard against these AND doesn't check the
> +op.status of the next op which was not enqueued, but just retries, it could
> result in an infinite loop.
> +
Ack. 

Thanks
Shally

>  Produced, Consumed And Operation Status
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
>  - If status is RTE_COMP_OP_STATUS_SUCCESS,
>      consumed = amount of data read from input buffer, and
>      produced = amount of data written in destination buffer
> -- If status is RTE_COMP_OP_STATUS_FAILURE,
> -    consumed = produced = 0 or undefined
> +- If status is RTE_COMP_OP_STATUS_ERROR,
> +    consumed = produced = undefined
>  - If status is RTE_COMP_OP_STATUS_OUT_OF_SPACE_TERMINATED,
>      consumed = 0 and
>      produced = usually 0, but in decompression cases a PMD may return > 0
> --
> 2.13.6