Re: [dpdk-dev] [RFC v3 1/1] lib: add compressdev API

[Ahmed Mansour <ahmed.mansour@nxp.com>]

Hi All,

Please see responses in line.

Thanks,

Ahmed

On 1/23/2018 6:58 AM, Verma, Shally wrote:
> Hi Fiona
>
>> -----Original Message-----
>> From: Trahe, Fiona [mailto:fiona.trahe@intel.com]
>> Sent: 19 January 2018 17:30
>> To: Verma, Shally <Shally.Verma@cavium.com>; dev@dpdk.org;
>> akhil.goyal@nxp.com
>> Cc: Challa, Mahipal <Mahipal.Challa@cavium.com>; Athreya, Narayana
>> Prasad <NarayanaPrasad.Athreya@cavium.com>; De Lara Guarch, Pablo
>> <pablo.de.lara.guarch@intel.com>; Gupta, Ashish
>> <Ashish.Gupta@cavium.com>; Sahu, Sunila <Sunila.Sahu@cavium.com>;
>> Jain, Deepak K <deepak.k.jain@intel.com>; Hemant Agrawal
>> <hemant.agrawal@nxp.com>; Roy Pledge <roy.pledge@nxp.com>; Youri
>> Querry <youri.querry_1@nxp.com>; Ahmed Mansour
>> <ahmed.mansour@nxp.com>; Trahe, Fiona <fiona.trahe@intel.com>
>> Subject: RE: [RFC v3 1/1] lib: add compressdev API
>>
>> Hi Shally,
>>
>>> -----Original Message-----
>>> From: Verma, Shally [mailto:Shally.Verma@cavium.com]
>>> Sent: Thursday, January 18, 2018 12:54 PM
>>> To: Trahe, Fiona <fiona.trahe@intel.com>; dev@dpdk.org
>>> Cc: Challa, Mahipal <Mahipal.Challa@cavium.com>; Athreya, Narayana
>> Prasad
>>> <NarayanaPrasad.Athreya@cavium.com>; De Lara Guarch, Pablo
>> <pablo.de.lara.guarch@intel.com>;
>>> Gupta, Ashish <Ashish.Gupta@cavium.com>; Sahu, Sunila
>> <Sunila.Sahu@cavium.com>; Jain, Deepak K
>>> <deepak.k.jain@intel.com>; Hemant Agrawal
>> <hemant.agrawal@nxp.com>; Roy Pledge
>>> <roy.pledge@nxp.com>; Youri Querry <youri.querry_1@nxp.com>;
>> Ahmed Mansour
>>> <ahmed.mansour@nxp.com>
>>> Subject: RE: [RFC v3 1/1] lib: add compressdev API
>>>
>>> Hi Fiona
>>>
>>> While revisiting this, we identified few questions and additions. Please see
>> them inline.
>>>
>>>> -----Original Message-----
>>>> From: Trahe, Fiona [mailto:fiona.trahe@intel.com]
>>>> Sent: 15 December 2017 23:19
>>>> To: dev@dpdk.org; Verma, Shally <Shally.Verma@cavium.com>
>>>> Cc: Challa, Mahipal <Mahipal.Challa@cavium.com>; Athreya, Narayana
>>>> Prasad <NarayanaPrasad.Athreya@cavium.com>;
>>>> pablo.de.lara.guarch@intel.com; fiona.trahe@intel.com
>>>> Subject: [RFC v3 1/1] lib: add compressdev API
>>>>
>>>> Signed-off-by: Trahe, Fiona <fiona.trahe@intel.com>
>>>> ---
>>> //snip
>>>
>>>> +
>>>> +int
>>>> +rte_compressdev_queue_pair_setup(uint8_t dev_id, uint16_t
>>>> queue_pair_id,
>>>> +		uint32_t max_inflight_ops, int socket_id)
>>> [Shally] Is max_inflights_ops different from nb_streams_per_qp in struct
>> rte_compressdev_info?
>>> I assume they both carry same purpose. If yes, then it will be better to use
>> single naming convention to
>>> avoid confusion.
>> [Fiona] No, I think they have different purposes.
>> max_inflight_ops should be used to configure the qp with the number of ops
>> the application expects to be able to submit to the qp before it needs to poll
>> for a response. It can be configured differently for each qp. In the QAT case it
>> dictates the depth of the qp created, it may have different implications on
>> other PMDs.
>> nb_sessions_per_qp and nb_streams_per_qp are limitations the devices
>> reports and are same for all qps on the device. QAT doesn't have those
>> limitations and so would report 0, however I assumed they may be necessary
>> for other devices.
>> This assumption is based on the patch submitted by NXP to cryptodev in Feb
>> 2017
>>  https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdpdk.org%2Fml%2Farchives%2Fdev%2F2017-March%2F060740.html&data=02%7C01%7Cahmed.mansour%40nxp.com%7Cb012d74d7530493b155108d56258955f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636523054981379413&sdata=2SazlEazMxcBGS7R58CpNrX0G5OeWx8PLMwf%2FYzqv34%3D&reserved=0
>> I also assume these are not necessarily the max number of sessions in ops on
>> the qp at a given time, but the total number attached, i.e. if the device has
>> this limitation then sessions must be attached to qps, and presumably
>> reserve some resources. Being attached doesn't imply there is an op on the
>> qp at that time using that session. So it's not to relating to the inflight op
>> count, but to the number of sessions attached/detached to the qp.
>> Including Akhil on the To list, maybe NXP can confirm if these params are
>> needed.
> [Shally] Ok. Then let's wait for NXP to confirm on this requirement as currently spec doesn't have any API to attach queue_pair_to_specific_session_or_stream as cryptodev.
>
> But then how application could know limit on max_inflight_ops supported on a qp? As it can pass any random number during qp_setup().
> Do you believe we need to add a capability field in dev_info to indicate limit on max_inflight_ops?
>
> Thanks
> Shally
[Ahmed] @Fiona This looks ok. max_inflight_ops makes sense. I understand
it as a push back mechanism per qp. We do not have physical limit for
number of streams or sessions on a qp in our hardware, so we would
return 0 here as well.
@Shally in our PMD implementation we do not attach streams or sessions
to a particular qp. Regarding max_inflight_ops. I think that limit
should be independent of hardware. Not all enqueues must succeed. The
hardware can push back against the enqueuer dynamically if the resources
needed to accommodate additional ops are not available yet. This push
back happens in the software if the user sets a max_inflight_ops that is
less that the hardware max_inflight_ops. The same return pathway can be
exercised if the user actually attempts to enqueue more than the
supported max_inflight_ops because of the hardware.
>>
>>> Also, is it optional API? Like Is this a valid use case?:
>>> dev_configure() --> dev_start() --> qp_start() --> enqueue/dequeue() -->
>> qp_stop() --> dev_stop() -->
>>> dev_close()?
>> [Fiona] I don't think it should be optional as some PMDs need to allocate
>> resources based on the setup data passed in on this API.
>>
>>> //snip
>>>
>>>> +
>>>> +#define RTE_COMPRESSDEV_PMD_NAME_ARG
>>>> 	("name")
>>>> +#define RTE_COMPRESSDEV_PMD_MAX_NB_QP_ARG
>>>> 	("max_nb_queue_pairs")
>>>> +#define RTE_COMPRESSDEV_PMD_SOCKET_ID_ARG
>> 	("socket_id")
>>>> +
>>> [Shally] Need to define argument macro for max_nb_session_per_qp and
>> max_nb_streams_per_qp  as
>>> well
>> [Fiona] ok
>>
>>>> +
>>>> +static const char * const compressdev_pmd_valid_params[] = {
>>>> +	RTE_COMPRESSDEV_PMD_NAME_ARG,
>>>> +	RTE_COMPRESSDEV_PMD_MAX_NB_QP_ARG,
>>>> +	RTE_COMPRESSDEV_PMD_SOCKET_ID_ARG
>>>> +};
>>> [Shally] Likewise, array need to be updated with other mentioned two
>> arguments
>> Fiona] ok
>>
>>
>>>> +
>>>> +/**
>>>> + * @internal
>>>> + * Initialisation parameters for comp devices
>>>> + */
>>>> +struct rte_compressdev_pmd_init_params {
>>>> +	char name[RTE_COMPRESSDEV_NAME_MAX_LEN];
>>>> +	size_t private_data_size;
>>>> +	int socket_id;
>>>> +	unsigned int max_nb_queue_pairs;
>>> [Shally] And this also need to be updated with max_nb_sessions_per_qp
>> and max_streams_per_qp
>> [Fiona] ok
>>
>>> //snip
>>>
>>> Thanks
>>> Shally