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

["Verma, Shally" <Shally.Verma@cavium.com>]

>-----Original Message-----
>From: Ahmed Mansour [mailto:ahmed.mansour@nxp.com]
>Sent: 27 February 2018 03:05
>To: Verma, Shally <Shally.Verma@cavium.com>; Trahe, Fiona <fiona.trahe@intel.com>; dev@dpdk.org
>Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Athreya, Narayana Prasad <NarayanaPrasad.Athreya@cavium.com>;
>Gupta, Ashish <Ashish.Gupta@cavium.com>; Sahu, Sunila <Sunila.Sahu@cavium.com>; Challa, Mahipal
><Mahipal.Challa@cavium.com>; Jain, Deepak K <deepak.k.jain@intel.com>; Hemant Agrawal <hemant.agrawal@nxp.com>; Roy
>Pledge <roy.pledge@nxp.com>; Youri Querry <youri.querry_1@nxp.com>
>Subject: Re: [dpdk-dev] [PATCH] compressdev: implement API
>
>> Hi Fiona, Ahmed
>>> 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.
>> [Shally]  As per my knowledge, session came with idea in DPDK, if system has multiple devices setup to do similar jobs then
>application can fan out ops to any of them for load-balancing. Though it is not possible for stateful ops but it still can work for stateless.
>If there's an application which only have stateless ops to process then I see this is still useful feature to support.
>[Ahmed] Is there an advantage to exposing load balancing to the user? I
>do not see load balancing as a feature within itself. Can the PMD take
>care of this? I guess a system that has

[Shally] I assume idea was to leverage multiple PMDs that are available in system (say QAT+SW ZLIB) and I believe matter of load-balancing came out of one of the earlier discussion with Fiona on RFC v1. http://dev.dpdk.narkive.com/CHS5l01B/dpdk-dev-rfc-v1-doc-compression-api-for-dpdk#post3
So, I wait for her comments on this. But in any case, with changed notion too it looks achievable to me, if so is desired.

>> In current proposal, stream logically represent data and hold its specific information and session is generic information that can be
>applied on multiple data. If we want to combine stream and session. Then one way to look at this is:
>>
>> "let application only allocate and initialize session with rte_comp_xform (and possibly op type) information so that PMD can do one-
>time setup and allocate enough resources. Once attached to op, cannot be reused until that op is fully processed. So, if app has 16
>data elements to process in a burst, it will setup 16 sessions."
>[Ahmed] Why not allow multiple inflight stateless ops with the same
>session? Stateless by definition guarantees that the resources used to
>work on one up will be free after the op is processed. That means that
>even if an op fails to process correctly on a session, it will have no
>effect on the next op since there is not interdependence. This assumes
>that the resources are shareable between hardware instances for
>stateless. That is not a bad assumption since hardware should not need
>more than the data of the op itself to work on a statelss op.

[Shally]  multiple ops in-flight can connect to same session but I assume you agree then they cannot execute in parallel i.e. only one op at-a-time can use session here? And as far as I understand your PMD works this way. Your HW execute one op at-a-time from queue?!

>> This is same as what Ahmed suggested. For a particular load-balancing case suggested above, If application want, can initialize
>different sessions on multiple devices with same xform so that each is prepared to process ops.  Application can then fanout stateless
>ops to multiple devices for load-balancing but then it would need to keep map of device & a session map.
>>
>> If this sound feasible, then I too believe we can rather get rid of either and keep one (possibly session but am open with stream as
>well).
>> However, regardless of case whether we live with name stream or session, I don't see much deviation from current API spec except
>description and few modifications/additions as identified.
>> So, then I see it as:
>>
>> - A stream(or session whichever name is chosen) can be used with only one-op at-a-time
>> - It can be re-used when previously attached op is processed
>> -  if it is stream then currently it is allocated from PMD managed pool whereas Sessions are allocated from application created
>mempool.
>>    In either of case, I would expect to review pool management API
>>
>> With this in mind, below are few of my comments
>>
>>>> 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
>>>>          */
>> [Shally] It sounds like stateless_data_init() nothing more than a replacement of session_init().
>> 	So, this is needed neither if we retain session concept nor if we retain stream concept ( rte_comp_stream_create() with
>op_type: stateless can serve same purpose).
>> 	It should be sufficient to provide either stream (or session) pointer.
>>
>>>>         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.
>> [Shally] my understanding is xform will be mapped by PMD to its internally managed stream(or session data structure). And then we
>can remove STATEFUL reference here and just say stream(or session) it belongs to. However, This condition still apply:
>>        *All operations that belong to same stream must be sent to the same device.*
>>
>>>> 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
>>>>
>>>>
>>>>
>>