diff mbox series

[v3,3/6] app/testpmd: remove restriction on txpkts set

Message ID 20200919104708.58451-4-xavier_huwei@163.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series minor fixes for testpmd |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Wei Hu (Xavier) Sept. 19, 2020, 10:47 a.m. UTC 
  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>
---
 app/test-pmd/config.c | 42 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

Comments

Ferruh Yigit Sept. 22, 2020, 2:51 p.m. UTC | #1 
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>
> ---
>   app/test-pmd/config.c | 42 ++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 4e33208..882de2d 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2984,17 +2984,51 @@ show_tx_pkt_segments(void)
>   	printf("Split packet: %s\n", split);
>   }
>   
> +static bool
> +nb_segs_is_invalid(unsigned int nb_segs)
> +{
> +	uint16_t port_id;
> +
> +	RTE_ETH_FOREACH_DEV(port_id) {
> +		struct rte_port *port = &ports[port_id];
> +		uint16_t ring_size;
> +		uint16_t queue_id;
> +
> +		/*
> +		 * When configure the txq by rte_eth_tx_queue_setup with
> +		 * nb_tx_desc being 0, it will use a default value provided by
> +		 * PMDs to setup this txq. If the default value is 0, it will
> +		 * use the RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
> +		 */
> +		for (queue_id = 0; queue_id < nb_txq; queue_id++) {
> +			if (port->nb_tx_desc[queue_id])
> +				ring_size = port->nb_tx_desc[queue_id];
> +			else if (port->dev_info.default_txportconf.ring_size)
> +				ring_size =
> +				    port->dev_info.default_txportconf.ring_size;
> +			else
> +				ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
> +
> +			if (ring_size < nb_segs) {
> +				printf("nb segments per TX packets=%u >= TX "
> +				       "queue(%u) ring_size=%u - ignored\n",
> +					nb_segs, queue_id, ring_size);
> +				return true;
> +			}
> +		}

What do you think calling 'rte_eth_rx_queue_info_get()' & 
'rte_eth_tx_queue_info_get()' to get the 'nb_desc'?
Wei Hu (Xavier) Sept. 23, 2020, 3:14 a.m. UTC | #2 
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>
>> ---
>>   app/test-pmd/config.c | 42 ++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 38 insertions(+), 4 deletions(-)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>> index 4e33208..882de2d 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -2984,17 +2984,51 @@ show_tx_pkt_segments(void)
>>       printf("Split packet: %s\n", split);
>>   }
>>   +static bool
>> +nb_segs_is_invalid(unsigned int nb_segs)
>> +{
>> +    uint16_t port_id;
>> +
>> +    RTE_ETH_FOREACH_DEV(port_id) {
>> +        struct rte_port *port = &ports[port_id];
>> +        uint16_t ring_size;
>> +        uint16_t queue_id;
>> +
>> +        /*
>> +         * When configure the txq by rte_eth_tx_queue_setup with
>> +         * nb_tx_desc being 0, it will use a default value provided by
>> +         * PMDs to setup this txq. If the default value is 0, it will
>> +         * use the RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
>> +         */
>> +        for (queue_id = 0; queue_id < nb_txq; queue_id++) {
>> +            if (port->nb_tx_desc[queue_id])
>> +                ring_size = port->nb_tx_desc[queue_id];
>> +            else if (port->dev_info.default_txportconf.ring_size)
>> +                ring_size =
>> + port->dev_info.default_txportconf.ring_size;
>> +            else
>> +                ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
>> +
>> +            if (ring_size < nb_segs) {
>> +                printf("nb segments per TX packets=%u >= TX "
>> +                       "queue(%u) ring_size=%u - ignored\n",
>> +                    nb_segs, queue_id, ring_size);
>> +                return true;
>> +            }
>> +        }
>
> 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.


Thanks, Xavier
Wei Hu (Xavier) Sept. 23, 2020, 11:57 a.m. UTC | #3 
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>
>>> ---
>>>   app/test-pmd/config.c | 42 ++++++++++++++++++++++++++++++++++++++----
>>>   1 file changed, 38 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>> index 4e33208..882de2d 100644
>>> --- a/app/test-pmd/config.c
>>> +++ b/app/test-pmd/config.c
>>> @@ -2984,17 +2984,51 @@ show_tx_pkt_segments(void)
>>>       printf("Split packet: %s\n", split);
>>>   }
>>>   +static bool
>>> +nb_segs_is_invalid(unsigned int nb_segs)
>>> +{
>>> +    uint16_t port_id;
>>> +
>>> +    RTE_ETH_FOREACH_DEV(port_id) {
>>> +        struct rte_port *port = &ports[port_id];
>>> +        uint16_t ring_size;
>>> +        uint16_t queue_id;
>>> +
>>> +        /*
>>> +         * When configure the txq by rte_eth_tx_queue_setup with
>>> +         * nb_tx_desc being 0, it will use a default value provided by
>>> +         * PMDs to setup this txq. If the default value is 0, it will
>>> +         * use the RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
>>> +         */
>>> +        for (queue_id = 0; queue_id < nb_txq; queue_id++) {
>>> +            if (port->nb_tx_desc[queue_id])
>>> +                ring_size = port->nb_tx_desc[queue_id];
>>> +            else if (port->dev_info.default_txportconf.ring_size)
>>> +                ring_size =
>>> + port->dev_info.default_txportconf.ring_size;
>>> +            else
>>> +                ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
>>> +
>>> +            if (ring_size < nb_segs) {
>>> +                printf("nb segments per TX packets=%u >= TX "
>>> +                       "queue(%u) ring_size=%u - ignored\n",
>>> +                    nb_segs, queue_id, ring_size);
>>> +                return true;
>>> +            }
>>> +        }
>>
>> 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?

Hopes for your suggestion.
Thanks, Xavier
> 
> Thanks, Xavier
Ferruh Yigit Sept. 23, 2020, 4:59 p.m. UTC | #4 
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>
>>>> ---
>>>>   app/test-pmd/config.c | 42 ++++++++++++++++++++++++++++++++++++++----
>>>>   1 file changed, 38 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>>> index 4e33208..882de2d 100644
>>>> --- a/app/test-pmd/config.c
>>>> +++ b/app/test-pmd/config.c
>>>> @@ -2984,17 +2984,51 @@ show_tx_pkt_segments(void)
>>>>       printf("Split packet: %s\n", split);
>>>>   }
>>>>   +static bool
>>>> +nb_segs_is_invalid(unsigned int nb_segs)
>>>> +{
>>>> +    uint16_t port_id;
>>>> +
>>>> +    RTE_ETH_FOREACH_DEV(port_id) {
>>>> +        struct rte_port *port = &ports[port_id];
>>>> +        uint16_t ring_size;
>>>> +        uint16_t queue_id;
>>>> +
>>>> +        /*
>>>> +         * When configure the txq by rte_eth_tx_queue_setup with
>>>> +         * nb_tx_desc being 0, it will use a default value provided by
>>>> +         * PMDs to setup this txq. If the default value is 0, it will
>>>> +         * use the RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
>>>> +         */
>>>> +        for (queue_id = 0; queue_id < nb_txq; queue_id++) {
>>>> +            if (port->nb_tx_desc[queue_id])
>>>> +                ring_size = port->nb_tx_desc[queue_id];
>>>> +            else if (port->dev_info.default_txportconf.ring_size)
>>>> +                ring_size =
>>>> + port->dev_info.default_txportconf.ring_size;
>>>> +            else
>>>> +                ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
>>>> +
>>>> +            if (ring_size < nb_segs) {
>>>> +                printf("nb segments per TX packets=%u >= TX "
>>>> +                       "queue(%u) ring_size=%u - ignored\n",
>>>> +                    nb_segs, queue_id, ring_size);
>>>> +                return true;
>>>> +            }
>>>> +        }
>>>
>>> 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?

What about verifying 'ring_size' if 'rte_eth_rx_queue_info_get()' 
returns a valid info, if not ignore the check?

> 
> Hopes for your suggestion.
> Thanks, Xavier
>>
>> Thanks, Xavier
Chengchang Tang Sept. 24, 2020, 6:08 a.m. UTC | #5 
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.
>>
<...>
> .
>
Ferruh Yigit Sept. 24, 2020, 12:19 p.m. UTC | #6 
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.)
diff mbox series

Patch

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 4e33208..882de2d 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2984,17 +2984,51 @@  show_tx_pkt_segments(void)
 	printf("Split packet: %s\n", split);
 }
 
+static bool
+nb_segs_is_invalid(unsigned int nb_segs)
+{
+	uint16_t port_id;
+
+	RTE_ETH_FOREACH_DEV(port_id) {
+		struct rte_port *port = &ports[port_id];
+		uint16_t ring_size;
+		uint16_t queue_id;
+
+		/*
+		 * When configure the txq by rte_eth_tx_queue_setup with
+		 * nb_tx_desc being 0, it will use a default value provided by
+		 * PMDs to setup this txq. If the default value is 0, it will
+		 * use the RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
+		 */
+		for (queue_id = 0; queue_id < nb_txq; queue_id++) {
+			if (port->nb_tx_desc[queue_id])
+				ring_size = port->nb_tx_desc[queue_id];
+			else if (port->dev_info.default_txportconf.ring_size)
+				ring_size =
+				    port->dev_info.default_txportconf.ring_size;
+			else
+				ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
+
+			if (ring_size < nb_segs) {
+				printf("nb segments per TX packets=%u >= TX "
+				       "queue(%u) ring_size=%u - ignored\n",
+					nb_segs, queue_id, ring_size);
+				return true;
+			}
+		}
+	}
+
+	return false;
+}
+
 void
 set_tx_pkt_segments(unsigned *seg_lengths, unsigned nb_segs)
 {
 	uint16_t tx_pkt_len;
 	unsigned i;
 
-	if (nb_segs >= (unsigned) nb_txd) {
-		printf("nb segments per TX packets=%u >= nb_txd=%u - ignored\n",
-		       nb_segs, (unsigned int) nb_txd);
+	if (nb_segs_is_invalid(nb_segs))
 		return;
-	}
 
 	/*
 	 * Check that each segment length is greater or equal than