Re: [dpdk-dev] [PATCH] net/hns3: add runtime config to set MBX limit time

["Min Hu (Connor)" <humin29@huawei.com>]

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,
>>
> 
> .
>