diff mbox series

[v2,4/4] app/testpmd: fix displaying Rx Tx queues information

Message ID 20200820014204.25035-5-huwei013@chinasoftinc.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series minor fixes for testpmd |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/travis-robot success Travis build: passed

Commit Message

Wei Hu (Xavier) Aug. 20, 2020, 1:42 a.m. UTC 
  From: Huisong Li <lihuisong@huawei.com>

Currently, the information of Rx/Tx queues from PMD driver is not displayed
exactly in the rxtx_config_display function. Because "ports[pid].rx_conf"
and "ports[pid].tx_conf" maintained in testpmd application may be not the
value actually used by PMD driver. For instance, user does not set a field,
but PMD driver has to use the default value.

This patch fixes rxtx_config_display so that the information of Rx/Tx
queues can be really displayed for the PMD driver that implement
.rxq_info_get and .txq_info_get ops callback function.

Fixes: 75c530c1bd5351 ("app/testpmd: fix port configuration print")
Fixes: d44f8a485f5d1f ("app/testpmd: enable per queue configure")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 app/test-pmd/config.c | 64 +++++++++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 17 deletions(-)

Comments

Ferruh Yigit Sept. 14, 2020, 4:31 p.m. UTC | #1 
On 8/20/2020 2:42 AM, Wei Hu (Xavier) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> Currently, the information of Rx/Tx queues from PMD driver is not displayed
> exactly in the rxtx_config_display function. Because "ports[pid].rx_conf"
> and "ports[pid].tx_conf" maintained in testpmd application may be not the
> value actually used by PMD driver. For instance, user does not set a field,
> but PMD driver has to use the default value.

Overall the question is why testpmd maintains the config values itself?
If this is testpmd implementation problem that is no big deal, but if
our APIs are forcing applications to maintain local copies that is
something to fix I think, which may lead the differences in application
copy and more troubles as you are fixing here.

> 
> This patch fixes rxtx_config_display so that the information of Rx/Tx
> queues can be really displayed for the PMD driver that implement
> .rxq_info_get and .txq_info_get ops callback function.
> 
> Fixes: 75c530c1bd5351 ("app/testpmd: fix port configuration print")
> Fixes: d44f8a485f5d1f ("app/testpmd: enable per queue configure")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Wei Hu (Xavier) Sept. 16, 2020, 9:23 a.m. UTC | #2 
Hi, Ferruh Yigit

On 2020/9/15 0:31, Ferruh Yigit wrote:
> On 8/20/2020 2:42 AM, Wei Hu (Xavier) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> Currently, the information of Rx/Tx queues from PMD driver is not displayed
>> exactly in the rxtx_config_display function. Because "ports[pid].rx_conf"
>> and "ports[pid].tx_conf" maintained in testpmd application may be not the
>> value actually used by PMD driver. For instance, user does not set a field,
>> but PMD driver has to use the default value.
> Overall the question is why testpmd maintains the config values itself?
> If this is testpmd implementation problem that is no big deal, but if
> our APIs are forcing applications to maintain local copies that is
> something to fix I think, which may lead the differences in application
> copy and more troubles as you are fixing here.

I think it is a minor problem about displaying some information about 
testpmd.

And it has no impact on the usage of application.


Thanks,

Xavier

