From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7933EA046B for ; Thu, 22 Aug 2019 06:19:31 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 584061BF1D; Thu, 22 Aug 2019 06:19:30 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id F1D661BF1C for ; Thu, 22 Aug 2019 06:19:27 +0200 (CEST) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Aug 2019 21:19:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,415,1559545200"; d="scan'208";a="330239215" Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.117.5]) by orsmga004.jf.intel.com with ESMTP; 21 Aug 2019 21:19:24 -0700 Date: Thu, 22 Aug 2019 12:18:09 +0800 From: Ye Xiaolong To: alvinx.zhang@intel.com Cc: beilei.xing@intel.com, dev@dpdk.org Message-ID: <20190822041809.GA82777@intel.com> References: <1566285506-63817-1-git-send-email-alvinx.zhang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1566285506-63817-1-git-send-email-alvinx.zhang@intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH] net/i40e: add checking for messages from VF X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" On 08/20, alvinx.zhang@intel.com wrote: >From: Alvin Zhang > >If VF driver in VM continuous sending invalid messages by mailbox, >it will waste CPU cycles on PF driver and impact other VF drivers >configuration. New feature can count the numbers of invalid and >unsupported messages from VFs, when the statistics from a VF >exceed maximum limit, PF driver will ignore any message from that >VF for some seconds. > >Signed-off-by: Alvin Zhang >--- > drivers/net/i40e/i40e_ethdev.c | 80 +++++++++++++++++ > drivers/net/i40e/i40e_ethdev.h | 30 +++++++ > drivers/net/i40e/i40e_pf.c | 189 ++++++++++++++++++++++++++++++++--------- > 3 files changed, 258 insertions(+), 41 deletions(-) > >diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c >index 4e40b7a..045ba49 100644 >--- a/drivers/net/i40e/i40e_ethdev.c >+++ b/drivers/net/i40e/i40e_ethdev.c >@@ -44,6 +44,7 @@ > #define ETH_I40E_SUPPORT_MULTI_DRIVER "support-multi-driver" > #define ETH_I40E_QUEUE_NUM_PER_VF_ARG "queue-num-per-vf" > #define ETH_I40E_USE_LATEST_VEC "use-latest-supported-vec" >+#define ETH_I40E_MAX_VF_WRONG_MSG "vf_max_wrong_msg" > > #define I40E_CLEAR_PXE_WAIT_MS 200 > >@@ -406,6 +407,7 @@ static int i40e_sw_tunnel_filter_insert(struct i40e_pf *pf, > ETH_I40E_SUPPORT_MULTI_DRIVER, > ETH_I40E_QUEUE_NUM_PER_VF_ARG, > ETH_I40E_USE_LATEST_VEC, >+ ETH_I40E_MAX_VF_WRONG_MSG, > NULL}; > > static const struct rte_pci_id pci_id_i40e_map[] = { >@@ -1256,6 +1258,82 @@ static inline void i40e_config_automask(struct i40e_pf *pf) > return 0; > } > >+static int >+read_vf_msg_check_info(__rte_unused const char *key, >+ const char *value, >+ void *opaque) >+{ >+ struct i40e_wrong_vf_msg info; >+ >+ memset(&info, 0, sizeof(info)); >+ /* >+ * VF message checking function need 3 parameters, max_invalid, >+ * max_unsupported and silence_seconds. >+ * When continuous invalid or unsupported message statistics >+ * from a VF exceed the limitation of 'max_invalid' or >+ * 'max_unsupported', PF will ignore any message from that VF for >+ * 'silence_seconds' seconds. >+ */ >+ if (sscanf(value, "%u:%u:%lu", &info.max_invalid, >+ &info.max_unsupport, &info.silence_seconds) >+ != 3) { >+ PMD_DRV_LOG(ERR, "vf_max_wrong_msg error! format like: " >+ "vf_max_wrong_msg=4:6:60"); >+ return -EINVAL; >+ } >+ >+ /* >+ * If invalid or unsupported message checking function is enabled >+ * by setting max_invalid or max_unsupport variable to not zero, >+ * 'slience_seconds' must be greater than zero. >+ */ >+ if ((info.max_invalid | info.max_unsupport) && info.max_invalid || info.max_unsupport? And I prefer to use unsupported in your variable names for unsupport is not a valid word. >+ !info.silence_seconds) { >+ PMD_DRV_LOG(ERR, "vf_max_wrong_msg error! last integer" >+ " must be larger than zero"); >+ return -EINVAL; >+ } >+ >+ memcpy(opaque, &info, sizeof(struct i40e_wrong_vf_msg)); >+ return 0; >+} >+ >+static int >+i40e_parse_vf_msg_check_info(struct rte_eth_dev *dev, >+ struct i40e_wrong_vf_msg *wrong_info) >+{ >+ int ret = 0; >+ int kvargs_count; >+ struct rte_kvargs *kvlist; >+ >+ /* reset all to zero */ >+ memset(wrong_info, 0, sizeof(*wrong_info)); >+ >+ if (!dev->device->devargs) >+ return ret; >+ >+ kvlist = rte_kvargs_parse(dev->device->devargs->args, valid_keys); >+ if (!kvlist) >+ return -EINVAL; >+ >+ kvargs_count = rte_kvargs_count(kvlist, ETH_I40E_MAX_VF_WRONG_MSG); >+ if (!kvargs_count) >+ goto free_end; >+ >+ if (kvargs_count > 1) >+ PMD_DRV_LOG(WARNING, "More than one argument \"%s\" and only " >+ "the first invalid or last valid one is used !", >+ ETH_I40E_MAX_VF_WRONG_MSG); What about we just allow 1 wrong msg argument? >+ >+ if (rte_kvargs_process(kvlist, ETH_I40E_MAX_VF_WRONG_MSG, >+ read_vf_msg_check_info, wrong_info) < 0) >+ ret = -EINVAL; >+ >+free_end: >+ rte_kvargs_free(kvlist); >+ return ret; >+} >+ > #define I40E_ALARM_INTERVAL 50000 /* us */ > > static int >@@ -1328,6 +1406,8 @@ static inline void i40e_config_automask(struct i40e_pf *pf) > return -EIO; > } > >+ /* read VF message checking function parameters */ >+ i40e_parse_vf_msg_check_info(dev, &pf->wrong_vf_msg_conf); > /* Check if need to support multi-driver */ > i40e_support_multi_driver(dev); > /* Check if users want the latest supported vec path */ >diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h >index 38ac3ea..6ab5762 100644 >--- a/drivers/net/i40e/i40e_ethdev.h >+++ b/drivers/net/i40e/i40e_ethdev.h >@@ -426,6 +426,23 @@ struct i40e_pf_vf { > /* version of the virtchnl from VF */ > struct virtchnl_version_info version; > uint32_t request_caps; /* offload caps requested from VF */ >+ >+ /* >+ * Counter of message from VF >+ * invalid_cmd, invalid command since last valid command >+ * unsupport_cmd, unsupported command since last valid command >+ * invalid_total, total invalid command >+ * unsupport_total, total unsupported command >+ * ignored_cmd, ignored command in silence >+ */ >+ uint16_t invalid_cmd; >+ uint16_t unsupport_cmd; >+ uint64_t invalid_total; >+ uint64_t unsupport_total; >+ uint64_t ignored_cmd; What about rename them to invalid_cmd_cnt/unsupported_cmd_cnt/ignored_cmd_cnt according to their meanings? >+ >+ /* cycle of stop ignoring VF message */ >+ uint64_t silence_end_cycle; > }; > [snip] >@@ -1291,6 +1297,14 @@ > uint8_t *msg, > uint16_t msglen) > { >+ /* Ratio of succeed to invalid or unsupported message. >+ * When PF read a invalid or unsupported message, the corresponding >+ * counter will increased by ratio. >+ * When PF read a succeed message, the invalid and unsupported >+ * message counter will decreased by 1. >+ */ >+ const uint16_t ratio = 2; >+ What's the benefit of using ratio? > struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private); > struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private); > struct i40e_pf_vf *vf; >@@ -1306,11 +1320,19 @@ > } > > vf = &pf->vfs[vf_id]; >+ >+ /* if timer not end, ignore the message and return */ >+ if (rte_get_timer_cycles() < vf->silence_end_cycle) { >+ vf->ignored_cmd++; >+ return; >+ } >+ > if (!vf->vsi) { > PMD_DRV_LOG(ERR, "NO VSI associated with VF found"); > i40e_pf_host_send_msg_to_vf(vf, opcode, > I40E_ERR_NO_AVAILABLE_VSI, NULL, 0); >- return; >+ ret = I40E_ERR_NO_AVAILABLE_VSI; >+ goto err_cmd; > } > > /* perform basic checks on the msg */ >@@ -1331,14 +1353,15 @@ > > if (ret) { > PMD_DRV_LOG(ERR, "Invalid message from VF %u, opcode %u, len %u", >- vf_id, opcode, msglen); >+ vf_id, opcode, msglen); > i40e_pf_host_send_msg_to_vf(vf, opcode, >- I40E_ERR_PARAM, NULL, 0); >- return; >+ I40E_ERR_PARAM, NULL, 0); >+ ret = I40E_ERR_PARAM; >+ goto err_cmd; > } > > /** >- * initialise structure to send to user application >+ * initialize structure to send to user application > * will return response from user in retval field > */ > ret_param.retval = RTE_PMD_I40E_MB_EVENT_PROCEED; >@@ -1373,78 +1396,84 @@ > break; > case VIRTCHNL_OP_GET_VF_RESOURCES: > PMD_DRV_LOG(INFO, "OP_GET_VF_RESOURCES received"); >- i40e_pf_host_process_cmd_get_vf_resource(vf, msg, b_op); >+ ret = i40e_pf_host_process_cmd_get_vf_resource(vf, msg, b_op); > break; > case VIRTCHNL_OP_CONFIG_VSI_QUEUES: > PMD_DRV_LOG(INFO, "OP_CONFIG_VSI_QUEUES received"); >- i40e_pf_host_process_cmd_config_vsi_queues(vf, msg, >+ ret = i40e_pf_host_process_cmd_config_vsi_queues(vf, msg, > msglen, b_op); > break; > case VIRTCHNL_OP_CONFIG_IRQ_MAP: > PMD_DRV_LOG(INFO, "OP_CONFIG_IRQ_MAP received"); >- i40e_pf_host_process_cmd_config_irq_map(vf, msg, msglen, b_op); >+ ret = i40e_pf_host_process_cmd_config_irq_map(vf, msg, >+ msglen, b_op); > break; > case VIRTCHNL_OP_ENABLE_QUEUES: > PMD_DRV_LOG(INFO, "OP_ENABLE_QUEUES received"); > if (b_op) { >- i40e_pf_host_process_cmd_enable_queues(vf, msg, msglen); >+ ret = i40e_pf_host_process_cmd_enable_queues(vf, >+ msg, msglen); > i40e_notify_vf_link_status(dev, vf); > } else { >- i40e_pf_host_send_msg_to_vf( >- vf, VIRTCHNL_OP_ENABLE_QUEUES, >- I40E_NOT_SUPPORTED, NULL, 0); >+ i40e_pf_host_send_msg_to_vf(vf, >+ VIRTCHNL_OP_ENABLE_QUEUES, >+ I40E_NOT_SUPPORTED, NULL, 0); >+ ret = I40E_NOT_SUPPORTED; > } > break; > case VIRTCHNL_OP_DISABLE_QUEUES: > PMD_DRV_LOG(INFO, "OP_DISABLE_QUEUE received"); >- i40e_pf_host_process_cmd_disable_queues(vf, msg, msglen, b_op); >+ ret = i40e_pf_host_process_cmd_disable_queues(vf, >+ msg, msglen, b_op); > break; > case VIRTCHNL_OP_ADD_ETH_ADDR: > PMD_DRV_LOG(INFO, "OP_ADD_ETHER_ADDRESS received"); >- i40e_pf_host_process_cmd_add_ether_address(vf, msg, >+ ret = i40e_pf_host_process_cmd_add_ether_address(vf, msg, > msglen, b_op); > break; > case VIRTCHNL_OP_DEL_ETH_ADDR: > PMD_DRV_LOG(INFO, "OP_DEL_ETHER_ADDRESS received"); >- i40e_pf_host_process_cmd_del_ether_address(vf, msg, >+ ret = i40e_pf_host_process_cmd_del_ether_address(vf, msg, > msglen, b_op); > break; > case VIRTCHNL_OP_ADD_VLAN: > PMD_DRV_LOG(INFO, "OP_ADD_VLAN received"); >- i40e_pf_host_process_cmd_add_vlan(vf, msg, msglen, b_op); >+ ret = i40e_pf_host_process_cmd_add_vlan(vf, msg, msglen, b_op); > break; > case VIRTCHNL_OP_DEL_VLAN: > PMD_DRV_LOG(INFO, "OP_DEL_VLAN received"); >- i40e_pf_host_process_cmd_del_vlan(vf, msg, msglen, b_op); >+ ret = i40e_pf_host_process_cmd_del_vlan(vf, msg, msglen, b_op); > break; > case VIRTCHNL_OP_CONFIG_PROMISCUOUS_MODE: > PMD_DRV_LOG(INFO, "OP_CONFIG_PROMISCUOUS_MODE received"); >- i40e_pf_host_process_cmd_config_promisc_mode(vf, msg, >+ ret = i40e_pf_host_process_cmd_config_promisc_mode(vf, msg, > msglen, b_op); > break; > case VIRTCHNL_OP_GET_STATS: > PMD_DRV_LOG(INFO, "OP_GET_STATS received"); >- i40e_pf_host_process_cmd_get_stats(vf, b_op); >+ ret = i40e_pf_host_process_cmd_get_stats(vf, b_op); > break; > case VIRTCHNL_OP_ENABLE_VLAN_STRIPPING: > PMD_DRV_LOG(INFO, "OP_ENABLE_VLAN_STRIPPING received"); >- i40e_pf_host_process_cmd_enable_vlan_strip(vf, b_op); >+ ret = i40e_pf_host_process_cmd_enable_vlan_strip(vf, b_op); > break; > case VIRTCHNL_OP_DISABLE_VLAN_STRIPPING: > PMD_DRV_LOG(INFO, "OP_DISABLE_VLAN_STRIPPING received"); >- i40e_pf_host_process_cmd_disable_vlan_strip(vf, b_op); >+ ret = i40e_pf_host_process_cmd_disable_vlan_strip(vf, b_op); > break; > case VIRTCHNL_OP_CONFIG_RSS_LUT: > PMD_DRV_LOG(INFO, "OP_CONFIG_RSS_LUT received"); >- i40e_pf_host_process_cmd_set_rss_lut(vf, msg, msglen, b_op); >+ ret = i40e_pf_host_process_cmd_set_rss_lut(vf, msg, >+ msglen, b_op); > break; > case VIRTCHNL_OP_CONFIG_RSS_KEY: > PMD_DRV_LOG(INFO, "OP_CONFIG_RSS_KEY received"); >- i40e_pf_host_process_cmd_set_rss_key(vf, msg, msglen, b_op); >+ ret = i40e_pf_host_process_cmd_set_rss_key(vf, msg, >+ msglen, b_op); > break; > case VIRTCHNL_OP_REQUEST_QUEUES: > PMD_DRV_LOG(INFO, "OP_REQUEST_QUEUES received"); >- i40e_pf_host_process_cmd_request_queues(vf, msg); >+ ret = i40e_pf_host_process_cmd_request_queues(vf, msg); > break; > > /* Don't add command supported below, which will >@@ -1452,10 +1481,83 @@ > */ > default: > PMD_DRV_LOG(ERR, "%u received, not supported", opcode); >- i40e_pf_host_send_msg_to_vf(vf, opcode, I40E_ERR_PARAM, >+ i40e_pf_host_send_msg_to_vf(vf, opcode, I40E_NOT_SUPPORTED, > NULL, 0); >+ ret = I40E_NOT_SUPPORTED; > break; > } >+ >+err_cmd: >+ /* If the amount of successive invalid or unsupported message from a >+ * VF exceed maximal limitation, PF will start a timer. >+ * Before the timer timed out, PF should ignore any message from >+ * this VF. >+ * Once the timer timed out, PF will accept a message from the VF, >+ * if this message is still invalid or unsupported, PF will reset >+ * and start the timer again. Otherwise if the first message execute >+ * success,PF will decrease invalid and unsupported message counter >+ * by 1. >+ */ >+ vf->silence_end_cycle = 0; >+ switch (ret) { >+ case I40E_SUCCESS: >+ if (!vf->invalid_cmd && !vf->unsupport_cmd) >+ break; >+ >+ if (vf->unsupport_cmd) >+ vf->unsupport_cmd--; >+ if (vf->invalid_cmd) >+ vf->invalid_cmd--; >+ >+ if (!vf->invalid_cmd && !vf->unsupport_cmd) >+ PMD_DRV_LOG(WARNING, "VF %u message, ignored %lu, " >+ "invalid %lu, unsupported %lu", vf_id, >+ vf->ignored_cmd, vf->invalid_total, >+ vf->unsupport_total); >+ break; >+ >+ case I40E_ERR_PARAM: >+ case I40E_ERR_NO_AVAILABLE_VSI: >+ vf->invalid_total++; >+ if (!pf->wrong_vf_msg_conf.max_invalid) >+ break; >+ if (vf->invalid_cmd >= pf->wrong_vf_msg_conf.max_invalid >+ * ratio) { >+ PMD_DRV_LOG(ERR, "VF %u too much continuous invalid" >+ " message(%u, maximum %u, total %lu)!", >+ vf_id, vf->invalid_cmd / ratio + 1, >+ pf->wrong_vf_msg_conf.max_invalid, >+ vf->invalid_total); >+ vf->silence_end_cycle = rte_get_timer_cycles() + >+ pf->wrong_vf_msg_conf.silence_seconds >+ * rte_get_timer_hz(); >+ } else { >+ vf->invalid_cmd += ratio; >+ } >+ break; >+ >+ case I40E_NOT_SUPPORTED: >+ vf->unsupport_total++; >+ if (!pf->wrong_vf_msg_conf.max_unsupport) >+ break; >+ if (vf->unsupport_cmd >= pf->wrong_vf_msg_conf.max_unsupport >+ * ratio) { >+ PMD_DRV_LOG(ERR, "VF %u too much continuous unsupported" >+ " message(%u, maximum %u, total %lu)!", >+ vf_id, vf->unsupport_cmd / ratio + 1, >+ pf->wrong_vf_msg_conf.max_unsupport, >+ vf->unsupport_total); >+ vf->silence_end_cycle = rte_get_timer_cycles() + >+ pf->wrong_vf_msg_conf.silence_seconds >+ * rte_get_timer_hz(); >+ } else { >+ vf->unsupport_cmd += ratio; >+ } >+ break; >+ default: >+ break; >+ } >+ return; > } > > int >@@ -1493,6 +1595,11 @@ > pf->vfs[i].pf = pf; > pf->vfs[i].state = I40E_VF_INACTIVE; > pf->vfs[i].vf_idx = i; >+ >+ pf->vfs[i].invalid_cmd = 0; >+ pf->vfs[i].unsupport_cmd = 0; >+ pf->vfs[i].silence_end_cycle = 0; >+ > ret = i40e_pf_host_vf_reset(&pf->vfs[i], 0); > if (ret != I40E_SUCCESS) > goto fail; >-- >1.8.3.1 >