Hi Pablo, Thanks for the patch. It is very good idea of allocating only necessary buffer for digests in the operation. Comments inline:
> -----Original Message----- > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Pablo de Lara > Sent: Friday, August 18, 2017 8:21 AM > To: Doherty, Declan <declan.dohe...@intel.com>; > jerin.ja...@caviumnetworks.com > Cc: dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.gua...@intel.com> > Subject: [dpdk-dev] [PATCH 1/8] crypto/aesni_gcm: do not append digest > > When performing an authentication verification, the PMD was using memory > at the end of the input buffer, to store temporarily the digest. > This operation requires the buffer to have enough tailroom unnecessarily. > Instead, memory is allocated for each queue pair, to store temporarily the > digest generated by the driver, so it can be compared with the one provided > in the crypto operation, without needing to touch the input buffer. > > Signed-off-by: Pablo de Lara <pablo.de.lara.gua...@intel.com> > --- > drivers/crypto/aesni_gcm/aesni_gcm_pmd.c | 31 > +++++------------------- > drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h | 7 ++++++ > 2 files changed, 13 insertions(+), 25 deletions(-) > > diff --git a/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c > b/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c > index d9c91d0..ae670a7 100644 > --- a/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c > +++ b/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c > @@ -298,14 +298,7 @@ process_gcm_crypto_op(struct aesni_gcm_qp *qp, > struct rte_crypto_op *op, > sym_op->aead.digest.data, > (uint64_t)session->digest_length); > } else if (session->op == > AESNI_GCM_OP_AUTHENTICATED_DECRYPTION) { > - uint8_t *auth_tag = (uint8_t > *)rte_pktmbuf_append(sym_op->m_dst ? > - sym_op->m_dst : sym_op->m_src, > - session->digest_length); > - > - if (!auth_tag) { > - GCM_LOG_ERR("auth_tag"); > - return -1; > - } qp->tmp_digest has already the data type of uint8_t*, the casting is not necessary here. Although the "&" here seems to be wrong. auth_tag didn't point to qp->tmp_digest but a buffer with the address as &qp->tmp_digest. > + uint8_t *auth_tag = (uint8_t *)&qp->temp_digest; > qp->ops[session->key].init(&session->gdata_key, > &qp->gdata_ctx, > @@ -350,14 +343,7 @@ process_gcm_crypto_op(struct aesni_gcm_qp *qp, > struct rte_crypto_op *op, > sym_op->auth.digest.data, > (uint64_t)session->digest_length); > } else { /* AESNI_GMAC_OP_VERIFY */ > - uint8_t *auth_tag = (uint8_t > *)rte_pktmbuf_append(sym_op->m_dst ? > - sym_op->m_dst : sym_op->m_src, > - session->digest_length); > - > - if (!auth_tag) { > - GCM_LOG_ERR("auth_tag"); > - return -1; > - } Same here. > + uint8_t *auth_tag = (uint8_t *)&qp->temp_digest; > > qp->ops[session->key].init(&session->gdata_key, > &qp->gdata_ctx, > @@ -385,11 +371,10 @@ process_gcm_crypto_op(struct aesni_gcm_qp *qp, > struct rte_crypto_op *op, > * - Returns NULL on invalid job > */ > static void > -post_process_gcm_crypto_op(struct rte_crypto_op *op, > +post_process_gcm_crypto_op(struct aesni_gcm_qp *qp, > + struct rte_crypto_op *op, > struct aesni_gcm_session *session) > { > - struct rte_mbuf *m = op->sym->m_dst ? op->sym->m_dst : op- > >sym->m_src; > - > op->status = RTE_CRYPTO_OP_STATUS_SUCCESS; > > /* Verify digest if required */ > @@ -397,8 +382,7 @@ post_process_gcm_crypto_op(struct rte_crypto_op > *op, > session->op == AESNI_GMAC_OP_VERIFY) { > uint8_t *digest; > Same here. > - uint8_t *tag = rte_pktmbuf_mtod_offset(m, uint8_t *, > - m->data_len - session->digest_length); > + uint8_t *tag = (uint8_t *)&qp->temp_digest; > ... > > -- > 2.9.4 The same problem lies the rest of your patches in the patchset. Regards, Fan
