Re: [dpdk-dev] [PATCH 1/3] ethdev: add max burst size to device info

[Shreyansh Jain <shreyansh.jain@nxp.com>]

On Tuesday 12 December 2017 04:33 PM, Ananyev, Konstantin wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Matan Azrad
>> Sent: Tuesday, December 12, 2017 10:46 AM
>> To: Nikhil Agarwal <nikhil.agarwal@linaro.org>; dev@dpdk.org
>> Cc: Hunt, David <david.hunt@intel.com>; nikhil.agarwal@nxp.com; hemant.agrawal@nxp.com; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH 1/3] ethdev: add max burst size to device info
>>
>> Hi Nikhil
>>
>>> -----Original Message-----
>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Nikhil Agarwal
>>> Sent: Tuesday, December 12, 2017 12:05 PM
>>> To: dev@dpdk.org
>>> Cc: david.hunt@intel.com; nikhil.agarwal@nxp.com;
>>> hemant.agrawal@nxp.com; ferruh.yigit@intel.com
>>> Subject: [dpdk-dev] [PATCH 1/3] ethdev: add max burst size to device info
>>>
>>> Currently, if the  rte_eth_rx_burst() function returns a value less than
>>> *nb_pkts*, the application will assume that no more packets are present..
>>>
>>> Some of the hw queue based hardware can only support smaller burst for RX
>>> and TX and thus break the expectation of the rx_burst API.
>>>
>>
>> Doesn't such like devices PMDs should try to retrieve multiple HW burst to adjust the asked received  packet number?
> 
> Same thought here...
> Can't that limitation be hidden inside PMD by calling HW burst multiple times?

This might be required in some cases for performance.
It is possible that for each request containing N buffers, if the PMD 
fetches all N (more than its preferred burst_size), cache misses reduce 
the performance - especially for SoC with limited cache size.

Also, a complete cycle of 
application->driver->hardware->driver->application can help driver 
prefetch buffers - which, in case of hw burst looping, might be too 
little to complete the prefetch cycle.

To summarize, indeed this is for performance specific cases and the idea 
that @Matan gave for renaming 'perf_buf_size' to highlight this, sounds 
logical.

> Also if I am not mistaken - it would increase size of struct rte_eth_dev_info, right?
> If so, then it means ABI breakage.

Yes, deprecation notice should have been sent - if we continue with the 
dev_info change. To me that looks as one of the option. Maybe, someone 
on the list has a better idea.

> Konstantin
> 
>>
>>> This patch adds support to provide the maximum burst size that can be
>>> supported by a given PMD. The dev_info is being memset to '0' in
>>> rte_ethdev library. The value of '0' indicates that any value for burst size can
>>> be supported i.e. no change for existing PMDs.
>>>
>>> The application can now use the lowest available max_burst_size value for
>>> rte_eth_rx_burst.
>>>
>>
>> If you are talking about performance, maybe the right field to expose is something like "perf_burst_size" or "preferred_burst_size".
>> I also suggest to expose different fields for RX and for TX.
>> Maybe the rte_eth_rx\tx_burst() descriptions should be updated.
>>
>> Thanks
>> Matan.
>>
>>> Signed-off-by: Nikhil Agarwal <nikhil.agarwal@linaro.org>
>>> ---
>>>   lib/librte_ether/rte_ethdev.h | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
>>> index 341c2d6..3ab6f02 100644
>>> --- a/lib/librte_ether/rte_ethdev.h
>>> +++ b/lib/librte_ether/rte_ethdev.h
>>> @@ -1047,6 +1047,7 @@ struct rte_eth_dev_info {
>>>   	/** Configured number of rx/tx queues */
>>>   	uint16_t nb_rx_queues; /**< Number of RX queues. */
>>>   	uint16_t nb_tx_queues; /**< Number of TX queues. */
>>> +	uint16_t max_burst_size; /**< MAX burst size, 0 for no limit. */
>>>   };
>>>
>>>   /**
>>> --
>>> 2.7.4
> 
>