RE: [EXT] [PATCH v3 1/4] crypto: use single buffer for asymmetric session

["Power, Ciara" <ciara.power@intel.com>]

Hi Akhil,

Left some replies inline. I will address all other comments in v4.

Thanks,
Ciara

>-----Original Message-----
>From: Akhil Goyal <gakhil@marvell.com>
>Sent: Monday 7 February 2022 08:20
>To: Power, Ciara <ciara.power@intel.com>; dev@dpdk.org
>Cc: Zhang, Roy Fan <roy.fan.zhang@intel.com>; Anoob Joseph
><anoobj@marvell.com>; mdr@ashroe.eu; Doherty, Declan
><declan.doherty@intel.com>; Ankur Dwivedi <adwivedi@marvell.com>;
>Tejasree Kondoj <ktejasree@marvell.com>; Griffin, John
><john.griffin@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>; Jain,
>Deepak K <deepak.k.jain@intel.com>
>Subject: RE: [EXT] [PATCH v3 1/4] crypto: use single buffer for asymmetric
>session
>
>> Rather than using a session buffer that contains pointers to private
>> session data elsewhere, have a single session buffer.
>> This session is created for a driver ID, and the mempool element
>> contains space for the max session private data needed for any driver.
>
>This means asymmetric ops are not allowed with scheduler PMD.
>
[CP]  Yes, currently asymmetric isn't supported for scheduler PMD anyway so this shouldn't be an issue for this patchset.
For the approach to be applied to symmetric crypto also in future release, the scheduler PMD would need to be reworked to align with the new session usage.

<snip>
		return NULL;
>> @@ -1919,10 +1957,27 @@ rte_cryptodev_asym_session_create(struct
>> rte_mempool *mp)
>>  		return NULL;
>>  	}
>>
>> +	sess->driver_id = dev->driver_id;
>> +	sess->max_priv_session_sz = pool_priv->max_priv_session_sz;
>> +
>>  	/* Clear device session pointer.
>>  	 * Include the flag indicating presence of private data
>>  	 */
>> -	memset(sess, 0, session_size);
>> +	memset(sess->sess_private_data, 0, session_priv_data_sz);
>> +
>> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
>> >asym_session_configure, NULL);
>> +
>> +	if (sess->sess_private_data[0] == 0) {
>> +		ret = dev->dev_ops->asym_session_configure(dev,
>> +							xforms,
>> +							sess, mp);
>
>The mempool object is allocated in the library layer, so why is it need to be
>passed to PMD? PMD cannot get mempool object. Right?

[CP] Yes true, I don't think the mempool needs to be passed to the configure function anymore, same with dev,
these were just used to allocate private session data before I believe. Will remove these parameters altogether instead of leaving as rte_unused.

<snip>
>> -/**
>> - * Initialize asymmetric session on a device with specific asymmetric
>> xform
>> - *
>> - * @param   dev_id   ID of device that we want the session to be used on
>> - * @param   sess     Session to be set up on a device
>> - * @param   xforms   Asymmetric crypto transform operations to apply on
>flow
>> - *                   processed with this session
>> - * @param   mempool  Mempool to be used for internal allocation.
>> - *
>> - * @return
>> - *  - On success, zero.
>> - *  - -EINVAL if input parameters are invalid.
>> - *  - -ENOTSUP if crypto device does not support the crypto transform.
>> - *  - -ENOMEM if the private session could not be allocated.
>> - */
>
>These error numbers should be added in the create() API.
>I guess your subsequent patch is doing that.

[CP] Correct, the 4th patch changes the return values of the create() function and adds these errors numbers. 

<snip>