RE: [EXTERNAL] [PATCH v2 1/2] crypto/mlx5: optimize AES-GCM IPsec operation

[Suanming Mou <suanmingm@nvidia.com>]

Hi,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Friday, June 14, 2024 5:07 PM
> To: Suanming Mou <suanmingm@nvidia.com>; Matan Azrad
> <matan@nvidia.com>
> Cc: dev@dpdk.org
> Subject: RE: [EXTERNAL] [PATCH v2 1/2] crypto/mlx5: optimize AES-GCM IPsec
> operation
> 
> > Hi Akhil,
> >
> > > -----Original Message-----
> > > From: Akhil Goyal <gakhil@marvell.com>
> > > Sent: Friday, June 14, 2024 2:49 PM
> > > To: Suanming Mou <suanmingm@nvidia.com>; Matan Azrad
> > > <matan@nvidia.com>
> > > Cc: dev@dpdk.org
> > > Subject: RE: [EXTERNAL] [PATCH v2 1/2] crypto/mlx5: optimize AES-GCM
> > > IPsec operation
> > >
> > > > To optimize AES-GCM IPsec operation within crypto/mlx5, the DPDK
> > > > API typically supplies AES_GCM AAD/Payload/Digest in separate
> > > > locations, potentially disrupting their contiguous layout. In
> > > > cases where the memory layout fails to meet hardware (HW)
> > > > requirements, an UMR WQE is initiated ahead of the GCM's GGA WQE
> > > > to establish a continuous AAD/Payload/Digest virtual memory space for the
> HW MMU.
> > > >
> > > > For IPsec scenarios, where the memory layout consistently adheres
> > > > to the fixed order of AAD/IV/Payload/Digest, directly shrinking
> > > > memory for AAD proves more efficient than preparing a UMR WQE. To
> > > > address this, a new devarg "crypto_mode" with mode "ipsec_opt" is
> > > > introduced in the commit, offering an optimization hint
> > > > specifically for IPsec cases. When enabled, the PMD copies AAD
> > > > directly before Payload in the enqueue_burst function instead of
> > > > employing the UMR WQE. Subsequently, in the dequeue_burst
> > > > function, the overridden IV before Payload is restored from the
> > > > GGA WQE. It's crucial for users to avoid utilizing the input mbuf data during
> processing.
> > >
> > > This seems very specific to mlx5 and is not as per the expectations
> > > of cryptodev APIs.
> > >
> > > It seems you are asking to alter the user application to accommodate
> > > this to support IPsec.
> > >
> > > Cryptodev APIs are for generic crypto processing of data as defined
> > > in rte_crypto_op.
> > > With your proposed changes, it seems the behavior of the crypto APIs
> > > will be different in case of mlx5 which I believe is not correct.
> > >
> > > Is it not possible for you to use rte_security IPsec path?
> > >
> >
> > Sorry I don't understand why that conflicts the API, IIUC crypto API
> > only just defines the AAD/Payload/Digest in struct rte_crypto_sym_op,
> > but not restrict the sequence, and AAD/Payload/Digest may come from
> difference memory space.
> > Am I missing something here?
> 
> Yes you are correct that there is no restriction there.
> 
> > The input requirements from mlx5 HW is AAD/Payload/Digest sequence, if
> > the input memory of AAD/Payload/Digest does not meet the requirements,
> > PMD will try to combine the memory address space with UMR WQE as that
> > commit does by software shrink.
> 
> And here, you are adding a restriction for IPsec case.
> I believe you need a way to identify IPsec case with non-ipsec case in data path.
> For that, instead of using a devarg(which is a specific case for mlx5), you can use
> generic rte_security session with action type
> RTE_SECURITY_ACTION_TYPE_NONE.

Just to emphasize, this is not a restriction, we don't restrict user must use that devarg for IPSEC case.
The way to identify or apply that optimization is user's devarg of "ipsec_opt".
Without that hint from devarg, pmd will work in UMR mode to combine the memory addresses.
I agree move to other API will also make the hint work. But if providing one hint devarg here does not conflict the API and bring better compatibility, it does not hurt.

> 
> You may also benefit from rte_ipsec library APIs and test framework, for
> processing of protocol specific things which are specifically written for
> RTE_SECURITY_ACTION_TYPE_NONE case.
And again, thanks for the suggestion, I assume we will also consider supporting that next for rte_security as well if possible, to provide more choice for user.

> 
> > And the most important thing is that "ipsec_opt" is not mandatory,
> > only if user have such case of layout and allows that optimization
> > happens. Otherwise, the existing UMR WQE will still be the default behavior
> here.
> >
> With this new devarg which application would you be using for testing?
> I do not see a patch for application changes for the layout that you are
> mentioning.
IIRC we used test-crypto-perf with headroom proper configured mbuf to verify.
If you think another new arg is worth to be added to test-crypto-perf to build everything, I can also send another application patch to support that later.(but sorry due to effort limitation maybe not happened soon in that series).

Thanks,
Suanming