From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH] qat: addition of optimized content descriptor for AES128-SHA1-HMAC Date: Tue, 21 Jun 2016 15:49:48 +0200 Message-ID: <3195917.C7veDrCJq7@xps13> References: <1466513759-55842-1-git-send-email-john.griffin@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: dev@dpdk.org, deepak.k.jain@intel.com, pablo.de.lara.guarch@intel.com To: John Griffin Return-path: Received: from mail-lb0-f172.google.com (mail-lb0-f172.google.com [209.85.217.172]) by dpdk.org (Postfix) with ESMTP id 075E2B47D for ; Tue, 21 Jun 2016 15:49:49 +0200 (CEST) Received: by mail-lb0-f172.google.com with SMTP id ak10so11254792lbc.3 for ; Tue, 21 Jun 2016 06:49:49 -0700 (PDT) In-Reply-To: <1466513759-55842-1-git-send-email-john.griffin@intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi, I'm not used to review crypto patches but I think this patch can be improved. 2016-06-21 13:55, John Griffin: > Adding an optimized content descriptor for AES128-SHA1-HMAC to > improve thoughput performance. Maybe you can explain how it improves the performance. > +/* > + * Function which will return if it's possible to use the > + * optimised content descriptor. > + */ > +int qat_crypto_sym_use_optimized_alg(struct qat_session *session) [...] > +/* > + * Function to create an optimised content descriptor for AES128 SHA1. > + */ > +int qat_crypto_create_optimzed_session(struct qat_session *session, These function are very specific with a generic name. Maybe that CBC AES128 SHA1 or something like that must be part of the function name. Otherwise you will come with yet another crypto refactoring patch in few weeks. > case ICP_QAT_FW_LA_CMD_CIPHER: > - session = qat_crypto_sym_configure_session_cipher(dev, xform, session); > + session = qat_crypto_sym_configure_session_cipher(dev, xform, > + session); > break; > case ICP_QAT_FW_LA_CMD_AUTH: > - session = qat_crypto_sym_configure_session_auth(dev, xform, session); > + session = qat_crypto_sym_configure_session_auth(dev, xform, > + session); > break; > case ICP_QAT_FW_LA_CMD_CIPHER_HASH: > - session = qat_crypto_sym_configure_session_cipher(dev, xform, session); > - session = qat_crypto_sym_configure_session_auth(dev, xform, session); > - break; > + session = qat_crypto_sym_configure_session_cipher(dev, xform, > + session); > + session = qat_crypto_sym_configure_session_auth(dev, xform, > + session); > + if (qat_crypto_sym_use_optimized_alg(session)) > + qat_crypto_sym_configure_optimized_session(dev, xform, > + session); > + break; > case ICP_QAT_FW_LA_CMD_HASH_CIPHER: > - session = qat_crypto_sym_configure_session_auth(dev, xform, session); > - session = qat_crypto_sym_configure_session_cipher(dev, xform, session); > - break; > + session = qat_crypto_sym_configure_session_auth(dev, xform, > + session); > + session = qat_crypto_sym_configure_session_cipher(dev, xform, > + session); > + if (qat_crypto_sym_use_optimized_alg(session)) > + qat_crypto_sym_configure_optimized_session(dev, xform, > + session); > + break; There is a lot of indent fixing mixed with the addition here. 2 patches would make things easier to understand. > @@ -551,11 +591,11 @@ qat_crypto_sym_configure_session_auth(struct rte_cryptodev *dev, > auth_xform->algo); > goto error_out; > } > - cipher_xform = qat_get_cipher_xform(xform); > > if ((session->qat_hash_alg == ICP_QAT_HW_AUTH_ALGO_GALOIS_128) || > (session->qat_hash_alg == > ICP_QAT_HW_AUTH_ALGO_GALOIS_64)) { > + cipher_xform = qat_get_cipher_xform(xform); > if (qat_alg_aead_session_create_content_desc_auth(session, How this move is related to the patch?