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

[Ahmed Mansour]

On 2/28/2018 1:39 PM, Trahe, Fiona wrote:
> Hi Ahmed, Shally,
>
> So just to capture what we concluded in the call today:
>
>  - There's no requirement for a device-agnostic session to facilitate load-balancing.
>  - For stateful data a stream is compulsory. Xform is passed to stream on creation. 
>     So no need for a session in stateful op.
>
> Re session data for stateless ops:
>  - All PMDs could cope with just passing in a xform with a stateless op. But it might 
>    not be performant. 
>  - Some PMDs need to allocate some resources which can only be used by one op
>    at a time. For stateful ops these resources can be attached to a stream. For stateless
>    they could allocate the resources on each op enqueue, but it would be better if
>    the resources were setup once based on the xform and could be re-used on ops,
>    though only by one op at a time. 
>  - Some PMDs don't need to allocate such resources, but could benefit by
>    setting up some pmd data based on the xform. This data would not be 
>    constrained, could be used in parallel by any op or qp of the device. 
>  - The name pmd_stateless_data was not popular, maybe something like 
>     xform_private_data can be used. On creation of this data, the PMD can return 
>     an indication of whether it should be used by one op at a time or shared.  
>   
> So I'll 
>  - remove the session completely from the API.
>  - add an initialiser API for the data to be attached to stateless ops
>  - add a union to the op:
>
>  union {
>         void *pmd_private_xform;
>         /**< Stateless private PMD data derived from an rte_comp_xform
>          * rte_comp_xform_init() must be called on a device 
>          * before sending any STATELESS operations. The PMD returns a handle
>          * which must be attached to subsequent STATELESS operations.
>          * The PMD also returns a flag, if this is COMP_PRIVATE_XFORM_SHAREABLE
>          * then the xform can be attached to multiple ops at the same time, 
>          * if it's COMP_PRIVATE_XFORM_SINGLE_OP then it can only be
>          * be used on one op at a time, other private xforms must be initialised
>          * to send other ops in parallel. 
>          */
>         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.
>          */
>     }
>
> Previous startup flow before sending a stateful op: 
> rte_comp_get_private_size(devid)
> rte_comp_mempool_create() - returns sess_pool
> rte_comp_session_create(sess_pool)
> rte_comp_session_init(devid, sess, sess_pool, xform)
> rte_comp_stream_create(devid, sess, **stream, op_type)
>
> simplified to:
> rte_comp_xform_init(devid, xform, **priv_xform, *flag) - returns handle and flag 
> (pool is within the PMD)
>
> Note, I don't think we bottomed out on removing the xform from the union, but I don't
> think we need it with above solution. 
>
> Other discussion:
>  - we should document on API that qp is not thread-safe, so enqueue
>    and dequeue should be performed by same thread.
[Ahmed] - I understand a qp should represent a single software user.
This is good because we will not have to add locking as you mentioned,
but are you sure that dequeues cannot be performed by another thread
without adding significant overhead? it would enable producer consumer
applications, and the name queue pair implies some independence.
- Another question. I want to be sure. A qp can be used to send both
compress and decompress ops.
- Nitpick: The name queue pair implies order preservation to the user.
Maybe we should change it to something that does not imply that.
>
> device and qp flow:
>  - dev_info_get() - application reads device capabilities, including the max qps the device can support.
>  - dev_config() - application specifies how many qps it intends to use - typically one per thread, must be < device max
>  - qp_setup() - called per qp. Creates the qp based on the size indicated by max_inflights
>  - dev_start() - once started device can't be reconfigured, must call dev_stop to reconfigure.
>
>
> Regards,
> Fiona
>
>> -----Original Message-----
>> From: Verma, Shally [mailto:Shally.Verma at cavium.com]
>> Sent: Tuesday, February 27, 2018 5:54 AM
>> To: Ahmed Mansour <ahmed.mansour at nxp.com>; Trahe, Fiona <fiona.trahe at intel.com>;
>> dev at dpdk.org
>> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>; Athreya, Narayana Prasad
>> <NarayanaPrasad.Athreya at cavium.com>; Gupta, Ashish <Ashish.Gupta at cavium.com>; Sahu, Sunila
>> <Sunila.Sahu at cavium.com>; Challa, Mahipal <Mahipal.Challa at cavium.com>; Jain, Deepak K
>> <deepak.k.jain at intel.com>; Hemant Agrawal <hemant.agrawal at nxp.com>; Roy Pledge
>> <roy.pledge at nxp.com>; Youri Querry <youri.querry_1 at nxp.com>
>> Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API
>>
>>
>>
>>> -----Original Message-----
>>> From: Ahmed Mansour [mailto:ahmed.mansour at nxp.com]
>>> Sent: 27 February 2018 03:05
>>> To: Verma, Shally <Shally.Verma at cavium.com>; Trahe, Fiona <fiona.trahe at intel.com>; dev at dpdk.org
>>> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>; Athreya, Narayana Prasad
>> <NarayanaPrasad.Athreya at cavium.com>;
>>> Gupta, Ashish <Ashish.Gupta at cavium.com>; Sahu, Sunila <Sunila.Sahu at cavium.com>; Challa,
>> Mahipal
>>> <Mahipal.Challa at cavium.com>; Jain, Deepak K <deepak.k.jain at intel.com>; Hemant Agrawal
>> <hemant.agrawal at nxp.com>; Roy
>>> Pledge <roy.pledge at nxp.com>; Youri Querry <youri.querry_1 at 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.
>> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdev.dpdk.narkive.com%2FCHS5l01B%2Fdpdk-dev-rfc-v1-doc-compression-api-for-dpdk%23post3&data=02%7C01%7Cahmed.mansour%40nxp.com%7C4299d16d58144e417f5208d57eda9ba9%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636554399768115871&sdata=MHWrD0qD%2FMKL%2FjX4j1kOdDeElh0cG1gQj9d1862K4V0%3D&reserved=0
>> 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
>>>>>>
>>>>>>
>>>>>>
>