Re: [dpdk-dev] [dpdk-dev, v1, 2/5] eventdev: add crypto adapter implementation

[Akhil Goyal <akhil.goyal@nxp.com>]

On 4/20/2018 6:44 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Fri, 20 Apr 2018 17:04:36 +0530
>> From: Akhil Goyal <akhil.goyal@nxp.com>
>> To: "Gujjar, Abhinandan S" <abhinandan.gujjar@intel.com>,
>>  "jerin.jacob@caviumnetworks.com" <jerin.jacob@caviumnetworks.com>,
>>  "hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>, "dev@dpdk.org"
>>  <dev@dpdk.org>
>> CC: "De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>, "Doherty,
>>  Declan" <declan.doherty@intel.com>, "Vangati, Narender"
>>  <narender.vangati@intel.com>, "Rao, Nikhil" <nikhil.rao@intel.com>, "Eads,
>>  Gage" <gage.eads@intel.com>
>> Subject: Re: [dpdk-dev, v1, 2/5] eventdev: add crypto adapter implementation
>> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
>>  Thunderbird/45.8.0
>>
>> Hi Abhinandan/ Jerin,
>> On 4/18/2018 11:51 AM, Gujjar, Abhinandan S wrote:
>>> Hi Akhil,
>>>
>>> Please find the comments inline.
>>>
>>>> -----Original Message-----
>>>> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
>>>> Sent: Tuesday, April 17, 2018 7:48 PM
>>>> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>;
>>>> jerin.jacob@caviumnetworks.com; hemant.agrawal@nxp.com; dev@dpdk.org
>>>> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Doherty, Declan
>>>> <declan.doherty@intel.com>; Vangati, Narender
>>>> <narender.vangati@intel.com>; Rao, Nikhil <nikhil.rao@intel.com>; Eads, Gage
>>>> <gage.eads@intel.com>
>>>> Subject: Re: [dpdk-dev, v1, 2/5] eventdev: add crypto adapter implementation
>>>>
>>>> Hi Abhinandan,
>>>>
>>>> I have not reviewed the patch completely. But I have below query for further
>>>> review.
>>>> On 4/4/2018 12:26 PM, Abhinandan Gujjar wrote:
>>>>> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
>>>>> Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
>>>>> Signed-off-by: Gage Eads <gage.eads@intel.com>
>>>>> ---
>>>>
>>>> [..snip..]
>>>>> +
>>>>> +int __rte_experimental
>>>>> +rte_event_crypto_adapter_queue_pair_add(uint8_t id,
>>>>> +					uint8_t cdev_id,
>>>>> +					int32_t queue_pair_id)
>>>>> +{
>>>>> +	struct rte_event_crypto_adapter *adapter;
>>>>> +	struct rte_eventdev *dev;
>>>>> +	struct crypto_device_info *dev_info;
>>>>> +	uint32_t cap;
>>>>> +	int ret;
>>>>> +
>>>>> +	RTE_EVENT_CRYPTO_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL);
>>>>> +
>>>>> +	if (!rte_cryptodev_pmd_is_valid_dev(cdev_id)) {
>>>>> +		RTE_EDEV_LOG_ERR("Invalid dev_id=%" PRIu8, cdev_id);
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	adapter = eca_id_to_adapter(id);
>>>>> +	if (adapter == NULL)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	dev = &rte_eventdevs[adapter->eventdev_id];
>>>>> +	ret = rte_event_crypto_adapter_caps_get(adapter->eventdev_id,
>>>>> +						cdev_id,
>>>>> +						&cap);
>>>>> +	if (ret) {
>>>>> +		RTE_EDEV_LOG_ERR("Failed to get adapter caps dev %" PRIu8
>>>>> +			"cdev %" PRIu8, id, cdev_id);
>>>>> +		return ret;
>>>>> +	}
>>>>> +
>>>>> +	dev_info = &adapter->cdevs[cdev_id];
>>>>> +
>>>>> +	if (queue_pair_id != -1 &&
>>>>> +	    (uint16_t)queue_pair_id >= dev_info->dev->data->nb_queue_pairs) {
>>>>> +		RTE_EDEV_LOG_ERR("Invalid queue_pair_id %" PRIu16,
>>>>> +				 (uint16_t)queue_pair_id);
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	if (cap & RTE_EVENT_CRYPTO_ADAPTER_CAP_INTERNAL_PORT) {
>>>>> +		RTE_FUNC_PTR_OR_ERR_RET(
>>>>> +			*dev->dev_ops->crypto_adapter_queue_pair_add,
>>>>> +			-ENOTSUP);
>>>>> +		if (dev_info->qpairs == NULL) {
>>>>> +			dev_info->qpairs =
>>>>> +			    rte_zmalloc_socket(adapter->mem_name,
>>>>> +					dev_info->dev->data->nb_queue_pairs
>>>> *
>>>>> +					sizeof(struct crypto_queue_pair_info),
>>>>> +					0, adapter->socket_id);
>>>>> +			if (dev_info->qpairs == NULL)
>>>>> +				return -ENOMEM;
>>>>> +		}
>>>>> +
>>>>> +		ret = (*dev->dev_ops->crypto_adapter_queue_pair_add)(dev,
>>>>> +				dev_info->dev,
>>>>> +				queue_pair_id);
>>>>
>>>> crypto_adapter_queue_pair_add is supposed to attach a queue
>>>> (queue_pair_id) of cryptodev(dev_info->dev) to event device (dev).
>>>> But how will the underlying implementation attach it to event device without
>>>> knowing the eventdev queue_id. This information was coming in the RFC
>>>> patches with the parameter (rte_event_crypto_queue_pair_conf).
>>>> Why is this removed and if removed how will the driver attach the queue.
>>>> I can see that rte_event is passed in the session private data but how can we
>>>> attach the crypto queue with event dev queue?
>>>
>>> Yes, this was present in the first version of the RFC which is similar to eth rx adapter.
>>> After couple of discussions, thread http://dpdk.org/dev/patchwork/patch/31752/),
>>> it was changed. In eth rx adapter, eth queues are mapped to eventdev, whereas in crypto
>>> adapter the sessions are mapped to eventdev. Since event info is present along with the
>>> session, the get API has to be called in respective API to get the event information and
>>> then map to eventdev.
>>>
>>
>> I think the intent of that discussion was misunderstood from our end.
>> But this is not going to work for hardware devices.
>>
>> Because in case of hardware implementation, the scheduling is done in
>> hardware and hardware cannot call the get API to get the event information
>> then map to event device. Actually the scheduling has happened before the
>> crypto_op is dequeued from the event port. So there is no point of set/get
>> private data in our case.
>>
>> We need to map the crypto queues to the event queue_ids at the time of
>> queue_pair add API. In hardware scheduler, we map n(may be 1-8) crypto
>> queues to m event queues(<= n). We can assign multiple sessions to any
>> crypto queue pair, and after the crypto op is received by event queue, they
>> are appropriately scheduled by hardware to event ports.
>>
>> Session based mapping to event queue cannot be supported. Our design is same
>> as that of eth rx adapter.
>
> Crypto queue pair to eventdev queue mapping should be supported. But
> That's a limited set. meaning if an application needs millions of IPSec SA sessions
> then we can not map it. So, IMO, If an HW/SW can not support session
> based mapping then it needs to be exposed/abstracted through capabilities.
>
> crypto qp to event queue mapping will be supported in all adapter
> implementation.
>
> Does that sounds OK?
>

Yes, that will be fine for us.

Thanks,
Akhil