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 4095DA04C1; Wed, 20 Nov 2019 15:27:28 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 056B92B87; Wed, 20 Nov 2019 15:27:28 +0100 (CET) Received: from mail-io1-f66.google.com (mail-io1-f66.google.com [209.85.166.66]) by dpdk.org (Postfix) with ESMTP id 13B7F27D; Wed, 20 Nov 2019 15:27:26 +0100 (CET) Received: by mail-io1-f66.google.com with SMTP id s3so27840966ioe.3; Wed, 20 Nov 2019 06:27:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=rHwnf7ilMLWF7Ww1kD3xTYJ8XXOq5s+n9GymCSF1y8o=; b=HIjUKSNOLZHedm5+sXrscPtWdAgjKgTDgd18gV9D8dWIuFscqhqt7J+ceo+Tk8i+AL 0wCWqwpM9/TX/qIbFtUw/eEqgd9s98bf/ako0gtb1QZPMVCFHWwah5FcDRozJi+dOoIO SV50+bLt+bYPckWdcwIkdrNRD6VTE4i34Am0usJoE0M0IbLoodBWF5+R5irCYDV24G17 1lzfZtr4jO5tPjKXiUZV+1PcTWulR7N4XxU+gM9SgsAv9eCW55K1T0gqG5yp6eKPT2sr dcBlAwRPQMbdOTjwHCcEy1/N8tQBI1KIgp1muFOVbSmfssgkUtfYqaUtVsPZ0ozPGe// VA+g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=rHwnf7ilMLWF7Ww1kD3xTYJ8XXOq5s+n9GymCSF1y8o=; b=aBLBFg5AHNuJbE/uEmFp8Tld87f+5sd7RHIK2aWDXjzeF1zjrfBB1vBE5oEhZuVlCY G96zaOgwor6u5vFbsUX72jY9/ijXWhmezOobpEKlIN0llM844Y78U69bPFma/wwYsk6W qscjtB6zR7ytm30M9GOO6LidujjYwM/fPBRzvLeIV2NNJuo1i26O2coLtXYlIarQ0T+O eP5I8CLJwuR9o1BtY8u8UUY5dIgCHdQXnK2hBbEja9Suvwyx/Z+HvxjHgTFDJG8Bms3L nBJ4R5CISzdifveXm2pjUtYoqaSrKB65yRhLkm5ITktgCHuPIhkN7gtyfQz1okJdo/ED rJEA== X-Gm-Message-State: APjAAAWN2jV5K1V8IWeXSKRsJt3TJGwX/6p+CTivWwIbinT+K+dpVxOe hivIDsRkLgUpl30OtnAX1NqoYtMy9uGe3sb6itE= X-Google-Smtp-Source: APXvYqyNyCQZknMu2wz5zq7sXMOi1oFZSxkiauSQcb+Tgsny+v7+DkMhRzXuoTC5O+O/ck+sZ0CZHgzEPqWicQ5kH3A= X-Received: by 2002:a05:6602:2429:: with SMTP id g9mr2525807iob.294.1574260045025; Wed, 20 Nov 2019 06:27:25 -0800 (PST) MIME-Version: 1.0 References: <20191105184122.15172-1-konstantin.ananyev@intel.com> In-Reply-To: From: Jerin Jacob Date: Wed, 20 Nov 2019 19:57:08 +0530 Message-ID: To: "Ananyev, Konstantin" Cc: dpdk-dev , "techboard@dpdk.org" , "Zhang, Roy Fan" , "Doherty, Declan" , Akhil Goyal Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [RFC 0/4] cpu-crypto API choices 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" On Mon, Nov 18, 2019 at 5:27 PM Ananyev, Konstantin wrote: > > Hi Jerin, Hi Konstantin, > > Thanks for input, my answers inline. > Other guys - please provide your input. > Thanks > Konstantin > > > > Originally both SW and HW crypto PMDs use rte_crypot_op based API to > > > process the crypto workload asynchronously. This way provides uniformity > > > to both PMD types, but also introduce unnecessary performance penalty to > > > SW PMDs that have to "simulate" HW async behavior > > > (crypto-ops enqueue/dequeue, HW addresses computations, > > > storing/dereferencing user provided data (mbuf) for each crypto-op, > > > etc). > > > > > > The aim is to introduce a new optional API for SW crypto-devices > > > to perform crypto processing in a synchronous manner. > > > As summarized by Akhil, we need a synchronous API to perform crypto > > > operations on raw data using SW PMDs, that provides: > > > - no crypto-ops. > > > - avoid using mbufs inside this API, use raw data buffers instead. > > > - no separate enqueue-dequeue, only single process() API for data path. > > > - input data buffers should be grouped by session, > > > i.e. each process() call takes one session and group of input buffers > > > that belong to that session. > > > - All parameters that are constant accross session, should be stored > > > inside the session itself and reused by all incoming data buffers. > > > > > > While there seems no controversy about need of such functionality, > > > there seems to be no agreement on what would be the best API for that. > > > So I am requesting for TB input on that matter. > > > > > > Series structure: > > > - patch #1 - intorduce basic data structures to be used by sync API > > > (no controversy here, I hope ..) > > > [RFC 1/4] cpu-crypto: Introduce basic data structures > > > - patch #2 - Intel initial approach for new API (via rte_security) > > > [RFC 2/4] security: introduce cpu-crypto API > > > - patch #3 - approach that reuses existing rte_cryptodev API as much as > > > possible > > > [RFC 3/4] cryptodev: introduce cpu-crypto API > > > - patch #4 - approach via introducing new session data structure and API > > > [RFC 4/4] cryptodev: introduce rte_crypto_cpu_sym_session API > > > > > > Patches 2,3,4 are mutually exclusive, > > > and we probably have to choose which one to go forward with. > > > I put some explanations in each of the patches, hopefully that will help > > > to understand pros and cons of each one. > > > > > > Akhil strongly supports #3, AFAIK mainly because it allows PMDs to > > > reuse existing API and minimize API level changes. > > > My favorite is #4, #2 is less preferable but ok too. > > > #3 seems problematic to me by the reasons I outlined in #4 patch > > > description. > > > > > > Please provide your opinion. > > > > I spend some time on the proposal and I agree that sync API is needed > > and it makes sense to remove queue emulation and allocating/freeing > > the crypto_ops > > in case of sync API. > > > > # I would prefer to not duplicate the session. If the newly added > > fields are for optimization > > then those can be applicable for HW too. For example, if we consider, > > offset to be > > constant for one session HW PMD will be able to leverage this. ref: > > rte_crypto_aead_xfrom::cpu_crypto:offset > > It might, but right for async API we pass this info in crypto_op instead. > So if I get you right your preference is sort of #3 approach > that reuses existing rte_cryptodev API as much as possible: > reuse existing rte_cryptodev_sym structure with new sync process() API? Yes. > > # I would prefer to not duplicate ops parameters, instead of the > > existing rte_crypto_ops can be updated. > > I see that most members introduced in rte_crypto_sym_vec & > > rte_crypto_vec are already existing in rte_crypto_op. > > rte_crypto_ops is way too generic/excessive. > Filling/reading it seems one of the main slowdowns that we trying to > avoid in new API. It does not look like it is going over 1 CL. Regarding the filling case, I think, We need to form the rte_crypto_ops in the slow path and change only in mutable fields need to update per packet. > > > > Also, since we are agreeing that the ops for SYNC API can be from > > stack/one time allocated, the size shouldn't matter. > > I can be on stack, but it means user will still have to fill them > and PMD will have to read/process/overwrite them. > > > I understand that this would cause ABI breakage, but for this release, > > we can work together and add some reserved fields > > that we can implement later. I believe that's the reason why you want > > to introduce new structures. I think that will bloat > > the existing crypto lib. > > It will increase the lib code, but I don't think it will be significant. > Honestly, I think messing with crypto_op and other existing structures > might have much more negative effect. Yes. We need to change it carefully. > > > If I understand it correctly, this will be used in conjunction with > > IXGBE to handle fragmented IPsec traffic. If that's the fundamental > > reasoning, then there is an alternate path possible. > > No, it's just one of the use-case. > Pretty important, but not the only one. > The main reason - current cryptodev API (crypto_op based) is suboptimal for SW based PMDs. > We wasting too many cycles to pretend that it is a lookaside device underneath. That I agree. I think, it should be fixed by the process() API. > I think makes more sense to admit that it is SW based and exploit it nature, > instead of trying to hide it. Yes. I thought the separate process() device op will solve the major problems. This is just my _personal_ opinion. I leave crypto code contributors to define specifics of API.