Hi Thomas, > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Tuesday, June 21, 2016 2:50 PM > To: Griffin, John <john.griffin at intel.com> > Cc: dev at dpdk.org; Jain, Deepak K <deepak.k.jain at intel.com>; De Lara > Guarch, Pablo <pablo.de.lara.guarch at intel.com> > Subject: Re: [dpdk-dev] [PATCH] qat: addition of optimized content > descriptor for AES128-SHA1-HMAC > > 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. [JFG] Can add some text- it's though a reduction in the amount of the session information which DMA'ed to the device - helps to reduce PCIe b/w. I'll add some text.
> > > +/* > > + * 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. [JFG] [JFG] Ok - I might not rename, but I'll make the implementation more generic to all it cope better with the potential addition of other algorithms. > > > 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. [JFG] [JFG] Ok let me remove from this patch. > > > @@ -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? [JFG] It's not tbh - I'll remove from this patch.