>> This patch fixes rxtx_config_display so that the information of Rx/Tx
>> queues can be really displayed for the PMD driver that implement
>> .rxq_info_get and .txq_info_get ops callback function.
>>
>> Fixes: 75c530c1bd5351 ("app/testpmd: fix port configuration print")
>> Fixes: d44f8a485f5d1f ("app/testpmd: enable per queue configure")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>
Ferruh Yigit Sept. 16, 2020, 3:58 p.m. UTC | #3 
On 9/16/2020 10:23 AM, Wei Hu (Xavier) wrote:
> Hi, Ferruh Yigit
> 
> On 2020/9/15 0:31, Ferruh Yigit wrote:
>> On 8/20/2020 2:42 AM, Wei Hu (Xavier) wrote:
>>> From: Huisong Li<lihuisong@huawei.com>
>>>
>>> Currently, the information of Rx/Tx queues from PMD driver is not displayed
>>> exactly in the rxtx_config_display function. Because "ports[pid].rx_conf"
>>> and "ports[pid].tx_conf" maintained in testpmd application may be not the
>>> value actually used by PMD driver. For instance, user does not set a field,
>>> but PMD driver has to use the default value.
>> Overall the question is why testpmd maintains the config values itself?
>> If this is testpmd implementation problem that is no big deal, but if
>> our APIs are forcing applications to maintain local copies that is
>> something to fix I think, which may lead the differences in application
>> copy and more troubles as you are fixing here.
> 
> I think it is a minor problem about displaying some information about 
> testpmd.
> 
> And it has no impact on the usage of application >

Agree this specific issue is minor problem, I already give the review tag.

And if the issue only concerns testpmd, still no big issue.

But I was questioning if this is sign of a design problem in DPDK APIs, 
which matters.

A simple question, why testpmd maintains the local version of the 
configuration?

And follow up can be,
Will it cause any functional problem if application always request 
configs from DPDK when needed instead of maintaining local copies? Are 
we missing APIs to do this?

> 
> Thanks,
> 
> Xavier
> 
>>> This patch fixes rxtx_config_display so that the information of Rx/Tx
>>> queues can be really displayed for the PMD driver that implement
>>> .rxq_info_get and .txq_info_get ops callback function.
>>>
>>> Fixes: 75c530c1bd5351 ("app/testpmd: fix port configuration print")
>>> Fixes: d44f8a485f5d1f ("app/testpmd: enable per queue configure")
>>> Cc:stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li<lihuisong@huawei.com>
>>> Signed-off-by: Wei Hu (Xavier)<xavier.huwei@huawei.com>
>> Reviewed-by: Ferruh Yigit<ferruh.yigit@intel.com>
>>
diff mbox series

Patch

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 6e8e05ab1..3ab24ebd2 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2089,10 +2089,17 @@  rxtx_config_display(void)
 		struct rte_eth_txconf *tx_conf = &ports[pid].tx_conf[0];
 		uint16_t *nb_rx_desc = &ports[pid].nb_rx_desc[0];
 		uint16_t *nb_tx_desc = &ports[pid].nb_tx_desc[0];
-		uint16_t nb_rx_desc_tmp;
-		uint16_t nb_tx_desc_tmp;
 		struct rte_eth_rxq_info rx_qinfo;
 		struct rte_eth_txq_info tx_qinfo;
+		uint16_t rx_free_thresh_tmp;
+		uint16_t tx_free_thresh_tmp;
+		uint16_t tx_rs_thresh_tmp;
+		uint16_t nb_rx_desc_tmp;
+		uint16_t nb_tx_desc_tmp;
+		uint64_t offloads_tmp;
+		uint8_t pthresh_tmp;
+		uint8_t hthresh_tmp;
+		uint8_t wthresh_tmp;
 		int32_t rc;
 
 		/* per port config */
