From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 25CAAA328D for ; Wed, 23 Oct 2019 00:21:08 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2A7131BF2E; Wed, 23 Oct 2019 00:21:07 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 5F8311BF22 for ; Wed, 23 Oct 2019 00:21:04 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Oct 2019 15:21:03 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.68,217,1569308400"; d="scan'208";a="188053652" Received: from irsmsx152.ger.corp.intel.com ([163.33.192.66]) by orsmga007.jf.intel.com with ESMTP; 22 Oct 2019 15:21:01 -0700 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.252]) by IRSMSX152.ger.corp.intel.com ([169.254.6.76]) with mapi id 14.03.0439.000; Tue, 22 Oct 2019 23:21:01 +0100 From: "Ananyev, Konstantin" To: 'Akhil Goyal' , "'dev@dpdk.org'" , "De Lara Guarch, Pablo" , 'Thomas Monjalon' , "Zhang, Roy Fan" , "Doherty, Declan" CC: 'Anoob Joseph' , 'Hemant Agrawal' Thread-Topic: [RFC PATCH 1/9] security: introduce CPU Crypto action type and API Thread-Index: AQHVYm4Y+AedzaNgY0qWMAmu5GwNVqcbQpEAgAArCICAAuADAIAARA4AgAYiKoCAAbQS0IABqquAgAZiqNCAAPA4gIABtviQgAu3KZCAAoImAIAExwxQgAA3zQCAAZ1E0IADFFEAgAZGHRCAAsIfgIAAUayAgAM4hICAB/TasIADCaEAgASiaLCAAasPgIAAIn3wgAB/6PA= Date: Tue, 22 Oct 2019 22:21:00 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725801A8C6E29D@IRSMSX104.ger.corp.intel.com> References: <20190903154046.55992-1-roy.fan.zhang@intel.com> <20190903154046.55992-2-roy.fan.zhang@intel.com> <9F7182E3F746AB4EA17801C148F3C6043369D686@IRSMSX101.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580191926A17@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580191962CD5@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580191966116@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580191966C23@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258019196A767@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258019196D53D@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258019196F386@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258019197206C@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258019197446B@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725801A8C69C5E@IRSMSX104.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725801A8C6D6AC@IRSMSX104.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725801A8C6E152@IRSMSX104.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB97725801A8C6E152@IRSMSX104.ger.corp.intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOGUyNTY1NjQtMWQ5Mi00ZmQ1LWE0NDUtMDQ0NGVhYjc0ZmNkIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoia09Jc01mQzBySCt6c21VS2RyV3k0VHFES2NHRFwvRDVzMG5nZlRjbkZnWTYxbHVnZ2pjSjNsaXNGVUFuaktFa1IifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [RFC PATCH 1/9] security: introduce CPU Crypto action type and 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > > > > Added my comments inline with your draft. > > > > [snip].. > > > > > > > > > > > > > > Ok, then my suggestion: > > > > > Let's at least write down all points about crypto-dev approach wh= ere we > > > > > disagree and then probably try to resolve them one by one.... > > > > > If we fail to make an agreement/progress in next week or so, > > > > > (and no more reviews from the community) > > > > > will have bring that subject to TB meeting to decide. > > > > > Sounds fair to you? > > > > Agreed > > > > > > > > > > List is below. > > > > > Please add/correct me, if I missed something. > > > > > > > > > > Konstantin > > > > > > > > Before going into comparison, we should define the requirement as w= ell. > > > > > > Good point. > > > > > > > What I understood from the patchset, > > > > "You need a synchronous API to perform crypto operations on raw dat= a using > > > SW PMDs" > > > > So, > > > > - no crypto-ops, > > > > - no separate enq-deq, only single process API for data path > > > > - Do not need any value addition to the session parameters. > > > > (You would need some parameters from the crypto-op which > > > > Are constant per session and since you wont use crypto-op, > > > > You need some place to store that) > > > > > > Yes, this is correct, I think. > > > > > > > > > > > Now as per your mail, the comparison > > > > 1. extra input parameters to create/init rte_(cpu)_sym_session. > > > > > > > > Will leverage existing 6B gap inside rte_crypto_*_xform between 'al= go' and > > > 'key' fields. > > > > New fields will be optional and would be used by PMD only when cpu-= crypto > > > session is requested. > > > > For lksd-crypto session PMD is free to ignore these fields. > > > > No ABI breakage is required. > > > > > > > > [Akhil] Agreed, no issues. > > > > > > > > 2. cpu-crypto create/init. > > > > a) Our suggestion - introduce new API for that: > > > > - rte_crypto_cpu_sym_init() that would init completely opaq= ue > > > rte_crypto_cpu_sym_session. > > > > - struct rte_crypto_cpu_sym_session_ops {(*process)(...); (= *clear); > > > /*whatever else we'll need *'}; > > > > - rte_crypto_cpu_sym_get_ops(const struct rte_crypto_sym_xf= orm > > > *xforms) > > > > that would return const struct rte_crypto_cpu_sym_session= _ops *based > > > on input xforms. > > > > Advantages: > > > > 1) totally opaque data structure (no ABI breakages in future), PM= D > > > writer is totally free > > > > with it format and contents. > > > > > > > > [Akhil] It will have breakage at some point till we don't hit the u= nion size. > > > > > > Not sure, what union you are talking about? > > > > Union of xforms in rte_security_session_conf >=20 > Hmm, how does it relates here? > I thought we discussing pure rte_cryptodev_sym_session, no? >=20 > > > > > > > > > Rather I don't suspect there will be more parameters added. > > > > Or do we really care about the ABI breakage when the argument is ab= out > > > > the correct place to add a piece of code or do we really agree to a= dd code > > > > anywhere just to avoid that breakage. > > > > > > I am talking about maintaining it in future. > > > if your struct is not seen externally, no chances to introduce ABI br= eakage. > > > > > > > > > > > 2) each session entity is self-contained, user doesn't need to bri= ng along > > > dev_id etc. > > > > dev_id is needed only at init stage, after that user will use= session ops > > > to perform > > > > all operations on that session (process(), clear(), etc.). > > > > > > > > [Akhil] There is nothing called as session ops in current DPDK. > > > > > > True, but it doesn't mean we can't/shouldn't have it. > > > > We can have it if it is not adding complexity for the user. Creating 2 = different code > > Paths for user is not desirable for the stack developers. > > > > > > > > > What you are proposing > > > > is a new concept which doesn't have any extra benefit, rather it is= adding > > > complexity > > > > to have two different code paths for session create. > > > > > > > > > > > > 3) User can decide does he wants to store ops[] pointer on a per s= ession > > > basis, > > > > or on a per group of same sessions, or... > > > > > > > > [Akhil] Will the user really care which process API should be calle= d from the > > > PMD. > > > > Rather it should be driver's responsibility to store that in the se= ssion private > > > data > > > > which would be opaque to the user. As per my suggestion same proces= s > > > function can > > > > be added to multiple sessions or a single session can be managed in= side the > > > PMD. > > > > > > In that case we either need to have a function per session (stored in= ternally), > > > or make decision (branches) at run-time. > > > But as I said in other mail - I am ok to add small shim structure her= e: > > > either rte_crypto_cpu_sym_session { void *ses; struct > > > rte_crypto_cpu_sym_session_ops ops; } > > > or rte_crypto_cpu_sym_session { void *ses; struct > > > rte_crypto_cpu_sym_session_ops *ops; } > > > And merge rte_crypto_cpu_sym_init() and rte_crypto_cpu_sym_get_ops() = into > > > one (init). > > > > Again that will be a separate API call from the user perspective which = is not good. > > > > > > > > > > > > > > > > > 4) No mandatory mempools for private sessions. User can allocate > > > memory for cpu-crypto > > > > session whenever he likes. > > > > > > > > [Akhil] you mean session private data? > > > > > > Yes. > > > > > > > You would need that memory anyways, user will be > > > > allocating that already. You do not need to manage that. > > > > > > What I am saying - right now user has no choice but to allocate it vi= a mempool. > > > Which is probably not the best options for all cases. > > > > > > > > > > > Disadvantages: > > > > 5) Extra changes in control path > > > > 6) User has to store session_ops pointer explicitly. > > > > > > > > [Akhil] More disadvantages: > > > > - All supporting PMDs will need to maintain TWO types of session fo= r the > > > > same crypto processing. Suppose a fix or a new feature(or algo) is = added, PMD > > > owner > > > > will need to add code in both the session create APIs. Hence more > > > maintenance and > > > > error prone. > > > > > > I think majority of code for both paths will be common, plus even we'= ll reuse > > > current sym_session_init() - > > > changes in PMD session_init() code will be unavoidable. > > > But yes, it will be new entry in devops, that PMD will have to suppor= t. > > > Ok to add it as 7) to the list. > > > > > > > - Stacks which will be using these new APIs also need to maintain t= wo > > > > code path for the same processing while doing session initializatio= n > > > > for sync and async > > > > > > That's the same as #5 above, I think. > > > > > > > > > > > > > > > b) Your suggestion - reuse existing rte_cryptodev_sym_session_= init() and > > > existing rte_cryptodev_sym_session > > > > structure. > > > > Advantages: > > > > 1) allows to reuse same struct and init/create/clear() functions. > > > > Probably less changes in control path. > > > > Disadvantages: > > > > 2) rte_cryptodev_sym_session. sess_data[] is indexed by driver_id, > > > which means that > > > > we can't use the same rte_cryptodev_sym_session to hold privat= e > > > sessions pointers > > > > for both sync and async mode for the same device. > > > > So the only option we have - make PMD devops- > > > >sym_session_configure() > > > > always create a session that can work in both cpu and lksd mod= es. > > > > For some implementations that would probably mean that under t= he > > > hood PMD would create > > > > 2 different session structs (sync/async) and then use one or a= nother > > > depending on from what API been called. > > > > Seems doable, but ...: > > > > - will contradict with statement from 1: > > > > " New fields will be optional and would be used by PMD only = when > > > cpu-crypto session is requested." > > > > Now it becomes mandatory for all apps to spec= ify cpu-crypto > > > related parameters too, > > > > even if they don't plan to use that mode - i.e. behavior ch= ange, > > > existing app change. > > > > - might cause extra space overhead. > > > > > > > > [Akhil] It will not contradict with #1, you will only have few chec= ks in the > > > session init PMD > > > > Which support this mode, find appropriate values and set the approp= riate > > > process() in it. > > > > User should be able to call, legacy enq-deq as well as the new proc= ess() > > > without any issue. > > > > User would be at runtime will be able to change the datapath. > > > > So this is not a disadvantage, it would be additional flexibility f= or the user. > > > > > > Ok, but that's what I am saying - if PMD would *always* have to creat= e a > > > session that can handle > > > both modes (sync/async), then user would *always* have to provide par= ameters > > > for both modes too. > > > Otherwise if let say user didn't setup sync specific parameters at al= l, what PMD > > > should do? > > > - return with error? > > > - init session that can be used with async path only? > > > My current assumption is #1. > > > If #2, then how user will be able to distinguish is that session vali= d for both > > > modes, or only for one? > > > > I would say a 3rd option, do nothing if sync params are not set. > > Probably have a debug print in the PMD(which support sync mode) to spec= ify that > > session is not configured properly for sync mode. >=20 > So, just print warning and proceed with init session that can be used wit= h async path only? > Then it sounds the same as #2 above. > Which actually means that sync mode parameters for sym_session_init() bec= omes optional. > Then we need an API to provide to the user information what modes > (sync+async/async only) is supported by that session for given dev_id. > And user would have to query/retain this information at control-path, > and store it somewhere in user-space together with session pointer and de= v_ids > to use later at data-path (same as we do now for session type). > That definitely requires changes in control-path to start using it. > Plus the fact that this value can differ for different dev_ids for the sa= me session - > doesn't make things easier here. >=20 > > Internally the PMD will not store the process() API in the session priv= data > > And while calling the first packet, devops->process will give an assert= that session > > Is not configured for sync mode. The session validation would be done i= n any case > > your suggestion or mine. So no extra overhead at runtime. >=20 > I believe that after session_init() user should get either an error or > valid session handler that he can use at runtime. > Pushing session validation to runtime doesn't seem like a good idea. >=20 > > > > > > > > > > > > > > > > > > > > 3) not possible to store device (not driver) specific data within = the > > > session, but I think it is not really needed right now. > > > > So probably minor compared to 2.b.2. > > > > > > > > [Akhil] So lets omit this for current discussion. And I hope we can= find some > > > way to deal with it. > > > > > > I don't think there is an easy way to fix that with existing API. > > > > > > > > > > > > > > > Actually #3 follows from #2, but decided to have them separated. > > > > > > > > 3. process() parameters/behavior > > > > a) Our suggestion: user stores ptr to session ops (or to (*proc= ess) itself) and > > > just does: > > > > session_ops->process(sess, ...); > > > > Advantages: > > > > 1) fastest possible execution path > > > > 2) no need to carry on dev_id for data-path > > > > > > > > [Akhil] I don't see any overhead of carrying dev id, at least it wo= uld be inline > > > with the > > > > current DPDK methodology. > > > > > > If we'll add process() into rte_cryptodev itself (same as we have > > > enqueue_burst/dequeue_burst), > > > then it will be an ABI breakage. > > > Also there are discussions to get rid of that approach completely: > > > http://mails.dpdk.org/archives/dev/2019-September/144674.html > > > So I am not sure this is a recommended way these days. > > > > We can either have it in rte_cryptodev or in rte_cryptodev_ops whicheve= r > > is good for you. > > > > Whether it is ABI breakage or not, as per your requirements, this is th= e correct > > approach. Do you agree with this or not? >=20 > I think it is possible approach, but not the best one: > it looks quite flakey to me (see all these uncertainty with sym_session_i= nit above), > plus introduces extra overhead at data-path. >=20 > > > > Now handling the API/ABI breakage is a separate story. In 19.11 release= we > > Are not much concerned about the ABI breakages, this was discussed in > > community. So adding a new dev_ops wouldn't have been an issue. > > Now since we are so close to RC1 deadline, we should come up with some > > other solution for next release. May be having a pmd API in 20.02 and > > converting it into formal one in 20.11 > > > > > > > > > > > What you are suggesting is a new way to get the things done without= much > > > benefit. > > > > > > Would help with ABI stability plus better performance, isn't it enoug= h? > > > > > > > Also I don't see any performance difference as crypto workload is h= eavier than > > > > Code cycles, so that wont matter. > > > > > > It depends. > > > Suppose function call costs you ~30 cycles. > > > If you have burst of big packets (let say crypto for each will take ~= 2K cycles) that > > > belong > > > to the same session, then yes you wouldn't notice these extra 30 cycl= es at all. > > > If you have burst of small packets (let say crypto for each will take= ~300 cycles) > > > each > > > belongs to different session, then it will cost you ~10% extra. > > > > Let us do some profiling on openssl with both the approaches and find o= ut the > > difference. > > > > > > > > > So IMO, there is no advantage in your suggestion as well. > > > > > > > > > > > > Disadvantages: > > > > 3) user has to carry on session_ops pointer explicitly > > > > b) Your suggestion: add (*cpu_process) inside rte_cryptodev_op= s and then: > > > > rte_crypto_cpu_sym_process(uint8_t dev_id, rte_cryptodev_sy= m_session > > > *sess, /*data parameters*/) {... > > > > rte_cryptodevs[dev_id].dev_ops->cpu_process(se= s, ...); > > > > /*and then inside PMD specifc process: */ > > > > pmd_private_session =3D sess->sess_data[this_p= md_driver_id].data; > > > > /* and then most likely either */ > > > > pmd_private_session->process(pmd_private_sessi= on, ...); > > > > /* or jump based on session/input data */ > > > > Advantages: > > > > 1) don't see any... > > > > Disadvantages: > > > > 2) User has to carry on dev_id inside data-path > > > > 3) Extra level of indirection (plus data dependency) - both for da= ta and > > > instructions. > > > > Possible slowdown compared to a) (not measured). > > > > > > > > Having said all this, if the disagreements cannot be resolved, you = can go for a > > > pmd API specific > > > > to your PMDs, > > > > > > I don't think it is good idea. > > > PMD specific API is sort of deprecated path, also there is no clean w= ay to use it > > > within the libraries. > > > > I know that this is a deprecated path, we can use it until we are not a= llowed > > to break ABI/API > > > > > > > > > because as per my understanding the solution doesn't look scalable = to other > > > PMDs. > > > > Your approach is aligned only to Intel , will not benefit others li= ke openssl > > > which is used by all > > > > vendors. > > > > > > I feel quite opposite, from my perspective majority of SW backed PMDs= will > > > benefit from it. > > > And I don't see anything Intel specific in my proposals above. > > > About openssl PMD: I am not an expert here, but looking at the code, = I think it > > > will fit really well. > > > Look yourself at its internal functions: > > > process_openssl_auth_op/process_openssl_crypto_op, > > > I think they doing exactly the same - they use sync API underneath, a= nd they are > > > session based > > > (AFAIK you don't need any device/queue data, everything that needed f= or > > > crypto/auth is stored inside session). Looked at drivers/crypto/armv8 - same story here, I believe. > > > > > By vendor specific, I mean, > > - no PMD would like to have 2 different variants of session Init APIs f= or doing the same stuff. > > - stacks will become vendor specific while using 2 separate session cre= ate APIs. No stack would > > Like to support 2 variants of session create- one for HW PMDs and one f= or SW PMDs. >=20 > I think what you refer on has nothing to do with 'vendor specific'. > I would name it 'extra overhead for PMD and stack writers'. > Yes, for sure there is extra overhead (as always with new API) - > for both producer (PMD writer) and consumer (stack writer): > New function(s) to support, probably more tests to create/run, etc. > Though this API is optional - if PMD/stack maintainer doesn't see > value in it, they are free not to support it. > From other side, re-using rte_cryptodev_sym_session_init() > wouldn't help anyway - both data-path and control-path would differ > from async mode anyway. > BTW, right now to support different HW flavors > we do have 4 different control and data-paths for both > ipsec-secgw and librte_ipsec: > lkds-none/lksd-proto/inline-crypto/inline-proto. > And that is considered to be ok. > Honestly, I don't understand why SW backed implementations > can't have their own path that would suite them most. > Konstantin >=20 >=20 >=20 >=20 >=20