From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 540C14C71 for ; Wed, 28 Feb 2018 19:39:29 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Feb 2018 10:39:27 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,406,1515484800"; d="scan'208";a="21018519" Received: from irsmsx109.ger.corp.intel.com ([163.33.3.23]) by orsmga007.jf.intel.com with ESMTP; 28 Feb 2018 10:39:25 -0800 Received: from irsmsx111.ger.corp.intel.com (10.108.20.4) by IRSMSX109.ger.corp.intel.com (163.33.3.23) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 28 Feb 2018 18:39:24 +0000 Received: from irsmsx101.ger.corp.intel.com ([169.254.1.188]) by irsmsx111.ger.corp.intel.com ([169.254.2.246]) with mapi id 14.03.0319.002; Wed, 28 Feb 2018 18:39:24 +0000 From: "Trahe, Fiona" To: "Verma, Shally" , Ahmed Mansour , "dev@dpdk.org" CC: "De Lara Guarch, Pablo" , "Athreya, Narayana Prasad" , "Gupta, Ashish" , "Sahu, Sunila" , "Challa, Mahipal" , "Jain, Deepak K" , Hemant Agrawal , "Roy Pledge" , Youri Querry , "Trahe, Fiona" Thread-Topic: [dpdk-dev] [PATCH] compressdev: implement API Thread-Index: AQHTnFNA34V0d0oQkkqZkaaruzarZKO35fiAgAJP7eA= Date: Wed, 28 Feb 2018 18:39:23 +0000 Message-ID: <348A99DA5F5B7549AA880327E580B4358932983C@IRSMSX101.ger.corp.intel.com> References: <1517595924-25963-1-git-send-email-fiona.trahe@intel.com> <12544144.czVLKRyaz4@xps> <348A99DA5F5B7549AA880327E580B43589325187@IRSMSX101.ger.corp.intel.com> In-Reply-To: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNmE5MTg2MjItMDBjZS00NTJjLWJkNGYtODhlZTM0MTFkN2M3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6InpXbGVib045dUdqTzVhUmFibFpJOWttQjBKRTV6TVV6UjFUdTZzZDUzSlk9In0= x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] compressdev: implement API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Feb 2018 18:39:31 -0000 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 c= reation.=20 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. Bu= t it might=20 not be performant.=20 - 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 be= tter 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.=20 - 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=20 constrained, could be used in parallel by any op or qp of the device.=20 - The name pmd_stateless_data was not popular, maybe something like=20 xform_private_data can be used. On creation of this data, the PMD can r= eturn=20 an indication of whether it should be used by one op at a time or share= d. =20 =20 So I'll=20 - 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=20 * before sending any STATELESS operations. The PMD returns a handl= e * which must be attached to subsequent STATELESS operations. * The PMD also returns a flag, if this is COMP_PRIVATE_XFORM_SHARE= ABLE * then the xform can be attached to multiple ops at the same time,= =20 * 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 initia= lised * to send other ops in parallel.=20 */ 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 STAT= EFUL=20 * 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:=20 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=20 (pool is within the PMD) Note, I don't think we bottomed out on removing the xform from the union, b= ut I don't think we need it with above solution.=20 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 ma= x qps the device can support. - dev_config() - application specifies how many qps it intends to use - ty= pically one per thread, must be < device max - qp_setup() - called per qp. Creates the qp based on the size indicated b= y max_inflights - dev_start() - once started device can't be reconfigured, must call dev_s= top to reconfigure. Regards, Fiona > -----Original Message----- > From: Verma, Shally [mailto:Shally.Verma@cavium.com] > Sent: Tuesday, February 27, 2018 5:54 AM > To: Ahmed Mansour ; Trahe, Fiona ; > dev@dpdk.org > Cc: De Lara Guarch, Pablo ; Athreya, Nara= yana Prasad > ; Gupta, Ashish ; Sahu, Sunila > ; Challa, Mahipal ; Ja= in, Deepak K > ; Hemant Agrawal ; Roy P= ledge > ; Youri Querry > Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API >=20 >=20 >=20 > >-----Original Message----- > >From: Ahmed Mansour [mailto:ahmed.mansour@nxp.com] > >Sent: 27 February 2018 03:05 > >To: Verma, Shally ; Trahe, Fiona ; dev@dpdk.org > >Cc: De Lara Guarch, Pablo ; Athreya, Nar= ayana Prasad > ; > >Gupta, Ashish ; Sahu, Sunila ; Challa, > Mahipal > >; Jain, Deepak K ; H= emant Agrawal > ; Roy > >Pledge ; Youri Querry > >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 c= all > >>> 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 strea= m 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 unli= kely 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 sess= ion > >>> holds many properties like the op-type, instead of having this > >>> information in the op itself. This way we lower the per op setup cos= t. > >>> This also allows rapid reuse of stateful infrastructure, once a strea= m > >>> 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 n= o 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 syst= em 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 >=20 > [Shally] I assume idea was to leverage multiple PMDs that are available i= n system (say QAT+SW ZLIB) > and I believe matter of load-balancing came out of one of the earlier dis= cussion 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. >=20 > >> In current proposal, stream logically represent data and hold its spec= ific 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_xf= orm (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. >=20 > [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?! >=20 > >> 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 p= repared to process ops. > Application can then fanout stateless > >ops to multiple devices for load-balancing but then it would need to kee= p map of device & a session > map. > >> > >> If this sound feasible, then I too believe we can rather get rid of ei= ther and keep one (possibly session > but am open with stream as > >well). > >> However, regardless of case whether we live with name stream or sessio= n, 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 poo= l 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_xfo= rm > >>>> * rte_comp_stateless_data_init() must be called on a device > >>>> * before sending any STATELESS operations. If the PMD retur= ns a non-NULL > >>>> * value the handle must be attached to subsequent STATELESS= operations. > >>>> * If a PMD returns NULL, then the xform should be passed di= rectly to each op > >>>> */ > >> [Shally] It sounds like stateless_data_init() nothing more than a repl= acement of session_init(). > >> So, this is needed neither if we retain session concept nor if we ret= ain 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 a= ll STATEFUL > >>>> * data streams and the resulting stream attached > >>>> * to the one or more operations associated with the data st= ream. > >>>> * All operations in a stream must be sent to the same devic= e. > >>>> */ > >>>> } > >>> [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 intern= ally managed stream(or > session data structure). And then we > >can remove STATEFUL reference here and just say stream(or session) it be= longs 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 fo= r both it can do, > >>>> just on the API I think it's better if they're named differentl= y 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 di= d, 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 e= ssence > >>>> of these data structs better than our current API does. > >>>> > >>>> We would put some more helper fns and structure around the above cod= e if people > >>>> are in agreement, just want to see if the concept flies before going= further? > >>>> > >>>> Fiona > >>>> > >>>> > >>>> > >>