@@ -2106,41 +2113,64 @@  rxtx_config_display(void)
 		/* per rx queue config only for first queue to be less verbose */
 		for (qid = 0; qid < 1; qid++) {
 			rc = rte_eth_rx_queue_info_get(pid, qid, &rx_qinfo);
-			if (rc)
+			if (rc) {
 				nb_rx_desc_tmp = nb_rx_desc[qid];
-			else
+				rx_free_thresh_tmp =
+					rx_conf[qid].rx_free_thresh;
+				pthresh_tmp = rx_conf[qid].rx_thresh.pthresh;
+				hthresh_tmp = rx_conf[qid].rx_thresh.hthresh;
+				wthresh_tmp = rx_conf[qid].rx_thresh.wthresh;
+				offloads_tmp = rx_conf[qid].offloads;
+			} else {
 				nb_rx_desc_tmp = rx_qinfo.nb_desc;
+				rx_free_thresh_tmp =
+						rx_qinfo.conf.rx_free_thresh;
+				pthresh_tmp = rx_qinfo.conf.rx_thresh.pthresh;
+				hthresh_tmp = rx_qinfo.conf.rx_thresh.hthresh;
+				wthresh_tmp = rx_qinfo.conf.rx_thresh.wthresh;
+				offloads_tmp = rx_qinfo.conf.offloads;
+			}
 
 			printf("    RX queue: %d\n", qid);
 			printf("      RX desc=%d - RX free threshold=%d\n",
-				nb_rx_desc_tmp, rx_conf[qid].rx_free_thresh);
+				nb_rx_desc_tmp, rx_free_thresh_tmp);
 			printf("      RX threshold registers: pthresh=%d hthresh=%d "
 				" wthresh=%d\n",
-				rx_conf[qid].rx_thresh.pthresh,
-				rx_conf[qid].rx_thresh.hthresh,
-				rx_conf[qid].rx_thresh.wthresh);
-			printf("      RX Offloads=0x%"PRIx64"\n",
-				rx_conf[qid].offloads);
+				pthresh_tmp, hthresh_tmp, wthresh_tmp);
+			printf("      RX Offloads=0x%"PRIx64"\n", offloads_tmp);
 		}
 
 		/* per tx queue config only for first queue to be less verbose */
 		for (qid = 0; qid < 1; qid++) {
 			rc = rte_eth_tx_queue_info_get(pid, qid, &tx_qinfo);
-			if (rc)
+			if (rc) {
 				nb_tx_desc_tmp = nb_tx_desc[qid];
-			else
+				tx_free_thresh_tmp =
+					tx_conf[qid].tx_free_thresh;
+				pthresh_tmp = tx_conf[qid].tx_thresh.pthresh;
+				hthresh_tmp = tx_conf[qid].tx_thresh.hthresh;
+				wthresh_tmp = tx_conf[qid].tx_thresh.wthresh;
+				offloads_tmp = tx_conf[qid].offloads;
+				tx_rs_thresh_tmp = tx_conf[qid].tx_rs_thresh;
+			} else {
 				nb_tx_desc_tmp = tx_qinfo.nb_desc;
+				tx_free_thresh_tmp =
+						tx_qinfo.conf.tx_free_thresh;
+				pthresh_tmp = tx_qinfo.conf.tx_thresh.pthresh;
+				hthresh_tmp = tx_qinfo.conf.tx_thresh.hthresh;
+				wthresh_tmp = tx_qinfo.conf.tx_thresh.wthresh;
+				offloads_tmp = tx_qinfo.conf.offloads;
+				tx_rs_thresh_tmp = tx_qinfo.conf.tx_rs_thresh;
+			}
 
 			printf("    TX queue: %d\n", qid);
 			printf("      TX desc=%d - TX free threshold=%d\n",
-				nb_tx_desc_tmp, tx_conf[qid].tx_free_thresh);
+				nb_tx_desc_tmp, tx_free_thresh_tmp);
 			printf("      TX threshold registers: pthresh=%d hthresh=%d "
 				" wthresh=%d\n",
-				tx_conf[qid].tx_thresh.pthresh,
-				tx_conf[qid].tx_thresh.hthresh,
-				tx_conf[qid].tx_thresh.wthresh);
+				pthresh_tmp, hthresh_tmp, wthresh_tmp);
 			printf("      TX offloads=0x%"PRIx64" - TX RS bit threshold=%d\n",
-				tx_conf[qid].offloads, tx_conf->tx_rs_thresh);
+				offloads_tmp, tx_rs_thresh_tmp);
 		}
 	}
 }