DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/hns3: add runtime config to set MBX limit time
@ 2021-08-30  3:48 Min Hu (Connor)
  2021-09-09 13:20 ` Ferruh Yigit
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Min Hu (Connor) @ 2021-08-30  3:48 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, andrew.rybchenko

From: Chengchang Tang <tangchengchang@huawei.com>

Current, the max waiting time for MBX response is 500ms, but in
some scenarios, it is not enough. Since it depends on the response
of the kernel mode driver, and its response time is related to the
scheduling of the system. In this special scenario, most of the
cores are isolated, and only a few cores are used for system
scheduling. When a large number of services are started, the
scheduling of the system will be very busy, and the reply of the
mbx message will time out, which will cause our PMD initialization
to fail.

This patch add a runtime config to set the max wait time. For the
above scenes, users can adjust the waiting time to a suitable value
by themselves.

Fixes: 463e748964f5 ("net/hns3: support mailbox")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/net/hns3/hns3_ethdev.c    | 26 +++++++++++++++++++++++++-
 drivers/net/hns3/hns3_ethdev.h    |  3 +++
 drivers/net/hns3/hns3_ethdev_vf.c |  3 ++-
 drivers/net/hns3/hns3_mbx.c       | 10 +++++++---
 drivers/net/hns3/hns3_mbx.h       |  1 +
 5 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
index 7d37004..4c1a6f1 100644
--- a/drivers/net/hns3/hns3_ethdev.c
+++ b/drivers/net/hns3/hns3_ethdev.c
@@ -7357,9 +7357,24 @@ hns3_parse_dev_caps_mask(const char *key, const char *value, void *extra_args)
 	return 0;
 }
 
+static int
+hns3_parse_mbx_time_limit(const char *key, const char *value, void *extra_args)
+{
+	uint32_t val;
+
+	RTE_SET_USED(key);
+
+	val = strtoul(value, NULL, 10);
+	if (val > HNS3_MBX_DEF_TIME_LIMIT_MS && val <= UINT16_MAX)
+		*(uint16_t *)extra_args = val;
+
+	return 0;
+}
+
 void
 hns3_parse_devargs(struct rte_eth_dev *dev)
 {
+	uint16_t mbx_time_limit_ms = HNS3_MBX_DEF_TIME_LIMIT_MS;
 	struct hns3_adapter *hns = dev->data->dev_private;
 	uint32_t rx_func_hint = HNS3_IO_FUNC_HINT_NONE;
 	uint32_t tx_func_hint = HNS3_IO_FUNC_HINT_NONE;
@@ -7380,6 +7395,9 @@ hns3_parse_devargs(struct rte_eth_dev *dev)
 			   &hns3_parse_io_hint_func, &tx_func_hint);
 	(void)rte_kvargs_process(kvlist, HNS3_DEVARG_DEV_CAPS_MASK,
 			   &hns3_parse_dev_caps_mask, &dev_caps_mask);
+	(void)rte_kvargs_process(kvlist, HNS3_DEVARG_MBX_TIME_LIMIT_MS,
+			   &hns3_parse_mbx_time_limit, &mbx_time_limit_ms);
+
 	rte_kvargs_free(kvlist);
 
 	if (rx_func_hint != HNS3_IO_FUNC_HINT_NONE)
@@ -7395,6 +7413,11 @@ hns3_parse_devargs(struct rte_eth_dev *dev)
 		hns3_warn(hw, "parsed %s = 0x%" PRIx64 ".",
 			  HNS3_DEVARG_DEV_CAPS_MASK, dev_caps_mask);
 	hns->dev_caps_mask = dev_caps_mask;
+
+	if (mbx_time_limit_ms != HNS3_MBX_DEF_TIME_LIMIT_MS)
+		hns3_warn(hw, "parsed %s = %u.", HNS3_DEVARG_MBX_TIME_LIMIT_MS,
+				mbx_time_limit_ms);
+	hns->mbx_time_limit_ms = mbx_time_limit_ms;
 }
 
 static const struct eth_dev_ops hns3_eth_dev_ops = {
@@ -7651,6 +7674,7 @@ RTE_PMD_REGISTER_KMOD_DEP(net_hns3, "* igb_uio | vfio-pci");
 RTE_PMD_REGISTER_PARAM_STRING(net_hns3,
 		HNS3_DEVARG_RX_FUNC_HINT "=vec|sve|simple|common "
 		HNS3_DEVARG_TX_FUNC_HINT "=vec|sve|simple|common "
-		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> ");
+		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> "
+		HNS3_DEVARG_MBX_TIME_LIMIT_MS "=<uint16> ");
 RTE_LOG_REGISTER_SUFFIX(hns3_logtype_init, init, NOTICE);
 RTE_LOG_REGISTER_SUFFIX(hns3_logtype_driver, driver, NOTICE);
diff --git a/drivers/net/hns3/hns3_ethdev.h b/drivers/net/hns3/hns3_ethdev.h
index 0e4e426..6476ad5 100644
--- a/drivers/net/hns3/hns3_ethdev.h
+++ b/drivers/net/hns3/hns3_ethdev.h
@@ -852,6 +852,7 @@ struct hns3_adapter {
 	uint32_t tx_func_hint;
 
 	uint64_t dev_caps_mask;
+	uint16_t mbx_time_limit_ms; /* wait time for mbx message */
 
 	struct hns3_ptype_table ptype_tbl __rte_cache_aligned;
 };
@@ -869,6 +870,8 @@ enum {
 
 #define HNS3_DEVARG_DEV_CAPS_MASK	"dev_caps_mask"
 
+#define HNS3_DEVARG_MBX_TIME_LIMIT_MS	"mbx_time_limit_ms"
+
 enum {
 	HNS3_DEV_SUPPORT_DCB_B,
 	HNS3_DEV_SUPPORT_COPPER_B,
diff --git a/drivers/net/hns3/hns3_ethdev_vf.c b/drivers/net/hns3/hns3_ethdev_vf.c
index 8d9b797..7ed9cb2 100644
--- a/drivers/net/hns3/hns3_ethdev_vf.c
+++ b/drivers/net/hns3/hns3_ethdev_vf.c
@@ -3115,4 +3115,5 @@ RTE_PMD_REGISTER_KMOD_DEP(net_hns3_vf, "* igb_uio | vfio-pci");
 RTE_PMD_REGISTER_PARAM_STRING(net_hns3_vf,
 		HNS3_DEVARG_RX_FUNC_HINT "=vec|sve|simple|common "
 		HNS3_DEVARG_TX_FUNC_HINT "=vec|sve|simple|common "
-		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> ");
+		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> "
+		HNS3_DEVARG_MBX_TIME_LIMIT_MS "=<uint16_t> ");
diff --git a/drivers/net/hns3/hns3_mbx.c b/drivers/net/hns3/hns3_mbx.c
index f36cb10..a1556f7 100644
--- a/drivers/net/hns3/hns3_mbx.c
+++ b/drivers/net/hns3/hns3_mbx.c
@@ -61,8 +61,9 @@ static int
 hns3_get_mbx_resp(struct hns3_hw *hw, uint16_t code, uint16_t subcode,
 		  uint8_t *resp_data, uint16_t resp_len)
 {
-#define HNS3_MAX_RETRY_US	500000
 #define HNS3_WAIT_RESP_US	100
+#define US_PER_MS		1000
+	uint32_t mbx_time_limit = HNS3_MBX_DEF_TIME_LIMIT_MS * US_PER_MS;
 	struct hns3_adapter *hns = HNS3_DEV_HW_TO_ADAPTER(hw);
 	struct hns3_mbx_resp_status *mbx_resp;
 	uint32_t wait_time = 0;
@@ -74,7 +75,10 @@ hns3_get_mbx_resp(struct hns3_hw *hw, uint16_t code, uint16_t subcode,
 		return -EINVAL;
 	}
 
-	while (wait_time < HNS3_MAX_RETRY_US) {
+	if (hns->mbx_time_limit_ms > HNS3_MBX_DEF_TIME_LIMIT_MS)
+		mbx_time_limit = (uint32_t)hns->mbx_time_limit_ms * US_PER_MS;
+
+	while (wait_time < mbx_time_limit) {
 		if (__atomic_load_n(&hw->reset.disable_cmd, __ATOMIC_RELAXED)) {
 			hns3_err(hw, "Don't wait for mbx respone because of "
 				 "disable_cmd");
@@ -103,7 +107,7 @@ hns3_get_mbx_resp(struct hns3_hw *hw, uint16_t code, uint16_t subcode,
 		wait_time += HNS3_WAIT_RESP_US;
 	}
 	hw->mbx_resp.req_msg_data = 0;
-	if (wait_time >= HNS3_MAX_RETRY_US) {
+	if (wait_time >= mbx_time_limit) {
 		hns3_mbx_proc_timeout(hw, code, subcode);
 		return -ETIME;
 	}
diff --git a/drivers/net/hns3/hns3_mbx.h b/drivers/net/hns3/hns3_mbx.h
index f868e33..d637bd2 100644
--- a/drivers/net/hns3/hns3_mbx.h
+++ b/drivers/net/hns3/hns3_mbx.h
@@ -87,6 +87,7 @@ enum hns3_mbx_link_fail_subcode {
 
 #define HNS3_MBX_MAX_MSG_SIZE	16
 #define HNS3_MBX_MAX_RESP_DATA_SIZE	8
+#define HNS3_MBX_DEF_TIME_LIMIT_MS	500
 
 enum {
 	HNS3_MBX_RESP_MATCHING_SCHEME_OF_ORIGINAL = 0,
-- 
2.7.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/hns3: add runtime config to set MBX limit time
  2021-08-30  3:48 [dpdk-dev] [PATCH] net/hns3: add runtime config to set MBX limit time Min Hu (Connor)
@ 2021-09-09 13:20 ` Ferruh Yigit
  2021-10-21  2:26   ` Min Hu (Connor)
  2021-10-21  2:22 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
  2021-10-22  1:38 ` [dpdk-dev] [PATCH v3] " Min Hu (Connor)
  2 siblings, 1 reply; 8+ messages in thread
