Re: [dpdk-dev] [PATCH v3 3/6] app/testpmd: remove restriction on txpkts set

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 9/24/2020 7:08 AM, Chengchang Tang wrote:
> 
> 
> On 2020/9/24 0:59, Ferruh Yigit wrote:
>> On 9/23/2020 12:57 PM, Wei Hu (Xavier) wrote:
>>> Hi, Ferruh Yigit
>>>
>>> On 2020/9/23 11:14, Wei Hu (Xavier) wrote:
>>>> Hi, Ferruh Yigit
>>>>
>>>> On 2020/9/22 22:51, Ferruh Yigit wrote:
>>>>> On 9/19/2020 11:47 AM, Wei Hu (Xavier) wrote:
>>>>>> From: Chengchang Tang <tangchengchang@huawei.com>
>>>>>>
>>>>>> Currently, if nb_txd is not set, the txpkts is not allowed to be set
>>>>>> because the nb_txd is used to avoid the numer of segments exceed the Tx
>>>>>> ring size and the default value of nb_txd is 0. And there is a bug that
>>>>>> nb_txd is the global configuration for Tx ring size and the ring size
>>>>>> could be changed by some command per queue. So these valid check is
>>>>>> unreliable and introduced unnecessary constraints.
>>>>>>
>>>>>> This patch adds a valid check function to use the real Tx ring size to
>>>>>> check the validity of txpkts.
>>>>>>
>>>>>> Fixes: af75078fece3 ("first public release")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>>>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>>>> ---
> 
> <...>
> 
>>>>> What do you think calling 'rte_eth_rx_queue_info_get()' & 'rte_eth_tx_queue_info_get()' to get the 'nb_desc'?
>>>>
>>>> Currently not all PMD driver implement the .rxq_info_get and
>>>>
>>>> .txq_info_get hook function. If calling rte_eth_rx_queue_info_get
>>>>
>>>> return -ENOSTUP, we still need to obtain the ring_size in this way.
>>>>
>>> If calling rte_eth_rx_queue_info_get function get ring_size, because not all PMDS implement the relevant the related hook function, we need to
>>> check the return value and if the return value is -ENOSTUP, we must obtain the ring_size in this way.
>>> Do you prefer this method, right?
>>>
>>
>> Do we really need this check?
> 
> IMHO, these checks are needed. There are two patches use the same method to obtain the ring_size to implement the
> verification in the patch set. One is used to verify the validity of the descriptor ID in the ring. Another is used
> to avoid the number of segments configuration won't exceed the ring_size. For the first case, if no check is
> performed, memory out of bound may occur. For the another one, if no check is performed, the sending may fail when
> the number of segment exceed the ring size.
>>
>> What about verifying 'ring_size' if 'rte_eth_rx_queue_info_get()' returns a valid info, if not ignore the check?
>>
> 
> if we ignore the check when the 'rte_eth_rx_queue_info_get()' returns -ENOTSUP, it may still cause an illegal memory
> access or sending failure.
 >

Ok, thanks for clarification, agree to your suggestion. (to check 
'rte_eth_rx_queue_info_get()' return value and if it is '-ENOSTUP' 
calculate the 'ring_size' as above.)