Re: [dpdk-dev] [PATCH] compressdev: implement API

[Ahmed Mansour <ahmed.mansour@nxp.com>]

Hi Fiona,

Thanks for starting this discussion. In the current API the user must
make 12 API calls just to get information to compress. Maybe there is a
way to simplify. At least for some use cases (stateless). I think a call
sometime next week would be good to help clarify coalesce some of the
complexity.

I added specific comments inline.

Thanks,

Ahmed

On 2/21/2018 2:12 PM, Trahe, Fiona wrote:
> We've been struggling with the idea of session in compressdev.
>
> Is it really a session? 
>  - It's not in the same sense as cryptodev where it's used to hold a key, and maps to a Security Association.
>  - It's a set of immutable data that is needed with the op and stream to perform the operation.
>  - It inherited from cryptodev the ability to be set up for multiple driver types and used across any 
>     devices of those types. For stateful ops this facility can't be used. 
>     For stateless we don't think it's important, and think it's unlikely to be used.
>  - Drivers use it to prepare private data, set up resources, do pre-work, so there's
>     less work to be done on the data path. Initially we didn't have a stream, we do now,
>     this may be a better alternative place for that work.
> So we've been toying with the idea of getting rid of the session. 
[Ahmed] In our proprietary API the stream and session are one. A session
holds many properties like the op-type, instead of having this
information in the op itself.  This way we lower the per op setup cost.
This also allows rapid reuse of stateful infrastructure, once a stream
is closed on a stateful session, the next op (stream) on this session
reuses the stateful storage. Obviously if a stream is in "pause mode" on
a session, all following ops that may be unrelated to this
stream/session must also wait until this current stream is closed or
aborted before the infrastructure can be reused.
>
> We also struggle with the idea of setting up a stream for stateless ops.
>   - Well, really I just think the name is misleading, i.e. there's no problem with setting 
>     up some private PMD data to use with stateless operations, just calling it a
>     stream doesn't seem right.
[Ahmed] I agree. The op has all the necessary information to process it
in the current API? Both the stream and the op are one time use. We
can't attach multiple similar ops to a single stream/session and rely on
their properties to simplify op setup, so why the hassle.
>
> So putting above thoughts together I want to propose:
> -	Removal of the session and all associated APIs.
> -	Passing in one of three data types in the rte_comp_op
>     
>     union {
>         struct rte_comp_xform *xform;
>         /**< Immutable compress/decompress params */
>         void *pmd_stateless_data;
>         /**< Stateless private PMD data derived from an rte_comp_xform
>          * rte_comp_stateless_data_init() must be called on a device 
>          * before sending any STATELESS operations. If the PMD returns a non-NULL
>          * value the handle must be attached to subsequent STATELESS operations.
>          * If a PMD returns NULL, then the xform should be passed directly to each op 
>          */
>         void *stream;
>         /* Private PMD data derived initially from an rte_comp_xform, which holds state
>          * and history data and evolves as operations are processed.
>          * rte_comp_stream_create() must be called on a device for all STATEFUL 
>          * data streams and the resulting stream attached
>          * to the one or more operations associated with the data stream.
>          * All operations in a stream must be sent to the same device.
>          */
>     }
[Ahmed] I like this setup, but I am not sure in what cases the xform
immutable would be used. I understand the other two.
> Notes: 
> 1. Internally if a PMD wants to use the exact same data structure for both it can do, 
>      just on the API I think it's better if they're named differently with 
>      different comments.
> 2. I'm not clear of the constraints if any, which attach to the pmd_stateless_data
>      For our PMD it would only hold immutable data as the session did, and so
>      could be attached to many ops in parallel.
>      Is this true for all PMDs or are there constraints which should be called out?
>      Is it limited to a specific device, qp, or to be used on one op at a time?
> 3. Am open to other naming suggestions, just trying to capture the essence
>     of these data structs better than our current API does.
>
> We would put some more helper fns and structure around the above code if people
> are in agreement, just want to see if the concept flies before going further?
>
> Fiona
>  
>
>