From: Ferruh Yigit @ 2021-09-09 13:20 UTC (permalink / raw)
  To: Min Hu (Connor), dev; +Cc: andrew.rybchenko

On 8/30/2021 4:48 AM, Min Hu (Connor) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> Current, the max waiting time for MBX response is 500ms, but in
> some scenarios, it is not enough. Since it depends on the response
> of the kernel mode driver, and its response time is related to the
> scheduling of the system. In this special scenario, most of the
> cores are isolated, and only a few cores are used for system
> scheduling. When a large number of services are started, the
> scheduling of the system will be very busy, and the reply of the
> mbx message will time out, which will cause our PMD initialization
> to fail.
> 
> This patch add a runtime config to set the max wait time. For the
> above scenes, users can adjust the waiting time to a suitable value
> by themselves.
> 
> Fixes: 463e748964f5 ("net/hns3: support mailbox")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  drivers/net/hns3/hns3_ethdev.c    | 26 +++++++++++++++++++++++++-
>  drivers/net/hns3/hns3_ethdev.h    |  3 +++
>  drivers/net/hns3/hns3_ethdev_vf.c |  3 ++-
>  drivers/net/hns3/hns3_mbx.c       | 10 +++++++---
>  drivers/net/hns3/hns3_mbx.h       |  1 +
>  5 files changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
> index 7d37004..4c1a6f1 100644
> --- a/drivers/net/hns3/hns3_ethdev.c
> +++ b/drivers/net/hns3/hns3_ethdev.c
> @@ -7357,9 +7357,24 @@ hns3_parse_dev_caps_mask(const char *key, const char *value, void *extra_args)
>  	return 0;
>  }
>  
> +static int
> +hns3_parse_mbx_time_limit(const char *key, const char *value, void *extra_args)
> +{
> +	uint32_t val;
> +
> +	RTE_SET_USED(key);
> +
> +	val = strtoul(value, NULL, 10);
> +	if (val > HNS3_MBX_DEF_TIME_LIMIT_MS && val <= UINT16_MAX)

This check adds a restriction that timeout can't be set less the 500ms but this
restriction is not documented in the help string, comment or commit log etc...
Can you either remove the restriction or document it?

> +		*(uint16_t *)extra_args = val;
> +
> +	return 0;
> +}
> +
>  void
>  hns3_parse_devargs(struct rte_eth_dev *dev)
>  {
> +	uint16_t mbx_time_limit_ms = HNS3_MBX_DEF_TIME_LIMIT_MS;
>  	struct hns3_adapter *hns = dev->data->dev_private;
>  	uint32_t rx_func_hint = HNS3_IO_FUNC_HINT_NONE;
>  	uint32_t tx_func_hint = HNS3_IO_FUNC_HINT_NONE;
> @@ -7380,6 +7395,9 @@ hns3_parse_devargs(struct rte_eth_dev *dev)
>  			   &hns3_parse_io_hint_func, &tx_func_hint);
>  	(void)rte_kvargs_process(kvlist, HNS3_DEVARG_DEV_CAPS_MASK,
>  			   &hns3_parse_dev_caps_mask, &dev_caps_mask);
> +	(void)rte_kvargs_process(kvlist, HNS3_DEVARG_MBX_TIME_LIMIT_MS,
> +			   &hns3_parse_mbx_time_limit, &mbx_time_limit_ms);
> +
>  	rte_kvargs_free(kvlist);
>  
>  	if (rx_func_hint != HNS3_IO_FUNC_HINT_NONE)
> @@ -7395,6 +7413,11 @@ hns3_parse_devargs(struct rte_eth_dev *dev)
>  		hns3_warn(hw, "parsed %s = 0x%" PRIx64 ".",
>  			  HNS3_DEVARG_DEV_CAPS_MASK, dev_caps_mask);
>  	hns->dev_caps_mask = dev_caps_mask;
> +
> +	if (mbx_time_limit_ms != HNS3_MBX_DEF_TIME_LIMIT_MS)
> +		hns3_warn(hw, "parsed %s = %u.", HNS3_DEVARG_MBX_TIME_LIMIT_MS,
> +				mbx_time_limit_ms);
> +	hns->mbx_time_limit_ms = mbx_time_limit_ms;
>  }
>  
>  static const struct eth_dev_ops hns3_eth_dev_ops = {
> @@ -7651,6 +7674,7 @@ RTE_PMD_REGISTER_KMOD_DEP(net_hns3, "* igb_uio | vfio-pci");
>  RTE_PMD_REGISTER_PARAM_STRING(net_hns3,
>  		HNS3_DEVARG_RX_FUNC_HINT "=vec|sve|simple|common "
>  		HNS3_DEVARG_TX_FUNC_HINT "=vec|sve|simple|common "
> -		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> ");
> +		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> "
> +		HNS3_DEVARG_MBX_TIME_LIMIT_MS "=<uint16> ");
>  RTE_LOG_REGISTER_SUFFIX(hns3_logtype_init, init, NOTICE);
>  RTE_LOG_REGISTER_SUFFIX(hns3_logtype_driver, driver, NOTICE);
> diff --git a/drivers/net/hns3/hns3_ethdev.h b/drivers/net/hns3/hns3_ethdev.h
> index 0e4e426..6476ad5 100644
> --- a/drivers/net/hns3/hns3_ethdev.h
> +++ b/drivers/net/hns3/hns3_ethdev.h
> @@ -852,6 +852,7 @@ struct hns3_adapter {
>  	uint32_t tx_func_hint;
>  
>  	uint64_t dev_caps_mask;
> +	uint16_t mbx_time_limit_ms; /* wait time for mbx message */
>  
>  	struct hns3_ptype_table ptype_tbl __rte_cache_aligned;
>  };
> @@ -869,6 +870,8 @@ enum {
>  
>  #define HNS3_DEVARG_DEV_CAPS_MASK	"dev_caps_mask"
>  
> +#define HNS3_DEVARG_MBX_TIME_LIMIT_MS	"mbx_time_limit_ms"
> +
>  enum {
>  	HNS3_DEV_SUPPORT_DCB_B,
>  	HNS3_DEV_SUPPORT_COPPER_B,
> diff --git a/drivers/net/hns3/hns3_ethdev_vf.c b/drivers/net/hns3/hns3_ethdev_vf.c
> index 8d9b797..7ed9cb2 100644
> --- a/drivers/net/hns3/hns3_ethdev_vf.c
> +++ b/drivers/net/hns3/hns3_ethdev_vf.c
> @@ -3115,4 +3115,5 @@ RTE_PMD_REGISTER_KMOD_DEP(net_hns3_vf, "* igb_uio | vfio-pci");
>  RTE_PMD_REGISTER_PARAM_STRING(net_hns3_vf,
>  		HNS3_DEVARG_RX_FUNC_HINT "=vec|sve|simple|common "
>  		HNS3_DEVARG_TX_FUNC_HINT "=vec|sve|simple|common "
> -		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> ");
> +		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> "
> +		HNS3_DEVARG_MBX_TIME_LIMIT_MS "=<uint16_t> ");
> diff --git a/drivers/net/hns3/hns3_mbx.c b/drivers/net/hns3/hns3_mbx.c
> index f36cb10..a1556f7 100644
> --- a/drivers/net/hns3/hns3_mbx.c
> +++ b/drivers/net/hns3/hns3_mbx.c
> @@ -61,8 +61,9 @@ static int
>  hns3_get_mbx_resp(struct hns3_hw *hw, uint16_t code, uint16_t subcode,
>  		  uint8_t *resp_data, uint16_t resp_len)
>  {
> -#define HNS3_MAX_RETRY_US	500000
>  #define HNS3_WAIT_RESP_US	100
> +#define US_PER_MS		1000
> +	uint32_t mbx_time_limit = HNS3_MBX_DEF_TIME_LIMIT_MS * US_PER_MS;
>  	struct hns3_adapter *hns = HNS3_DEV_HW_TO_ADAPTER(hw);
>  	struct hns3_mbx_resp_status *mbx_resp;
>  	uint32_t wait_time = 0;
> @@ -74,7 +75,10 @@ hns3_get_mbx_resp(struct hns3_hw *hw, uint16_t code, uint16_t subcode,
>  		return -EINVAL;
>  	}
>  
> -	while (wait_time < HNS3_MAX_RETRY_US) {
> +	if (hns->mbx_time_limit_ms > HNS3_MBX_DEF_TIME_LIMIT_MS)
> +		mbx_time_limit = (uint32_t)hns->mbx_time_limit_ms * US_PER_MS;
> +

Why calculating the 'mbx_time_limit' twice in this function, isn't
'hns->mbx_time_limit_ms' has already either default value or user provided
value, why not just keep this calculation even with removing above check?

> +	while (wait_time < mbx_time_limit) {
>  		if (__atomic_load_n(&hw->reset.disable_cmd, __ATOMIC_RELAXED)) {
>  			hns3_err(hw, "Don't wait for mbx respone because of "
>  				 "disable_cmd");
> @@ -103,7 +107,7 @@ hns3_get_mbx_resp(struct hns3_hw *hw, uint16_t code, uint16_t subcode,
>  		wait_time += HNS3_WAIT_RESP_US;
>  	}
>  	hw->mbx_resp.req_msg_data = 0;
> -	if (wait_time >= HNS3_MAX_RETRY_US) {
> +	if (wait_time >= mbx_time_limit) {
>  		hns3_mbx_proc_timeout(hw, code, subcode);
>  		return -ETIME;
>  	}
> diff --git a/drivers/net/hns3/hns3_mbx.h b/drivers/net/hns3/hns3_mbx.h
> index f868e33..d637bd2 100644
> --- a/drivers/net/hns3/hns3_mbx.h
> +++ b/drivers/net/hns3/hns3_mbx.h
> @@ -87,6 +87,7 @@ enum hns3_mbx_link_fail_subcode {
>  
>  #define HNS3_MBX_MAX_MSG_SIZE	16
>  #define HNS3_MBX_MAX_RESP_DATA_SIZE	8
> +#define HNS3_MBX_DEF_TIME_LIMIT_MS	500
>  
>  enum {
>  	HNS3_MBX_RESP_MATCHING_SCHEME_OF_ORIGINAL = 0,
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH v2] net/hns3: add runtime config to set MBX limit time
  2021-08-30  3:48 [dpdk-dev] [PATCH] net/hns3: add runtime config to set MBX limit time Min Hu (Connor)
  2021-09-09 13:20 ` Ferruh Yigit
@ 2021-10-21  2:22 ` Min Hu (Connor)
  2021-10-21 13:06   ` Ferruh Yigit
  2021-10-22  1:38 ` [dpdk-dev] [PATCH v3] " Min Hu (Connor)
  2 siblings, 1 reply; 8+ messages in thread
From: Min Hu (Connor) @ 2021-10-21  2:22 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, andrew.rybchenko

From: Chengchang Tang <tangchengchang@huawei.com>

Current, the max waiting time for MBX response is 500ms, but in
some scenarios, it is not enough. Since it depends on the response
of the kernel mode driver, and its response time is related to the
scheduling of the system. In this special scenario, most of the
cores are isolated, and only a few cores are used for system
scheduling. When a large number of services are started, the
scheduling of the system will be very busy, and the reply of the
mbx message will time out, which will cause our PMD initialization
to fail.

This patch add a runtime config to set the max wait time. For the
above scenes, users can adjust the waiting time to a suitable value
by themselves.

Fixes: 463e748964f5 ("net/hns3: support mailbox")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
v2:
* add some comment for HNS3_MBX_DEF_TIME_LIMIT_MS.
* remove some check for mbx_time_limit.
---
 drivers/net/hns3/hns3_ethdev.c    | 32 ++++++++++++++++++++++++++++++-
 drivers/net/hns3/hns3_ethdev.h    |  3 +++
 drivers/net/hns3/hns3_ethdev_vf.c |  3 ++-
 drivers/net/hns3/hns3_mbx.c       |  8 +++++---
 drivers/net/hns3/hns3_mbx.h       |  1 +
 5 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
index 693048f587..6b89bcef97 100644
--- a/drivers/net/hns3/hns3_ethdev.c
+++ b/drivers/net/hns3/hns3_ethdev.c
@@ -7306,9 +7306,30 @@ hns3_parse_dev_caps_mask(const char *key, const char *value, void *extra_args)
 	return 0;
 }
 
+static int
+hns3_parse_mbx_time_limit(const char *key, const char *value, void *extra_args)
+{
+	uint32_t val;
+
+	RTE_SET_USED(key);
+
+	val = strtoul(value, NULL, 10);
+
+	/*
+	 * 500ms is empirical value in process of mailbox communication. If
+	 * the delay value is set to one lower thanthe empirical value, mailbox
+	 * communication may fail.
+	 */
+	if (val > HNS3_MBX_DEF_TIME_LIMIT_MS && val <= UINT16_MAX)
+		*(uint16_t *)extra_args = val;
+
+	return 0;
+}
+
 void
 hns3_parse_devargs(struct rte_eth_dev *dev)
 {
+	uint16_t mbx_time_limit_ms = HNS3_MBX_DEF_TIME_LIMIT_MS;
 	struct hns3_adapter *hns = dev->data->dev_private;
 	uint32_t rx_func_hint = HNS3_IO_FUNC_HINT_NONE;
 	uint32_t tx_func_hint = HNS3_IO_FUNC_HINT_NONE;
@@ -7329,6 +7350,9 @@ hns3_parse_devargs(struct rte_eth_dev *dev)
 			   &hns3_parse_io_hint_func, &tx_func_hint);
 	(void)rte_kvargs_process(kvlist, HNS3_DEVARG_DEV_CAPS_MASK,
 			   &hns3_parse_dev_caps_mask, &dev_caps_mask);
+	(void)rte_kvargs_process(kvlist, HNS3_DEVARG_MBX_TIME_LIMIT_MS,
+			   &hns3_parse_mbx_time_limit, &mbx_time_limit_ms);
+
 	rte_kvargs_free(kvlist);
 
 	if (rx_func_hint != HNS3_IO_FUNC_HINT_NONE)
@@ -7344,6 +7368,11 @@ hns3_parse_devargs(struct rte_eth_dev *dev)
 		hns3_warn(hw, "parsed %s = 0x%" PRIx64 ".",
 			  HNS3_DEVARG_DEV_CAPS_MASK, dev_caps_mask);
 	hns->dev_caps_mask = dev_caps_mask;
+
+	if (mbx_time_limit_ms != HNS3_MBX_DEF_TIME_LIMIT_MS)
+		hns3_warn(hw, "parsed %s = %u.", HNS3_DEVARG_MBX_TIME_LIMIT_MS,
+				mbx_time_limit_ms);
+	hns->mbx_time_limit_ms = mbx_time_limit_ms;
 }
 
 static const struct eth_dev_ops hns3_eth_dev_ops = {
@@ -7600,6 +7629,7 @@ RTE_PMD_REGISTER_KMOD_DEP(net_hns3, "* igb_uio | vfio-pci");
 RTE_PMD_REGISTER_PARAM_STRING(net_hns3,
 		HNS3_DEVARG_RX_FUNC_HINT "=vec|sve|simple|common "
 		HNS3_DEVARG_TX_FUNC_HINT "=vec|sve|simple|common "
-		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> ");
+		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> "
+		HNS3_DEVARG_MBX_TIME_LIMIT_MS "=<uint16> ");
 RTE_LOG_REGISTER_SUFFIX(hns3_logtype_init, init, NOTICE);
 RTE_LOG_REGISTER_SUFFIX(hns3_logtype_driver, driver, NOTICE);
diff --git a/drivers/net/hns3/hns3_ethdev.h b/drivers/net/hns3/hns3_ethdev.h
index e28056b1bd..fa08fadc94 100644
--- a/drivers/net/hns3/hns3_ethdev.h
+++ b/drivers/net/hns3/hns3_ethdev.h
@@ -851,6 +851,7 @@ struct hns3_adapter {
 	uint32_t tx_func_hint;
 
 	uint64_t dev_caps_mask;
+	uint16_t mbx_time_limit_ms; /* wait time for mbx message */
 
 	struct hns3_ptype_table ptype_tbl __rte_cache_aligned;
 };
@@ -868,6 +869,8 @@ enum {
 
 #define HNS3_DEVARG_DEV_CAPS_MASK	"dev_caps_mask"
 
+#define HNS3_DEVARG_MBX_TIME_LIMIT_MS	"mbx_time_limit_ms"
+
 enum {
 	HNS3_DEV_SUPPORT_DCB_B,
 	HNS3_DEV_SUPPORT_COPPER_B,
diff --git a/drivers/net/hns3/hns3_ethdev_vf.c b/drivers/net/hns3/hns3_ethdev_vf.c
index 54dbd4b798..8e5df05aa2 100644
--- a/drivers/net/hns3/hns3_ethdev_vf.c
+++ b/drivers/net/hns3/hns3_ethdev_vf.c
@@ -3086,4 +3086,5 @@ RTE_PMD_REGISTER_KMOD_DEP(net_hns3_vf, "* igb_uio | vfio-pci");
 RTE_PMD_REGISTER_PARAM_STRING(net_hns3_vf,
 		HNS3_DEVARG_RX_FUNC_HINT "=vec|sve|simple|common "
 		HNS3_DEVARG_TX_FUNC_HINT "=vec|sve|simple|common "
-		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> ");
+		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> "
+		HNS3_DEVARG_MBX_TIME_LIMIT_MS "=<uint16_t> ");
diff --git a/drivers/net/hns3/hns3_mbx.c b/drivers/net/hns3/hns3_mbx.c
index f36cb10d08..a47622b8a6 100644
--- a/drivers/net/hns3/hns3_mbx.c
+++ b/drivers/net/hns3/hns3_mbx.c
@@ -61,8 +61,9 @@ static int
 hns3_get_mbx_resp(struct hns3_hw *hw, uint16_t code, uint16_t subcode,
 		  uint8_t *resp_data, uint16_t resp_len)
 {
-#define HNS3_MAX_RETRY_US	500000
 #define HNS3_WAIT_RESP_US	100
+#define US_PER_MS		1000
+	uint32_t mbx_time_limit;
 	struct hns3_adapter *hns = HNS3_DEV_HW_TO_ADAPTER(hw);
 	struct hns3_mbx_resp_status *mbx_resp;
 	uint32_t wait_time = 0;
@@ -74,7 +75,8 @@ hns3_get_mbx_resp(struct hns3_hw *hw, uint16_t code, uint16_t subcode,
 		return -EINVAL;
 	}
 
-	while (wait_time < HNS3_MAX_RETRY_US) {
+	mbx_time_limit = (uint32_t)hns->mbx_time_limit_ms * US_PER_MS;
+	while (wait_time < mbx_time_limit) {
 		if (__atomic_load_n(&hw->reset.disable_cmd, __ATOMIC_RELAXED)) {
 			hns3_err(hw, "Don't wait for mbx respone because of "
 				 "disable_cmd");
@@ -103,7 +105,7 @@ hns3_get_mbx_resp(struct hns3_hw *hw, uint16_t code, uint16_t subcode,
 		wait_time += HNS3_WAIT_RESP_US;
 	}
 	hw->mbx_resp.req_msg_data = 0;
-	if (wait_time >= HNS3_MAX_RETRY_US) {
+	if (wait_time >= mbx_time_limit) {
 		hns3_mbx_proc_timeout(hw, code, subcode);
 		return -ETIME;
 	}
diff --git a/drivers/net/hns3/hns3_mbx.h b/drivers/net/hns3/hns3_mbx.h
index f868e33a9e..d637bd2b23 100644
--- a/drivers/net/hns3/hns3_mbx.h
+++ b/drivers/net/hns3/hns3_mbx.h
@@ -87,6 +87,7 @@ enum hns3_mbx_link_fail_subcode {
 
 #define HNS3_MBX_MAX_MSG_SIZE	16
 #define HNS3_MBX_MAX_RESP_DATA_SIZE	8
+#define HNS3_MBX_DEF_TIME_LIMIT_MS	500
 
 enum {
 	HNS3_MBX_RESP_MATCHING_SCHEME_OF_ORIGINAL = 0,
-- 
2.33.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/hns3: add runtime config to set MBX limit time
  2021-09-09 13:20 ` Ferruh Yigit
@ 2021-10-21  2:26   ` Min Hu (Connor)
  0 siblings, 0 replies; 8+ messages in thread
From: Min Hu (Connor) @ 2021-10-21  2:26 UTC (permalink / raw)
  To: Ferruh Yigit, dev; +Cc: andrew.rybchenko

Hi, Ferruh,

在 2021/9/9 21:20, Ferruh Yigit 写道:
> On 8/30/2021 4:48 AM, Min Hu (Connor) wrote:
>> From: Chengchang Tang <tangchengchang@huawei.com>
>>
>> Current, the max waiting time for MBX response is 500ms, but in
>> some scenarios, it is not enough. Since it depends on the response
>> of the kernel mode driver, and its response time is related to the
>> scheduling of the system. In this special scenario, most of the
>> cores are isolated, and only a few cores are used for system
>> scheduling. When a large number of services are started, the
>> scheduling of the system will be very busy, and the reply of the
>> mbx message will time out, which will cause our PMD initialization
>> to fail.
>>
>> This patch add a runtime config to set the max wait time. For the
>> above scenes, users can adjust the waiting time to a suitable value
>> by themselves.
>>
>> Fixes: 463e748964f5 ("net/hns3: support mailbox")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   drivers/net/hns3/hns3_ethdev.c    | 26 +++++++++++++++++++++++++-
>>   drivers/net/hns3/hns3_ethdev.h    |  3 +++
>>   drivers/net/hns3/hns3_ethdev_vf.c |  3 ++-
>>   drivers/net/hns3/hns3_mbx.c       | 10 +++++++---
>>   drivers/net/hns3/hns3_mbx.h       |  1 +
>>   5 files changed, 38 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
>> index 7d37004..4c1a6f1 100644
>> --- a/drivers/net/hns3/hns3_ethdev.c
>> +++ b/drivers/net/hns3/hns3_ethdev.c
>> @@ -7357,9 +7357,24 @@ hns3_parse_dev_caps_mask(const char *key, const char *value, void *extra_args)
>>   	return 0;
>>   }
>>   
>> +static int
>> +hns3_parse_mbx_time_limit(const char *key, const char *value, void *extra_args)
>> +{
>> +	uint32_t val;
>> +
>> +	RTE_SET_USED(key);
>> +
>> +	val = strtoul(value, NULL, 10);
>> +	if (val > HNS3_MBX_DEF_TIME_LIMIT_MS && val <= UINT16_MAX)
> 
> This check adds a restriction that timeout can't be set less the 500ms but this
> restriction is not documented in the help string, comment or commit log etc...
> Can you either remove the restriction or document it?
Fixed in v2, please check it out.
> 
>> +		*(uint16_t *)extra_args = val;
>> +
>> +	return 0;
>> +}
>> +
>>   void
>>   hns3_parse_devargs(struct rte_eth_dev *dev)
>>   {
>> +	uint16_t mbx_time_limit_ms = HNS3_MBX_DEF_TIME_LIMIT_MS;
>>   	struct hns3_adapter *hns = dev->data->dev_private;
>>   	uint32_t rx_func_hint = HNS3_IO_FUNC_HINT_NONE;
>>   	uint32_t tx_func_hint = HNS3_IO_FUNC_HINT_NONE;
>> @@ -7380,6 +7395,9 @@ hns3_parse_devargs(struct rte_eth_dev *dev)
>>   			   &hns3_parse_io_hint_func, &tx_func_hint);
>>   	(void)rte_kvargs_process(kvlist, HNS3_DEVARG_DEV_CAPS_MASK,
>>   			   &hns3_parse_dev_caps_mask, &dev_caps_mask);
>> +	(void)rte_kvargs_process(kvlist, HNS3_DEVARG_MBX_TIME_LIMIT_MS,
>> +			   &hns3_parse_mbx_time_limit, &mbx_time_limit_ms);
>> +
>>   	rte_kvargs_free(kvlist);
>>   
>>   	if (rx_func_hint != HNS3_IO_FUNC_HINT_NONE)
>> @@ -7395,6 +7413,11 @@ hns3_parse_devargs(struct rte_eth_dev *dev)
>>   		hns3_warn(hw, "parsed %s = 0x%" PRIx64 ".",
>>   			  HNS3_DEVARG_DEV_CAPS_MASK, dev_caps_mask);
>>   	hns->dev_caps_mask = dev_caps_mask;
>> +
>> +	if (mbx_time_limit_ms != HNS3_MBX_DEF_TIME_LIMIT_MS)
>> +		hns3_warn(hw, "parsed %s = %u.", HNS3_DEVARG_MBX_TIME_LIMIT_MS,
>> +				mbx_time_limit_ms);
>> +	hns->mbx_time_limit_ms = mbx_time_limit_ms;
>>   }
>>   
>>   static const struct eth_dev_ops hns3_eth_dev_ops = {
>> @@ -7651,6 +7674,7 @@ RTE_PMD_REGISTER_KMOD_DEP(net_hns3, "* igb_uio | vfio-pci");
>>   RTE_PMD_REGISTER_PARAM_STRING(net_hns3,
>>   		HNS3_DEVARG_RX_FUNC_HINT "=vec|sve|simple|common "
>>   		HNS3_DEVARG_TX_FUNC_HINT "=vec|sve|simple|common "
>> -		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> ");
>> +		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> "
>> +		HNS3_DEVARG_MBX_TIME_LIMIT_MS "=<uint16> ");
>>   RTE_LOG_REGISTER_SUFFIX(hns3_logtype_init, init, NOTICE);
>>   RTE_LOG_REGISTER_SUFFIX(hns3_logtype_driver, driver, NOTICE);
>> diff --git a/drivers/net/hns3/hns3_ethdev.h b/drivers/net/hns3/hns3_ethdev.h
>> index 0e4e426..6476ad5 100644
>> --- a/drivers/net/hns3/hns3_ethdev.h
>> +++ b/drivers/net/hns3/hns3_ethdev.h
>> @@ -852,6 +852,7 @@ struct hns3_adapter {
>>   	uint32_t tx_func_hint;
>>   
>>   	uint64_t dev_caps_mask;
>> +	uint16_t mbx_time_limit_ms; /* wait time for mbx message */
>>   
>>   	struct hns3_ptype_table ptype_tbl __rte_cache_aligned;
>>   };
>> @@ -869,6 +870,8 @@ enum {
>>   
>>   #define HNS3_DEVARG_DEV_CAPS_MASK	"dev_caps_mask"
>>   
>> +#define HNS3_DEVARG_MBX_TIME_LIMIT_MS	"mbx_time_limit_ms"
>> +
>>   enum {
>>   	HNS3_DEV_SUPPORT_DCB_B,
>>   	HNS3_DEV_SUPPORT_COPPER_B,
>> diff --git a/drivers/net/hns3/hns3_ethdev_vf.c b/drivers/net/hns3/hns3_ethdev_vf.c
>> index 8d9b797..7ed9cb2 100644
>> --- a/drivers/net/hns3/hns3_ethdev_vf.c
>> +++ b/drivers/net/hns3/hns3_ethdev_vf.c
>> @@ -3115,4 +3115,5 @@ RTE_PMD_REGISTER_KMOD_DEP(net_hns3_vf, "* igb_uio | vfio-pci");
>>   RTE_PMD_REGISTER_PARAM_STRING(net_hns3_vf,
>>   		HNS3_DEVARG_RX_FUNC_HINT "=vec|sve|simple|common "
>>   		HNS3_DEVARG_TX_FUNC_HINT "=vec|sve|simple|common "
>> -		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> ");
>> +		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> "
>> +		HNS3_DEVARG_MBX_TIME_LIMIT_MS "=<uint16_t> ");
>> diff --git a/drivers/net/hns3/hns3_mbx.c b/drivers/net/hns3/hns3_mbx.c
>> index f36cb10..a1556f7 100644
>> --- a/drivers/net/hns3/hns3_mbx.c
>> +++ b/drivers/net/hns3/hns3_mbx.c
>> @@ -61,8 +61,9 @@ static int
>>   hns3_get_mbx_resp(struct hns3_hw *hw, uint16_t code, uint16_t subcode,
>>   		  uint8_t *resp_data, uint16_t resp_len)
>>   {
>> -#define HNS3_MAX_RETRY_US	500000
>>   #define HNS3_WAIT_RESP_US	100
>> +#define US_PER_MS		1000
>> +	uint32_t mbx_time_limit = HNS3_MBX_DEF_TIME_LIMIT_MS * US_PER_MS;
>>   	struct hns3_adapter *hns = HNS3_DEV_HW_TO_ADAPTER(hw);
>>   	struct hns3_mbx_resp_status *mbx_resp;
>>   	uint32_t wait_time = 0;
>> @@ -74,7 +75,10 @@ hns3_get_mbx_resp(struct hns3_hw *hw, uint16_t code, uint16_t subcode,
>>   		return -EINVAL;
>>   	}
>>   
>> -	while (wait_time < HNS3_MAX_RETRY_US) {
>> +	if (hns->mbx_time_limit_ms > HNS3_MBX_DEF_TIME_LIMIT_MS)
>> +		mbx_time_limit = (uint32_t)hns->mbx_time_limit_ms * US_PER_MS;
>> +
> 
> Why calculating the 'mbx_time_limit' twice in this function, isn't
> 'hns->mbx_time_limit_ms' has already either default value or user provided
> value, why not just keep this calculation even with removing above check?
> 
Fixed in v2, please check it out.
>> +	while (wait_time < mbx_time_limit) {
>>   		if (__atomic_load_n(&hw->reset.disable_cmd, __ATOMIC_RELAXED)) {
>>   			hns3_err(hw, "Don't wait for mbx respone because of "
>>   				 "disable_cmd");
>> @@ -103,7 +107,7 @@ hns3_get_mbx_resp(struct hns3_hw *hw, uint16_t code, uint16_t subcode,
>>   		wait_time += HNS3_WAIT_RESP_US;
>>   	}
>>   	hw->mbx_resp.req_msg_data = 0;
>> -	if (wait_time >= HNS3_MAX_RETRY_US) {
>> +	if (wait_time >= mbx_time_limit) {
>>   		hns3_mbx_proc_timeout(hw, code, subcode);
>>   		return -ETIME;
>>   	}
>> diff --git a/drivers/net/hns3/hns3_mbx.h b/drivers/net/hns3/hns3_mbx.h
>> index f868e33..d637bd2 100644
>> --- a/drivers/net/hns3/hns3_mbx.h
>> +++ b/drivers/net/hns3/hns3_mbx.h
>> @@ -87,6 +87,7 @@ enum hns3_mbx_link_fail_subcode {
>>   
>>   #define HNS3_MBX_MAX_MSG_SIZE	16
>>   #define HNS3_MBX_MAX_RESP_DATA_SIZE	8
>> +#define HNS3_MBX_DEF_TIME_LIMIT_MS	500
>>   
>>   enum {
>>   	HNS3_MBX_RESP_MATCHING_SCHEME_OF_ORIGINAL = 0,
>>
> 
> .
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/hns3: add runtime config to set MBX limit time
  2021-10-21  2:22 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
@ 2021-10-21 13:06   ` Ferruh Yigit
  2021-10-22  1:41     ` Min Hu (Connor)
  0 siblings, 1 reply; 8+ messages in thread
From: Ferruh Yigit @ 2021-10-21 13:06 UTC (permalink / raw)
  To: Min Hu (Connor), dev; +Cc: andrew.rybchenko

On 10/21/2021 3:22 AM, Min Hu (Connor) wrote:
> From: Chengchang Tang<tangchengchang@huawei.com>
> 
> Current, the max waiting time for MBX response is 500ms, but in
> some scenarios, it is not enough. Since it depends on the response
> of the kernel mode driver, and its response time is related to the
> scheduling of the system. In this special scenario, most of the
> cores are isolated, and only a few cores are used for system
> scheduling. When a large number of services are started, the
> scheduling of the system will be very busy, and the reply of the
> mbx message will time out, which will cause our PMD initialization
> to fail.
> 
> This patch add a runtime config to set the max wait time. For the
> above scenes, users can adjust the waiting time to a suitable value
> by themselves.
> 
> Fixes: 463e748964f5 ("net/hns3: support mailbox")
> Cc:stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang<tangchengchang@huawei.com>
> Signed-off-by: Min Hu (Connor)<humin29@huawei.com>
> ---
> v2:
> * add some comment for HNS3_MBX_DEF_TIME_LIMIT_MS.
> * remove some check for mbx_time_limit.
> ---
>   drivers/net/hns3/hns3_ethdev.c    | 32 ++++++++++++++++++++++++++++++-
>   drivers/net/hns3/hns3_ethdev.h    |  3 +++
>   drivers/net/hns3/hns3_ethdev_vf.c |  3 ++-
>   drivers/net/hns3/hns3_mbx.c       |  8 +++++---
>   drivers/net/hns3/hns3_mbx.h       |  1 +
>   5 files changed, 42 insertions(+), 5 deletions(-)


New devarg need to be documented in the 'doc/guides/nics/hns3.rst',
in "Runtime Config Options" section.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH v3] net/hns3: add runtime config to set MBX limit time
  2021-08-30  3:48 [dpdk-dev] [PATCH] net/hns3: add runtime config to set MBX limit time Min Hu (Connor)
  2021-09-09 13:20 ` Ferruh Yigit
  2021-10-21  2:22 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
@ 2021-10-22  1:38 ` Min Hu (Connor)
  2021-10-22  2:13   ` Ferruh Yigit
  2 siblings, 1 reply; 8+ messages in thread
From: Min Hu (Connor) @ 2021-10-22  1:38 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, andrew.rybchenko

From: Chengchang Tang <tangchengchang@huawei.com>

Current, the max waiting time for MBX response is 500ms, but in
some scenarios, it is not enough. Since it depends on the response
of the kernel mode driver, and its response time is related to the
scheduling of the system. In this special scenario, most of the
cores are isolated, and only a few cores are used for system
scheduling. When a large number of services are started, the
scheduling of the system will be very busy, and the reply of the
mbx message will time out, which will cause our PMD initialization
to fail.

This patch add a runtime config to set the max wait time. For the
above scenes, users can adjust the waiting time to a suitable value
by themselves.

Fixes: 463e748964f5 ("net/hns3: support mailbox")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
v3:
* set mbx_time_limit_ms to be documented.

v2:
* add some comment for HNS3_MBX_DEF_TIME_LIMIT_MS.
* remove some check for mbx_time_limit.
---
 doc/guides/nics/hns3.rst          | 14 ++++++++++++++
 drivers/net/hns3/hns3_ethdev.c    | 32 ++++++++++++++++++++++++++++++-
 drivers/net/hns3/hns3_ethdev.h    |  3 +++
 drivers/net/hns3/hns3_ethdev_vf.c |  3 ++-
 drivers/net/hns3/hns3_mbx.c       |  8 +++++---
 drivers/net/hns3/hns3_mbx.h       |  1 +
 6 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/doc/guides/nics/hns3.rst b/doc/guides/nics/hns3.rst
index 6fbeccddba..5f68a10ecf 100644
--- a/doc/guides/nics/hns3.rst
+++ b/doc/guides/nics/hns3.rst
@@ -115,6 +115,20 @@ Runtime Config Options
   For example::
   -a 0000:7d:00.0,dev_caps_mask=0xF
 
+- ``mbx_time_limit_ms`` (default ``500``)
+   Used to define the mailbox time limit by user.
+   Current, the max waiting time for MBX response is 500ms, but in
+   some scenarios, it is not enough. Since it depends on the response
+   of the kernel mode driver, and its response time is related to the
+   scheduling of the system. In this special scenario, most of the
+   cores are isolated, and only a few cores are used for system
+   scheduling. When a large number of services are started, the
+   scheduling of the system will be very busy, and the reply of the
+   mbx message will time out, which will cause our PMD initialization
+   to fail. So provide access to set mailbox time limit for user.
+
+   For example::
+   -a 0000:7d:00.0,mbx_time_limit_ms=600
 
 Link status event Pre-conditions
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
index 693048f587..6b89bcef97 100644
--- a/drivers/net/hns3/hns3_ethdev.c
+++ b/drivers/net/hns3/hns3_ethdev.c
@@ -7306,9 +7306,30 @@ hns3_parse_dev_caps_mask(const char *key, const char *value, void *extra_args)
 	return 0;
 }
 
+static int
+hns3_parse_mbx_time_limit(const char *key, const char *value, void *extra_args)
+{
+	uint32_t val;
+
+	RTE_SET_USED(key);
+
+	val = strtoul(value, NULL, 10);
+
+	/*
+	 * 500ms is empirical value in process of mailbox communication. If
+	 * the delay value is set to one lower thanthe empirical value, mailbox
+	 * communication may fail.
+	 */
+	if (val > HNS3_MBX_DEF_TIME_LIMIT_MS && val <= UINT16_MAX)
+		*(uint16_t *)extra_args = val;
+
+	return 0;
+}
+
 void
 hns3_parse_devargs(struct rte_eth_dev *dev)
 {
+	uint16_t mbx_time_limit_ms = HNS3_MBX_DEF_TIME_LIMIT_MS;
 	struct hns3_adapter *hns = dev->data->dev_private;
 	uint32_t rx_func_hint = HNS3_IO_FUNC_HINT_NONE;
 	uint32_t tx_func_hint = HNS3_IO_FUNC_HINT_NONE;
@@ -7329,6 +7350,9 @@ hns3_parse_devargs(struct rte_eth_dev *dev)
 			   &hns3_parse_io_hint_func, &tx_func_hint);
 	(void)rte_kvargs_process(kvlist, HNS3_DEVARG_DEV_CAPS_MASK,
 			   &hns3_parse_dev_caps_mask, &dev_caps_mask);
+	(void)rte_kvargs_process(kvlist, HNS3_DEVARG_MBX_TIME_LIMIT_MS,
+			   &hns3_parse_mbx_time_limit, &mbx_time_limit_ms);
+
 	rte_kvargs_free(kvlist);
 
 	if (rx_func_hint != HNS3_IO_FUNC_HINT_NONE)
@@ -7344,6 +7368,11 @@ hns3_parse_devargs(struct rte_eth_dev *dev)
 		hns3_warn(hw, "parsed %s = 0x%" PRIx64 ".",
 			  HNS3_DEVARG_DEV_CAPS_MASK, dev_caps_mask);
 	hns->dev_caps_mask = dev_caps_mask;
+
+	if (mbx_time_limit_ms != HNS3_MBX_DEF_TIME_LIMIT_MS)
+		hns3_warn(hw, "parsed %s = %u.", HNS3_DEVARG_MBX_TIME_LIMIT_MS,
+				mbx_time_limit_ms);
+	hns->mbx_time_limit_ms = mbx_time_limit_ms;
 }
 
 static const struct eth_dev_ops hns3_eth_dev_ops = {
@@ -7600,6 +7629,7 @@ RTE_PMD_REGISTER_KMOD_DEP(net_hns3, "* igb_uio | vfio-pci");
 RTE_PMD_REGISTER_PARAM_STRING(net_hns3,
 		HNS3_DEVARG_RX_FUNC_HINT "=vec|sve|simple|common "
 		HNS3_DEVARG_TX_FUNC_HINT "=vec|sve|simple|common "
-		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> ");
+		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> "
+		HNS3_DEVARG_MBX_TIME_LIMIT_MS "=<uint16> ");
 RTE_LOG_REGISTER_SUFFIX(hns3_logtype_init, init, NOTICE);
 RTE_LOG_REGISTER_SUFFIX(hns3_logtype_driver, driver, NOTICE);
diff --git a/drivers/net/hns3/hns3_ethdev.h b/drivers/net/hns3/hns3_ethdev.h
index e28056b1bd..fa08fadc94 100644
--- a/drivers/net/hns3/hns3_ethdev.h
+++ b/drivers/net/hns3/hns3_ethdev.h
@@ -851,6 +851,7 @@ struct hns3_adapter {
 	uint32_t tx_func_hint;
 
 	uint64_t dev_caps_mask;
+	uint16_t mbx_time_limit_ms; /* wait time for mbx message */
 
 	struct hns3_ptype_table ptype_tbl __rte_cache_aligned;
 };
@@ -868,6 +869,8 @@ enum {
 
 #define HNS3_DEVARG_DEV_CAPS_MASK	"dev_caps_mask"
 
+#define HNS3_DEVARG_MBX_TIME_LIMIT_MS	"mbx_time_limit_ms"
+
 enum {
 	HNS3_DEV_SUPPORT_DCB_B,
 	HNS3_DEV_SUPPORT_COPPER_B,
diff --git a/drivers/net/hns3/hns3_ethdev_vf.c b/drivers/net/hns3/hns3_ethdev_vf.c
index 54dbd4b798..8e5df05aa2 100644
--- a/drivers/net/hns3/hns3_ethdev_vf.c
+++ b/drivers/net/hns3/hns3_ethdev_vf.c
@@ -3086,4 +3086,5 @@ RTE_PMD_REGISTER_KMOD_DEP(net_hns3_vf, "* igb_uio | vfio-pci");
 RTE_PMD_REGISTER_PARAM_STRING(net_hns3_vf,
 		HNS3_DEVARG_RX_FUNC_HINT "=vec|sve|simple|common "
 		HNS3_DEVARG_TX_FUNC_HINT "=vec|sve|simple|common "
-		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> ");
+		HNS3_DEVARG_DEV_CAPS_MASK "=<1-65535> "
+		HNS3_DEVARG_MBX_TIME_LIMIT_MS "=<uint16_t> ");
diff --git a/drivers/net/hns3/hns3_mbx.c b/drivers/net/hns3/hns3_mbx.c
index f36cb10d08..a47622b8a6 100644
--- a/drivers/net/hns3/hns3_mbx.c
+++ b/drivers/net/hns3/hns3_mbx.c
@@ -61,8 +61,9 @@ static int
 hns3_get_mbx_resp(struct hns3_hw *hw, uint16_t code, uint16_t subcode,
 		  uint8_t *resp_data, uint16_t resp_len)
 {
-#define HNS3_MAX_RETRY_US	500000
 #define HNS3_WAIT_RESP_US	100
+#define US_PER_MS		1000
+	uint32_t mbx_time_limit;
 	struct hns3_adapter *hns = HNS3_DEV_HW_TO_ADAPTER(hw);
 	struct hns3_mbx_resp_status *mbx_resp;
 	uint32_t wait_time = 0;
@@ -74,7 +75,8 @@ hns3_get_mbx_resp(struct hns3_hw *hw, uint16_t code, uint16_t subcode,
 		return -EINVAL;
 	}
 
-	while (wait_time < HNS3_MAX_RETRY_US) {
+	mbx_time_limit = (uint32_t)hns->mbx_time_limit_ms * US_PER_MS;
+	while (wait_time < mbx_time_limit) {
 		if (__atomic_load_n(&hw->reset.disable_cmd, __ATOMIC_RELAXED)) {
 			hns3_err(hw, "Don't wait for mbx respone because of "
 				 "disable_cmd");
@@ -103,7 +105,7 @@ hns3_get_mbx_resp(struct hns3_hw *hw, uint16_t code, uint16_t subcode,
 		wait_time += HNS3_WAIT_RESP_US;
 	}
 	hw->mbx_resp.req_msg_data = 0;
-	if (wait_time >= HNS3_MAX_RETRY_US) {
+	if (wait_time >= mbx_time_limit) {
 		hns3_mbx_proc_timeout(hw, code, subcode);
 		return -ETIME;
 	}
diff --git a/drivers/net/hns3/hns3_mbx.h b/drivers/net/hns3/hns3_mbx.h
index f868e33a9e..d637bd2b23 100644
--- a/drivers/net/hns3/hns3_mbx.h
+++ b/drivers/net/hns3/hns3_mbx.h
@@ -87,6 +87,7 @@ enum hns3_mbx_link_fail_subcode {
 
 #define HNS3_MBX_MAX_MSG_SIZE	16
 #define HNS3_MBX_MAX_RESP_DATA_SIZE	8
+#define HNS3_MBX_DEF_TIME_LIMIT_MS	500
 
 enum {
 	HNS3_MBX_RESP_MATCHING_SCHEME_OF_ORIGINAL = 0,
-- 
2.33.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/hns3: add runtime config to set MBX limit time
  2021-10-21 13:06   ` Ferruh Yigit
@ 2021-10-22  1:41     ` Min Hu (Connor)
  0 siblings, 0 replies; 8+ messages in thread
From: Min Hu (Connor) @ 2021-10-22  1:41 UTC (permalink / raw)
  To: Ferruh Yigit, dev; +Cc: andrew.rybchenko

Hi, Ferruh,
	V3 has been sent,please check it out, thanks.

在 2021/10/21 21:06, Ferruh Yigit 写道:
> On 10/21/2021 3:22 AM, Min Hu (Connor) wrote:
>> From: Chengchang Tang<tangchengchang@huawei.com>
>>
>> Current, the max waiting time for MBX response is 500ms, but in
>> some scenarios, it is not enough. Since it depends on the response
>> of the kernel mode driver, and its response time is related to the
>> scheduling of the system. In this special scenario, most of the
>> cores are isolated, and only a few cores are used for system
>> scheduling. When a large number of services are started, the
>> scheduling of the system will be very busy, and the reply of the
>> mbx message will time out, which will cause our PMD initialization
>> to fail.
>>
>> This patch add a runtime config to set the max wait time. For the
>> above scenes, users can adjust the waiting time to a suitable value
>> by themselves.
>>
>> Fixes: 463e748964f5 ("net/hns3: support mailbox")
>> Cc:stable@dpdk.org
>>
>> Signed-off-by: Chengchang Tang<tangchengchang@huawei.com>
>> Signed-off-by: Min Hu (Connor)<humin29@huawei.com>
>> ---
>> v2:
>> * add some comment for HNS3_MBX_DEF_TIME_LIMIT_MS.
>> * remove some check for mbx_time_limit.
>> ---
>>   drivers/net/hns3/hns3_ethdev.c    | 32 ++++++++++++++++++++++++++++++-
>>   drivers/net/hns3/hns3_ethdev.h    |  3 +++
>>   drivers/net/hns3/hns3_ethdev_vf.c |  3 ++-
>>   drivers/net/hns3/hns3_mbx.c       |  8 +++++---
>>   drivers/net/hns3/hns3_mbx.h       |  1 +
>>   5 files changed, 42 insertions(+), 5 deletions(-)
> 
> 
> New devarg need to be documented in the 'doc/guides/nics/hns3.rst',
> in "Runtime Config Options" section.
> .

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v3] net/hns3: add runtime config to set MBX limit time
  2021-10-22  1:38 ` [dpdk-dev] [PATCH v3] " Min Hu (Connor)
@ 2021-10-22  2:13   ` Ferruh Yigit
  0 siblings, 0 replies; 8+ messages in thread
From: Ferruh Yigit @ 2021-10-22  2:13 UTC (permalink / raw)
  To: Min Hu (Connor), dev; +Cc: andrew.rybchenko

On 10/22/2021 2:38 AM, Min Hu (Connor) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> Current, the max waiting time for MBX response is 500ms, but in
> some scenarios, it is not enough. Since it depends on the response
> of the kernel mode driver, and its response time is related to the
> scheduling of the system. In this special scenario, most of the
> cores are isolated, and only a few cores are used for system
> scheduling. When a large number of services are started, the
> scheduling of the system will be very busy, and the reply of the
> mbx message will time out, which will cause our PMD initialization
> to fail.
> 
> This patch add a runtime config to set the max wait time. For the
> above scenes, users can adjust the waiting time to a suitable value
> by themselves.
> 
> Fixes: 463e748964f5 ("net/hns3: support mailbox")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

Applied to dpdk-next-net/main, thanks.


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-10-22  2:13 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-30  3:48 [dpdk-dev] [PATCH] net/hns3: add runtime config to set MBX limit time Min Hu (Connor)
2021-09-09 13:20 ` Ferruh Yigit
2021-10-21  2:26   ` Min Hu (Connor)
2021-10-21  2:22 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
2021-10-21 13:06   ` Ferruh Yigit
2021-10-22  1:41     ` Min Hu (Connor)
2021-10-22  1:38 ` [dpdk-dev] [PATCH v3] " Min Hu (Connor)
2021-10-22  2:13   ` Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).