Hi Fiona >-----Original Message----- >From: Trahe, Fiona [mailto:fiona.tr...@intel.com] >Sent: 01 March 2018 00:09 >To: Verma, Shally <shally.ve...@cavium.com>; Ahmed Mansour ><ahmed.mans...@nxp.com>; dev@dpdk.org >Cc: De Lara Guarch, Pablo <pablo.de.lara.gua...@intel.com>; Athreya, Narayana >Prasad <narayanaprasad.athr...@cavium.com>; >Gupta, Ashish <ashish.gu...@cavium.com>; Sahu, Sunila ><sunila.s...@cavium.com>; Challa, Mahipal ><mahipal.cha...@cavium.com>; Jain, Deepak K <deepak.k.j...@intel.com>; Hemant >Agrawal <hemant.agra...@nxp.com>; Roy >Pledge <roy.ple...@nxp.com>; Youri Querry <youri.querr...@nxp.com>; Trahe, >Fiona <fiona.tr...@intel.com> >Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API > >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.
[Shally] This looks better to me. So it mean app would always call xform_init() for stateless and attach an updated priv_xform to ops (depending upon if there's shareable or not). So it does not need to have NULL pointer on priv_xform. right? > >Other discussion: > - we should document on API that qp is not thread-safe, so enqueue > and dequeue should be performed by same thread. > >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.ve...@cavium.com] >> Sent: Tuesday, February 27, 2018 5:54 AM >> To: Ahmed Mansour <ahmed.mans...@nxp.com>; Trahe, Fiona >> <fiona.tr...@intel.com>; >> dev@dpdk.org >> Cc: De Lara Guarch, Pablo <pablo.de.lara.gua...@intel.com>; Athreya, >> Narayana Prasad >> <narayanaprasad.athr...@cavium.com>; Gupta, Ashish >> <ashish.gu...@cavium.com>; Sahu, Sunila >> <sunila.s...@cavium.com>; Challa, Mahipal <mahipal.cha...@cavium.com>; Jain, >> Deepak K >> <deepak.k.j...@intel.com>; Hemant Agrawal <hemant.agra...@nxp.com>; Roy >> Pledge >> <roy.ple...@nxp.com>; Youri Querry <youri.querr...@nxp.com> >> Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API >> >> >> >> >-----Original Message----- >> >From: Ahmed Mansour [mailto:ahmed.mans...@nxp.com] >> >Sent: 27 February 2018 03:05 >> >To: Verma, Shally <shally.ve...@cavium.com>; Trahe, Fiona >> ><fiona.tr...@intel.com>; dev@dpdk.org >> >Cc: De Lara Guarch, Pablo <pablo.de.lara.gua...@intel.com>; Athreya, >> >Narayana Prasad >> <narayanaprasad.athr...@cavium.com>; >> >Gupta, Ashish <ashish.gu...@cavium.com>; Sahu, Sunila >> ><sunila.s...@cavium.com>; Challa, >> Mahipal >> ><mahipal.cha...@cavium.com>; Jain, Deepak K <deepak.k.j...@intel.com>; >> >Hemant Agrawal >> <hemant.agra...@nxp.com>; Roy >> >Pledge <roy.ple...@nxp.com>; Youri Querry <youri.querr...@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 >> >>>> >> >>>> >> >>>> >> >>
