From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id DF8A5A0C43; Thu, 21 Oct 2021 04:26:07 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C00CD40150; Thu, 21 Oct 2021 04:26:07 +0200 (CEST) Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by mails.dpdk.org (Postfix) with ESMTP id B45FF40142 for ; Thu, 21 Oct 2021 04:26:05 +0200 (CEST) Received: from dggeme756-chm.china.huawei.com (unknown [172.30.72.54]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4HZWXG53Fcz8tlG for ; Thu, 21 Oct 2021 10:24:46 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by dggeme756-chm.china.huawei.com (10.3.19.102) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2308.15; Thu, 21 Oct 2021 10:26:01 +0800 To: Ferruh Yigit , CC: References: <1630295328-44604-1-git-send-email-humin29@huawei.com> <22f0f080-b12b-2d7c-6d1b-4646252caec8@intel.com> From: "Min Hu (Connor)" Message-ID: <300aa38f-ba08-d4e4-3246-a9847cde816f@huawei.com> Date: Thu, 21 Oct 2021 10:26:01 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: <22f0f080-b12b-2d7c-6d1b-4646252caec8@intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) To dggeme756-chm.china.huawei.com (10.3.19.102) X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH] net/hns3: add runtime config to set MBX limit time X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi, Ferruh, 在 2021/9/9 21:20, Ferruh Yigit 写道: > On 8/30/2021 4:48 AM, Min Hu (Connor) wrote: >> From: Chengchang Tang >> >> 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 >> Signed-off-by: Min Hu (Connor) >> --- >> 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 "= "); >> 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 "= "); >> 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, >> > > . >