[RFC] app/testpmd: use RSS conf from software when configuring DCB

[RFC] app/testpmd: use RSS conf from software when configuring DCB

[Min Zhou]

In the testpmd command, we have to stop the port firstly before configuring
the DCB. However, some PMDs may execute a hardware reset during the port
stop, such as ixgbe. Some kind of reset operations of PMD could clear the
configurations of RSS in the hardware register. This would cause the loss
of RSS configurations that were set during the testpmd initialization. As
a result, I find that I cannot enable RSS and DCB at the same time in the
testpmd command when using Intel 82599 NIC.

Although this patch can solve the problem I encountered, is there any risk
of using rss conf from software instead of reading from the hardware
register when configuring DCB?

Signed-off-by: Min Zhou <zhoumin@loongson.cn>
---
 app/test-pmd/testpmd.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 5cb6f92523..3c382267b8 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -4247,14 +4247,12 @@ const uint16_t vlan_tags[] = {
 };
 
 static  int
-get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
+get_eth_dcb_conf(portid_t pid __rte_unused, struct rte_eth_conf *eth_conf,
 		 enum dcb_mode_enable dcb_mode,
 		 enum rte_eth_nb_tcs num_tcs,
 		 uint8_t pfc_en)
 {
 	uint8_t i;
-	int32_t rc;
-	struct rte_eth_rss_conf rss_conf;
 
 	/*
 	 * Builds up the correct configuration for dcb+vt based on the vlan tags array
@@ -4296,12 +4294,6 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
 		struct rte_eth_dcb_tx_conf *tx_conf =
 				&eth_conf->tx_adv_conf.dcb_tx_conf;
 
-		memset(&rss_conf, 0, sizeof(struct rte_eth_rss_conf));
-
-		rc = rte_eth_dev_rss_hash_conf_get(pid, &rss_conf);
-		if (rc != 0)
-			return rc;
-
 		rx_conf->nb_tcs = num_tcs;
 		tx_conf->nb_tcs = num_tcs;
 
@@ -4313,7 +4305,6 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
 		eth_conf->rxmode.mq_mode =
 				(enum rte_eth_rx_mq_mode)
 					(rx_mq_mode & RTE_ETH_MQ_RX_DCB_RSS);
-		eth_conf->rx_adv_conf.rss_conf = rss_conf;
 		eth_conf->txmode.mq_mode = RTE_ETH_MQ_TX_DCB;
 	}
 
-- 
2.31.1

Re: [RFC] app/testpmd: use RSS conf from software when configuring DCB

[zhoumin]

Kindly ping.

Any comments or suggestions will be appreciated.

Best regards
Min


On 2023/4/12 下午5:52, Min Zhou wrote:
> In the testpmd command, we have to stop the port firstly before configuring
> the DCB. However, some PMDs may execute a hardware reset during the port
> stop, such as ixgbe. Some kind of reset operations of PMD could clear the
> configurations of RSS in the hardware register. This would cause the loss
> of RSS configurations that were set during the testpmd initialization. As
> a result, I find that I cannot enable RSS and DCB at the same time in the
> testpmd command when using Intel 82599 NIC.
>
> Although this patch can solve the problem I encountered, is there any risk
> of using rss conf from software instead of reading from the hardware
> register when configuring DCB?
>
> Signed-off-by: Min Zhou <zhoumin@loongson.cn>
> ---
>   app/test-pmd/testpmd.c | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 5cb6f92523..3c382267b8 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -4247,14 +4247,12 @@ const uint16_t vlan_tags[] = {
>   };
>   
>   static  int
> -get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
> +get_eth_dcb_conf(portid_t pid __rte_unused, struct rte_eth_conf *eth_conf,
>   		 enum dcb_mode_enable dcb_mode,
>   		 enum rte_eth_nb_tcs num_tcs,
>   		 uint8_t pfc_en)
>   {
>   	uint8_t i;
> -	int32_t rc;
> -	struct rte_eth_rss_conf rss_conf;
>   
>   	/*
>   	 * Builds up the correct configuration for dcb+vt based on the vlan tags array
> @@ -4296,12 +4294,6 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
>   		struct rte_eth_dcb_tx_conf *tx_conf =
>   				&eth_conf->tx_adv_conf.dcb_tx_conf;
>   
> -		memset(&rss_conf, 0, sizeof(struct rte_eth_rss_conf));
> -
> -		rc = rte_eth_dev_rss_hash_conf_get(pid, &rss_conf);
> -		if (rc != 0)
> -			return rc;
> -
>   		rx_conf->nb_tcs = num_tcs;
>   		tx_conf->nb_tcs = num_tcs;
>   
> @@ -4313,7 +4305,6 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
>   		eth_conf->rxmode.mq_mode =
>   				(enum rte_eth_rx_mq_mode)
>   					(rx_mq_mode & RTE_ETH_MQ_RX_DCB_RSS);
> -		eth_conf->rx_adv_conf.rss_conf = rss_conf;
>   		eth_conf->txmode.mq_mode = RTE_ETH_MQ_TX_DCB;
>   	}
>   

Re: [RFC] app/testpmd: use RSS conf from software when configuring DCB

[zhoumin]

Kindly ping.

Should we solve this problem by modifying PMDs?

Is it suitable to maintain the RSS configurations for NIC by testpmd in 
order to set DCB?


On Thur, Apr 27, 2023 at 4:33PM, zhoumin wrote:
> Kindly ping.
>
> Any comments or suggestions will be appreciated.
>
> Best regards
> Min
>
>
> On 2023/4/12 下午5:52, Min Zhou wrote:
>> In the testpmd command, we have to stop the port firstly before 
>> configuring
>> the DCB. However, some PMDs may execute a hardware reset during the port
>> stop, such as ixgbe. Some kind of reset operations of PMD could clear 
>> the
>> configurations of RSS in the hardware register. This would cause the 
>> loss
>> of RSS configurations that were set during the testpmd 
>> initialization. As
>> a result, I find that I cannot enable RSS and DCB at the same time in 
>> the
>> testpmd command when using Intel 82599 NIC.
>>
>> Although this patch can solve the problem I encountered, is there any 
>> risk
>> of using rss conf from software instead of reading from the hardware
>> register when configuring DCB?
>>
>> Signed-off-by: Min Zhou <zhoumin@loongson.cn>
>> ---
>>   app/test-pmd/testpmd.c | 11 +----------
>>   1 file changed, 1 insertion(+), 10 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index 5cb6f92523..3c382267b8 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -4247,14 +4247,12 @@ const uint16_t vlan_tags[] = {
>>   };
>>     static  int
>> -get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
>> +get_eth_dcb_conf(portid_t pid __rte_unused, struct rte_eth_conf 
>> *eth_conf,
>>            enum dcb_mode_enable dcb_mode,
>>            enum rte_eth_nb_tcs num_tcs,
>>            uint8_t pfc_en)
>>   {
>>       uint8_t i;
>> -    int32_t rc;
>> -    struct rte_eth_rss_conf rss_conf;
>>         /*
>>        * Builds up the correct configuration for dcb+vt based on the 
>> vlan tags array
>> @@ -4296,12 +4294,6 @@ get_eth_dcb_conf(portid_t pid, struct 
>> rte_eth_conf *eth_conf,
>>           struct rte_eth_dcb_tx_conf *tx_conf =
>>                   &eth_conf->tx_adv_conf.dcb_tx_conf;
>>   -        memset(&rss_conf, 0, sizeof(struct rte_eth_rss_conf));
>> -
>> -        rc = rte_eth_dev_rss_hash_conf_get(pid, &rss_conf);
>> -        if (rc != 0)
>> -            return rc;
>> -
>>           rx_conf->nb_tcs = num_tcs;
>>           tx_conf->nb_tcs = num_tcs;
>>   @@ -4313,7 +4305,6 @@ get_eth_dcb_conf(portid_t pid, struct 
>> rte_eth_conf *eth_conf,
>>           eth_conf->rxmode.mq_mode =
>>                   (enum rte_eth_rx_mq_mode)
>>                       (rx_mq_mode & RTE_ETH_MQ_RX_DCB_RSS);
>> -        eth_conf->rx_adv_conf.rss_conf = rss_conf;
>>           eth_conf->txmode.mq_mode = RTE_ETH_MQ_TX_DCB;
>>       }

Best regards,

Min

Re: [RFC] app/testpmd: use RSS conf from software when configuring DCB

[Ferruh Yigit]

On 5/8/2023 2:21 AM, zhoumin wrote:
> Kindly ping.
> 

Hi Min,

Sorry for the delay.

> Should we solve this problem by modifying PMDs?
> 
> Is it suitable to maintain the RSS configurations for NIC by testpmd in
> order to set DCB?
> 
> 
> On Thur, Apr 27, 2023 at 4:33PM, zhoumin wrote:
>> Kindly ping.
>>
>> Any comments or suggestions will be appreciated.
>>
>> Best regards
>> Min
>>
>>
>> On 2023/4/12 下午5:52, Min Zhou wrote:
>>> In the testpmd command, we have to stop the port firstly before
>>> configuring
>>> the DCB. However, some PMDs may execute a hardware reset during the port
>>> stop, such as ixgbe. Some kind of reset operations of PMD could clear
>>> the
>>> configurations of RSS in the hardware register. This would cause the
>>> loss
>>> of RSS configurations that were set during the testpmd
>>> initialization. As
>>> a result, I find that I cannot enable RSS and DCB at the same time in
>>> the
>>> testpmd command when using Intel 82599 NIC.
>>>

cc'ed ixgbe maintainers too.

@Qiming, @Wenjun, are you testing DCB with RSS configuration?


Min,

Can you please give more detail on the problem, do you have in only on
device reset, or is there a generic problem on enabling DCB with RSS?

Also can you please describe how/why removing RSS configuration (this
patch) helps on the problem?

Thanks,
ferruh


>>> Although this patch can solve the problem I encountered, is there any
>>> risk
>>> of using rss conf from software instead of reading from the hardware
>>> register when configuring DCB?
>>>
>>> Signed-off-by: Min Zhou <zhoumin@loongson.cn>
>>> ---
>>>   app/test-pmd/testpmd.c | 11 +----------
>>>   1 file changed, 1 insertion(+), 10 deletions(-)
>>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>>> index 5cb6f92523..3c382267b8 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -4247,14 +4247,12 @@ const uint16_t vlan_tags[] = {
>>>   };
>>>     static  int
>>> -get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
>>> +get_eth_dcb_conf(portid_t pid __rte_unused, struct rte_eth_conf
>>> *eth_conf,
>>>            enum dcb_mode_enable dcb_mode,
>>>            enum rte_eth_nb_tcs num_tcs,
>>>            uint8_t pfc_en)
>>>   {
>>>       uint8_t i;
>>> -    int32_t rc;
>>> -    struct rte_eth_rss_conf rss_conf;
>>>         /*
>>>        * Builds up the correct configuration for dcb+vt based on the
>>> vlan tags array
>>> @@ -4296,12 +4294,6 @@ get_eth_dcb_conf(portid_t pid, struct
>>> rte_eth_conf *eth_conf,
>>>           struct rte_eth_dcb_tx_conf *tx_conf =
>>>                   &eth_conf->tx_adv_conf.dcb_tx_conf;
>>>   -        memset(&rss_conf, 0, sizeof(struct rte_eth_rss_conf));
>>> -
>>> -        rc = rte_eth_dev_rss_hash_conf_get(pid, &rss_conf);
>>> -        if (rc != 0)
>>> -            return rc;
>>> -
>>>           rx_conf->nb_tcs = num_tcs;
>>>           tx_conf->nb_tcs = num_tcs;
>>>   @@ -4313,7 +4305,6 @@ get_eth_dcb_conf(portid_t pid, struct
>>> rte_eth_conf *eth_conf,
>>>           eth_conf->rxmode.mq_mode =
>>>                   (enum rte_eth_rx_mq_mode)
>>>                       (rx_mq_mode & RTE_ETH_MQ_RX_DCB_RSS);
>>> -        eth_conf->rx_adv_conf.rss_conf = rss_conf;
>>>           eth_conf->txmode.mq_mode = RTE_ETH_MQ_TX_DCB;
>>>       }
> 
> Best regards,
> 
> Min
> 

Re: [RFC] app/testpmd: use RSS conf from software when configuring DCB

[zhoumin]

[-- Attachment #1: Type: text/plain, Size: 4724 bytes --]

Hi Ferruh,

On Wed, June 7, 2023 at 2:09AM, Ferruh Yigit wrote:
> On 5/8/2023 2:21 AM, zhoumin wrote:
>> Kindly ping.
>>
> Hi Min,
>
> Sorry for the delay.
>
>> Should we solve this problem by modifying PMDs?
>>
>> Is it suitable to maintain the RSS configurations for NIC by testpmd in
>> order to set DCB?
>>
>>
>> On Thur, Apr 27, 2023 at 4:33PM, zhoumin wrote:
>>> Kindly ping.
>>>
>>> Any comments or suggestions will be appreciated.
>>>
>>> Best regards
>>> Min
>>>
>>>
>>> On 2023/4/12 下午5:52, Min Zhou wrote:
>>>> In the testpmd command, we have to stop the port firstly before
>>>> configuring
>>>> the DCB. However, some PMDs may execute a hardware reset during the port
>>>> stop, such as ixgbe. Some kind of reset operations of PMD could clear
>>>> the
>>>> configurations of RSS in the hardware register. This would cause the
>>>> loss
>>>> of RSS configurations that were set during the testpmd
>>>> initialization. As
>>>> a result, I find that I cannot enable RSS and DCB at the same time in
>>>> the
>>>> testpmd command when using Intel 82599 NIC.
>>>>
> cc'ed ixgbe maintainers too.
OK, thanks.
>
> @Qiming, @Wenjun, are you testing DCB with RSS configuration?
>
>
> Min,
>
> Can you please give more detail on the problem, do you have in only on
> device reset, or is there a generic problem on enabling DCB with RSS?

Yes, I will give more details on the problem blow.

I think it is not a generic problem on enabling DCB with RSS. It depends 
on the implementation of "port stop" in the driver's codes. I can 
successfully enable DCB with RSS in the testpmd when it uses the i40e 
driver.

It's very easy to reproduce this problem. I will demonstrate the process 
below:

1. First we start the testpmd program with enabling  RSS using 82599 NICs:

$ ./build/app/dpdk-testpmd -l 0-6 -- -i --rss-ip --rxq=6 --txq=6 
--nb-cores=6

2. Then show the RSS configuration in the testpmd:

testpmd> show port 0 rss-hash

RSS functions:
   ipv4  ipv6  ipv6-ex

3. The above outputs are normal and then we try to enable DCB:

testpmd> port config 0 dcb vt off 4 pfc on
Please stop port 0 first

4. So we have to stop the port before enabling DCB:

testpmd> port stop 0
Stopping ports...
Checking link statuses...
ixgbe_dev_link_update_share(): Other link thread is running now!
Done

5. However, we will find that the RSS was disabled after stopping the port:

testpmd> show port 0 rss-hash
RSS disabled


RSS is enabled in the MRQC register of 82599. The MRQC register supplied 
in section 8.2.3.7.12 of the Intel 82599 datasheet.

I added some outputs before and after port reset in the port stop 
process of ixgbe driver as follows:

diff --git a/drivers/net/ixgbe/base/ixgbe_82599.c 
b/drivers/net/ixgbe/base/ixgbe_82599.c
index c6e8b7e976..b13431a43f 100644
--- a/drivers/net/ixgbe/base/ixgbe_82599.c
+++ b/drivers/net/ixgbe/base/ixgbe_82599.c
@@ -1053,6 +1053,7 @@ s32 ixgbe_reset_hw_82599(struct ixgbe_hw *hw)
                 if (link_up)
                         ctrl = IXGBE_CTRL_RST;
         }
+       printf("%s: MRQC = 0x%x\n", __func__, IXGBE_READ_REG(hw, 
IXGBE_MRQC));

         ctrl |= IXGBE_READ_REG(hw, IXGBE_CTRL);
         IXGBE_WRITE_REG(hw, IXGBE_CTRL, ctrl);
@@ -1065,6 +1066,7 @@ s32 ixgbe_reset_hw_82599(struct ixgbe_hw *hw)
                 if (!(ctrl & IXGBE_CTRL_RST_MASK))
                         break;
         }
+       printf("%s: MRQC = 0x%x\n", __func__, IXGBE_READ_REG(hw, 
IXGBE_MRQC));

         if (ctrl & IXGBE_CTRL_RST_MASK) {
                 status = IXGBE_ERR_RESET_FAILED;

We could find that the RSS configurations will be cleared after port 
reset as follows:

testpmd> port stop 0
Stopping ports...
ixgbe_reset_hw_82599: MRQC = 0x1a0001
ixgbe_reset_hw_82599: MRQC = 0x0
Checking link statuses...
ixgbe_dev_link_update_share(): Other link thread is running now!
Done

However, I have not find the exact descriptions about this behavior in 
the 82599 datasheet. Could @Qiming, @Wenjun give me some indications ?

So, if we want to get the RSS configurations by reading from the 
register after the port stop, we will get nothing.

> Also can you please describe how/why removing RSS configuration (this
> patch) helps on the problem?

As above said, I'm concerned whether we could reuse the RSS 
configurations recorded in the testpmd if the port reset cannot be 
avoided in the driver.

So, this patch wants to reuse the RSS configurations maintained by 
testpmd instead of reading from the register.

I'm not sure if this is a reasonable method to solve this problem.

> Thanks,
> ferruh

[...]

Best regards,

Min



[-- Attachment #2: Type: text/html, Size: 7541 bytes --]

Re: [RFC] app/testpmd: use RSS conf from software when configuring DCB

[Stephen Hemminger]

On Wed, 12 Apr 2023 17:52:39 +0800
Min Zhou <zhoumin@loongson.cn> wrote:

> In the testpmd command, we have to stop the port firstly before configuring
> the DCB. However, some PMDs may execute a hardware reset during the port
> stop, such as ixgbe. Some kind of reset operations of PMD could clear the
> configurations of RSS in the hardware register. This would cause the loss
> of RSS configurations that were set during the testpmd initialization. As
> a result, I find that I cannot enable RSS and DCB at the same time in the
> testpmd command when using Intel 82599 NIC.
> 
> Although this patch can solve the problem I encountered, is there any risk
> of using rss conf from software instead of reading from the hardware
> register when configuring DCB?
> 
> Signed-off-by: Min Zhou <zhoumin@loongson.cn>
> ---
>  app/test-pmd/testpmd.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 5cb6f92523..3c382267b8 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -4247,14 +4247,12 @@ const uint16_t vlan_tags[] = {
>  };
>  
>  static  int
> -get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
> +get_eth_dcb_conf(portid_t pid __rte_unused, struct rte_eth_conf *eth_conf,
>  		 enum dcb_mode_enable dcb_mode,
>  		 enum rte_eth_nb_tcs num_tcs,
>  		 uint8_t pfc_en)
>  {
>  	uint8_t i;
> -	int32_t rc;
> -	struct rte_eth_rss_conf rss_conf;
>  

Sorry for the late review, but almost uses DCB and fewer still with DPDK;
so not surprised that there would be issues like this.

If you change an internal function like get_eth_dcb_conf to remove parameters
then do not mark those parameters as unused, instead remove them from the function
and the caller. Plus the function now always returns 0 so it can be reset as void.
first place.


Suggest the following (untested):

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index b1401136e4..5eaac752c6 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -4291,15 +4291,11 @@ const uint16_t vlan_tags[] = {
 		24, 25, 26, 27, 28, 29, 30, 31
 };
 
-static  int
-get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
-		 enum dcb_mode_enable dcb_mode,
-		 enum rte_eth_nb_tcs num_tcs,
-		 uint8_t pfc_en)
+static void
+get_eth_dcb_conf(struct rte_eth_conf *eth_conf, enum dcb_mode_enable dcb_mode,
+		 enum rte_eth_nb_tcs num_tcs, uint8_t pfc_en)
 {
 	uint8_t i;
-	int32_t rc;
-	struct rte_eth_rss_conf rss_conf;
 
 	/*
 	 * Builds up the correct configuration for dcb+vt based on the vlan tags array
@@ -4341,12 +4337,6 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
 		struct rte_eth_dcb_tx_conf *tx_conf =
 				&eth_conf->tx_adv_conf.dcb_tx_conf;
 
-		memset(&rss_conf, 0, sizeof(struct rte_eth_rss_conf));
-
-		rc = rte_eth_dev_rss_hash_conf_get(pid, &rss_conf);
-		if (rc != 0)
-			return rc;
-
 		rx_conf->nb_tcs = num_tcs;
 		tx_conf->nb_tcs = num_tcs;
 
@@ -4358,7 +4348,6 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
 		eth_conf->rxmode.mq_mode =
 				(enum rte_eth_rx_mq_mode)
 					(rx_mq_mode & RTE_ETH_MQ_RX_DCB_RSS);
-		eth_conf->rx_adv_conf.rss_conf = rss_conf;
 		eth_conf->txmode.mq_mode = RTE_ETH_MQ_TX_DCB;
 	}
 
@@ -4367,8 +4356,6 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
 				RTE_ETH_DCB_PG_SUPPORT | RTE_ETH_DCB_PFC_SUPPORT;
 	else
 		eth_conf->dcb_capability_en = RTE_ETH_DCB_PG_SUPPORT;
-
-	return 0;
 }
 
 int
@@ -4391,10 +4378,9 @@ init_port_dcb_config(portid_t pid,
 	/* retain the original device configuration. */
 	memcpy(&port_conf, &rte_port->dev_conf, sizeof(struct rte_eth_conf));
 
-	/*set configuration of DCB in vt mode and DCB in non-vt mode*/
-	retval = get_eth_dcb_conf(pid, &port_conf, dcb_mode, num_tcs, pfc_en);
-	if (retval < 0)
-		return retval;
+	/* set configuration of DCB in vt mode and DCB in non-vt mode */
+	get_eth_dcb_conf(&port_conf, dcb_mode, num_tcs, pfc_en);
+
 	port_conf.rxmode.offloads |= RTE_ETH_RX_OFFLOAD_VLAN_FILTER;
 	/* remove RSS HASH offload for DCB in vt mode */
 	if (port_conf.rxmode.mq_mode == RTE_ETH_MQ_RX_VMDQ_DCB) {

Re: [RFC] app/testpmd: use RSS conf from software when configuring DCB

[zhoumin]

Hi Stephen,

Sorry for the delay.

On Thur, Oct 10 at 2024 6:30AM, Stephen Hemminger wrote:
> On Wed, 12 Apr 2023 17:52:39 +0800
> Min Zhou <zhoumin@loongson.cn> wrote:
>
>> In the testpmd command, we have to stop the port firstly before configuring
>> the DCB. However, some PMDs may execute a hardware reset during the port
>> stop, such as ixgbe. Some kind of reset operations of PMD could clear the
>> configurations of RSS in the hardware register. This would cause the loss
>> of RSS configurations that were set during the testpmd initialization. As
>> a result, I find that I cannot enable RSS and DCB at the same time in the
>> testpmd command when using Intel 82599 NIC.
>>
>> Although this patch can solve the problem I encountered, is there any risk
>> of using rss conf from software instead of reading from the hardware
>> register when configuring DCB?
>>
>> Signed-off-by: Min Zhou <zhoumin@loongson.cn>
>> ---
>>   app/test-pmd/testpmd.c | 11 +----------
>>   1 file changed, 1 insertion(+), 10 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index 5cb6f92523..3c382267b8 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -4247,14 +4247,12 @@ const uint16_t vlan_tags[] = {
>>   };
>>   
>>   static  int
>> -get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
>> +get_eth_dcb_conf(portid_t pid __rte_unused, struct rte_eth_conf *eth_conf,
>>   		 enum dcb_mode_enable dcb_mode,
>>   		 enum rte_eth_nb_tcs num_tcs,
>>   		 uint8_t pfc_en)
>>   {
>>   	uint8_t i;
>> -	int32_t rc;
>> -	struct rte_eth_rss_conf rss_conf;
>>   
> Sorry for the late review, but almost uses DCB and fewer still with DPDK;
> so not surprised that there would be issues like this.
>
> If you change an internal function like get_eth_dcb_conf to remove parameters
> then do not mark those parameters as unused, instead remove them from the function
> and the caller. Plus the function now always returns 0 so it can be reset as void.
> first place.

Yes, I agree with your point. Thanks for providing the suggested codes. 
I have tested this change and it can solve the problem. I will send the 
V2 version.

Best Regards,

Min Zhou

>
> Suggest the following (untested):
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index b1401136e4..5eaac752c6 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -4291,15 +4291,11 @@ const uint16_t vlan_tags[] = {
>   		24, 25, 26, 27, 28, 29, 30, 31
>   };
>   
> -static  int
> -get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
> -		 enum dcb_mode_enable dcb_mode,
> -		 enum rte_eth_nb_tcs num_tcs,
> -		 uint8_t pfc_en)
> +static void
> +get_eth_dcb_conf(struct rte_eth_conf *eth_conf, enum dcb_mode_enable dcb_mode,
> +		 enum rte_eth_nb_tcs num_tcs, uint8_t pfc_en)
>   {
>   	uint8_t i;
> -	int32_t rc;
> -	struct rte_eth_rss_conf rss_conf;
>   
>   	/*
>   	 * Builds up the correct configuration for dcb+vt based on the vlan tags array
> @@ -4341,12 +4337,6 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
>   		struct rte_eth_dcb_tx_conf *tx_conf =
>   				&eth_conf->tx_adv_conf.dcb_tx_conf;
>   
> -		memset(&rss_conf, 0, sizeof(struct rte_eth_rss_conf));
> -
> -		rc = rte_eth_dev_rss_hash_conf_get(pid, &rss_conf);
> -		if (rc != 0)
> -			return rc;
> -
>   		rx_conf->nb_tcs = num_tcs;
>   		tx_conf->nb_tcs = num_tcs;
>   
> @@ -4358,7 +4348,6 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
>   		eth_conf->rxmode.mq_mode =
>   				(enum rte_eth_rx_mq_mode)
>   					(rx_mq_mode & RTE_ETH_MQ_RX_DCB_RSS);
> -		eth_conf->rx_adv_conf.rss_conf = rss_conf;
>   		eth_conf->txmode.mq_mode = RTE_ETH_MQ_TX_DCB;
>   	}
>   
> @@ -4367,8 +4356,6 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
>   				RTE_ETH_DCB_PG_SUPPORT | RTE_ETH_DCB_PFC_SUPPORT;
>   	else
>   		eth_conf->dcb_capability_en = RTE_ETH_DCB_PG_SUPPORT;
> -
> -	return 0;
>   }
>   
>   int
> @@ -4391,10 +4378,9 @@ init_port_dcb_config(portid_t pid,
>   	/* retain the original device configuration. */
>   	memcpy(&port_conf, &rte_port->dev_conf, sizeof(struct rte_eth_conf));
>   
> -	/*set configuration of DCB in vt mode and DCB in non-vt mode*/
> -	retval = get_eth_dcb_conf(pid, &port_conf, dcb_mode, num_tcs, pfc_en);
> -	if (retval < 0)
> -		return retval;
> +	/* set configuration of DCB in vt mode and DCB in non-vt mode */
> +	get_eth_dcb_conf(&port_conf, dcb_mode, num_tcs, pfc_en);
> +
>   	port_conf.rxmode.offloads |= RTE_ETH_RX_OFFLOAD_VLAN_FILTER;
>   	/* remove RSS HASH offload for DCB in vt mode */
>   	if (port_conf.rxmode.mq_mode == RTE_ETH_MQ_RX_VMDQ_DCB) {

[v2 1/1] app/testpmd: use RSS conf from software when configuring DCB

[Min Zhou]

In the testpmd command, we have to stop the port firstly before configuring
the DCB. However, some PMDs may execute a hardware reset during the port
stop, such as ixgbe. Some kind of reset operations of PMD could clear the
configurations of RSS in the hardware register. This would cause the loss
of RSS configurations that were set during the testpmd initialization. As
a result, I find that I cannot enable RSS and DCB at the same time in the
testpmd command when using Intel 82599 NIC.

The patch uses rss conf from software instead of reading from the hardware
register when configuring DCB.

Signed-off-by: Min Zhou <zhoumin@loongson.cn>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

---
v2:
Make internal function get_eth_dcb_conf return void and remove the unused
parameter port_id.
---

 app/test-pmd/testpmd.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index f487769578..ac654048df 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -4080,15 +4080,11 @@ const uint16_t vlan_tags[] = {
 		24, 25, 26, 27, 28, 29, 30, 31
 };
 
-static  int
-get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
-		 enum dcb_mode_enable dcb_mode,
-		 enum rte_eth_nb_tcs num_tcs,
-		 uint8_t pfc_en)
+static void
+get_eth_dcb_conf(struct rte_eth_conf *eth_conf, enum dcb_mode_enable dcb_mode,
+		 enum rte_eth_nb_tcs num_tcs, uint8_t pfc_en)
 {
 	uint8_t i;
-	int32_t rc;
-	struct rte_eth_rss_conf rss_conf;
 
 	/*
 	 * Builds up the correct configuration for dcb+vt based on the vlan tags array
@@ -4130,12 +4126,6 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
 		struct rte_eth_dcb_tx_conf *tx_conf =
 				&eth_conf->tx_adv_conf.dcb_tx_conf;
 
-		memset(&rss_conf, 0, sizeof(struct rte_eth_rss_conf));
-
-		rc = rte_eth_dev_rss_hash_conf_get(pid, &rss_conf);
-		if (rc != 0)
-			return rc;
-
 		rx_conf->nb_tcs = num_tcs;
 		tx_conf->nb_tcs = num_tcs;
 
@@ -4147,7 +4137,6 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
 		eth_conf->rxmode.mq_mode =
 				(enum rte_eth_rx_mq_mode)
 					(rx_mq_mode & RTE_ETH_MQ_RX_DCB_RSS);
-		eth_conf->rx_adv_conf.rss_conf = rss_conf;
 		eth_conf->txmode.mq_mode = RTE_ETH_MQ_TX_DCB;
 	}
 
@@ -4156,8 +4145,6 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf,
 				RTE_ETH_DCB_PG_SUPPORT | RTE_ETH_DCB_PFC_SUPPORT;
 	else
 		eth_conf->dcb_capability_en = RTE_ETH_DCB_PG_SUPPORT;
-
-	return 0;
 }
 
 int
@@ -4180,10 +4167,9 @@ init_port_dcb_config(portid_t pid,
 	/* retain the original device configuration. */
 	memcpy(&port_conf, &rte_port->dev_conf, sizeof(struct rte_eth_conf));
 
-	/*set configuration of DCB in vt mode and DCB in non-vt mode*/
-	retval = get_eth_dcb_conf(pid, &port_conf, dcb_mode, num_tcs, pfc_en);
-	if (retval < 0)
-		return retval;
+	/* set configuration of DCB in vt mode and DCB in non-vt mode */
+	get_eth_dcb_conf(&port_conf, dcb_mode, num_tcs, pfc_en);
+
 	port_conf.rxmode.offloads |= RTE_ETH_RX_OFFLOAD_VLAN_FILTER;
 	/* remove RSS HASH offload for DCB in vt mode */
 	if (port_conf.rxmode.mq_mode == RTE_ETH_MQ_RX_VMDQ_DCB) {
-- 
2.20.1