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 08C7DA0613 for ; Tue, 24 Sep 2019 01:54:57 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2C2AF343C; Tue, 24 Sep 2019 01:54:56 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id CEBC09E4 for ; Tue, 24 Sep 2019 01:54:53 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Sep 2019 16:54:52 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,542,1559545200"; d="scan'208";a="195521337" Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.117.17]) by FMSMGA003.fm.intel.com with ESMTP; 23 Sep 2019 16:54:51 -0700 Date: Tue, 24 Sep 2019 07:52:46 +0800 From: Ye Xiaolong To: alvinx.zhang@intel.com Cc: qi.z.zhang@intel.com, dev@dpdk.org Message-ID: <20190923235246.GG7338@intel.com> References: <20190918210349.67635-1-alvinx.zhang@intel.com> <20190920102246.44945-1-alvinx.zhang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190920102246.44945-1-alvinx.zhang@intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH v7] net/i40e: limit the number of VF messages 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 09/20, alvinx.zhang@intel.com wrote: >From: Alvin Zhang > >PF driver supportes counting VF adminQ messages. If any VF driver s/supportes/supports/ >sending much more adminQ messages to it's PF driver in a period of s/sending/sends s/it's/its >time, it will trigger the PF's message limiting, then in the s/limiting/limitation >next certain amount of seconds the PF driver will ignore any new >adminQ message from that VF. > >Signed-off-by: Alvin Zhang >--- > >v7: modify codes according to the comments. >v6: combine valid/invalid/unsupported messages to one. >v5: add validating the "valid" message, scheme redesigned. >v4: change the instruction file "i40e.rst". >v3: resolve the compile issue on ubuntu 32-bit system. >v2: modify codes according to the comments. >--- > doc/guides/nics/i40e.rst | 10 ++++++ > drivers/net/i40e/i40e_ethdev.c | 72 ++++++++++++++++++++++++++++++++++++++++++ > drivers/net/i40e/i40e_ethdev.h | 32 +++++++++++++++++++ > drivers/net/i40e/i40e_pf.c | 71 +++++++++++++++++++++++++++++++++++++++-- > 4 files changed, 183 insertions(+), 2 deletions(-) > >diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst >index 0884e15..38acf59 100644 >--- a/doc/guides/nics/i40e.rst >+++ b/doc/guides/nics/i40e.rst >@@ -185,6 +185,16 @@ Runtime Config Options > > -w 84:00.0,use-latest-supported-vec=1 > >+- ``Enable validation for VF message`` (default ``not enabled``) >+ >+ The PF counts messages from each VF. If in any period of seconds the message >+ statistic from a VF exceeds maximal limitation, the PF will ignore any new message >+ from that VF for some seconds. >+ Format -- "maximal-message@period-seconds:ignore-seconds" >+ For example:: >+ >+ -w 84:00.0,vf_msg_cfg=80@120:180 >+ > Vector RX Pre-conditions > ~~~~~~~~~~~~~~~~~~~~~~~~ > For Vector RX it is assumed that the number of descriptor rings will be a power >diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c >index 4e40b7a..4e2c488 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_VF_MSG_CFG "vf_msg_cfg" > > #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_VF_MSG_CFG, > NULL}; > > static const struct rte_pci_id pci_id_i40e_map[] = { >@@ -1256,6 +1258,74 @@ static inline void i40e_config_automask(struct i40e_pf *pf) > return 0; > } > >+static int >+read_vf_msg_config(__rte_unused const char *key, >+ const char *value, >+ void *opaque) >+{ >+ struct i40e_vf_msg_cfg cfg; >+ >+ memset(&cfg, 0, sizeof(cfg)); >+ if (sscanf(value, "%u@%u:%u", &cfg.max_msg, &cfg.period, >+ &cfg.ignore_second) != 3) { >+ PMD_DRV_LOG(ERR, "format error! example: " >+ "%s=60@120:180", ETH_I40E_VF_MSG_CFG); >+ return -EINVAL; >+ } >+ >+ /* >+ * If the message validation function been enabled, the 'period' >+ * and 'ignore_second' must greater than 0. >+ */ >+ if (cfg.max_msg && (!cfg.period || !cfg.ignore_second)) { >+ PMD_DRV_LOG(ERR, "%s error! the second and third" >+ " number must be greater than 0!", >+ ETH_I40E_VF_MSG_CFG); >+ return -EINVAL; >+ } >+ >+ memcpy(opaque, &cfg, sizeof(cfg)); rte_memcpy is preferred. And how about struct i40e_vf_msg_cfg *cfg = opaque; Then you don't need memset/memcpy at all. >+ return 0; >+} >+ >+static int >+i40e_parse_vf_msg_config(struct rte_eth_dev *dev, >+ struct i40e_vf_msg_cfg *msg_cfg) >+{ >+ struct rte_kvargs *kvlist; >+ int kvargs_count; >+ int ret = 0; >+ >+ /* reset all to zero */ >+ memset(msg_cfg, 0, sizeof(*msg_cfg)); The code is quite straightforward, the comment is not needed. >+ >+ 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_VF_MSG_CFG); >+ if (!kvargs_count) >+ goto free_end; >+ >+ if (kvargs_count > 1) { >+ PMD_DRV_LOG(ERR, "More than one argument \"%s\"!", >+ ETH_I40E_VF_MSG_CFG); >+ ret = -EINVAL; >+ goto free_end; >+ } >+ >+ if (rte_kvargs_process(kvlist, ETH_I40E_VF_MSG_CFG, >+ read_vf_msg_config, msg_cfg) < 0) >+ ret = -EINVAL; >+ >+free_end: >+ rte_kvargs_free(kvlist); >+ return ret; >+} >+ > #define I40E_ALARM_INTERVAL 50000 /* us */ > > static int >@@ -1328,6 +1398,8 @@ static inline void i40e_config_automask(struct i40e_pf *pf) > return -EIO; > } > >+ /* read VF message configuration */ Unnecessary comment. >+ i40e_parse_vf_msg_config(dev, &pf->vf_msg_cfg); > /* 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..261954b 100644 >--- a/drivers/net/i40e/i40e_ethdev.h >+++ b/drivers/net/i40e/i40e_ethdev.h >@@ -426,6 +426,22 @@ struct i40e_pf_vf { > /* version of the virtchnl from VF */ > struct virtchnl_version_info version; > uint32_t request_caps; /* offload caps requested from VF */ >+ >+ /* >+ * Variables for store the arrival timestamp of VF messages. >+ * If the timestamp of latest message stored at >+ * `msg_timestamps[index % max]` then the timestamp of >+ * earliest message stored at `msg_time[(index + 1) % max]`. >+ * When a new message come, the timestamp of this message >+ * will be stored at `msg_timestamps[(index + 1) % max]` and the >+ * earliest message timestamp is at >+ * `msg_timestamps[(index + 2) % max]` now... >+ */ >+ uint32_t msg_index; >+ uint64_t *msg_timestamps; >+ >+ /* cycle of stop ignoring VF message */ >+ uint64_t ignore_end_cycle; > }; > > /* >@@ -900,6 +916,20 @@ struct i40e_rte_flow_rss_conf { > uint16_t queue[I40E_MAX_Q_PER_TC]; /**< Queues indices to use. */ > }; > >+struct i40e_vf_msg_cfg { >+ /* maximal VF message during a statistic period */ >+ uint32_t max_msg; >+ >+ /* statistic period, in second */ >+ uint32_t period; >+ /* >+ * If message statistics from a VF exceed the maximal limitation, >+ * the PF will ignore any new message from that VF for >+ * 'ignor_second' time. >+ */ >+ uint32_t ignore_second; >+}; >+ > /* > * Structure to store private data specific for PF instance. > */ >@@ -975,6 +1005,8 @@ struct i40e_pf { > struct i40e_customized_pctype customized_pctype[I40E_CUSTOMIZED_MAX]; > /* Switch Domain Id */ > uint16_t switch_domain_id; >+ >+ struct i40e_vf_msg_cfg vf_msg_cfg; > }; > > enum pending_msg { >diff --git a/drivers/net/i40e/i40e_pf.c b/drivers/net/i40e/i40e_pf.c >index b28d02e..9ee2be6 100644 >--- a/drivers/net/i40e/i40e_pf.c >+++ b/drivers/net/i40e/i40e_pf.c >@@ -1297,6 +1297,8 @@ > /* AdminQ will pass absolute VF id, transfer to internal vf id */ > uint16_t vf_id = abs_vf_id - hw->func_caps.vf_base_id; > struct rte_pmd_i40e_mb_event_param ret_param; >+ uint64_t first_cycle, cur_cycle; >+ uint32_t next, limit; > bool b_op = TRUE; > int ret; > >@@ -1306,11 +1308,19 @@ > } > > vf = &pf->vfs[vf_id]; >+ >+ /* read current cycle */ Unnecessary comment. >+ cur_cycle = rte_get_timer_cycles(); >+ >+ /* if the VF being blocked, ignore the message and return */ >+ if (cur_cycle < vf->ignore_end_cycle) >+ 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; >+ goto check; > } > > /* perform basic checks on the msg */ >@@ -1334,7 +1344,7 @@ > vf_id, opcode, msglen); > i40e_pf_host_send_msg_to_vf(vf, opcode, > I40E_ERR_PARAM, NULL, 0); >- return; >+ goto check; > } > > /** >@@ -1456,6 +1466,41 @@ > NULL, 0); > break; > } >+ >+check: >+ /* if message validation not enabled */ >+ if (!pf->vf_msg_cfg.max_msg) >+ return; >+ >+ /* store current cycle to next place */ >+ limit = pf->vf_msg_cfg.max_msg - 1; >+ next = vf->msg_index; >+ next = (next >= limit) ? 0 : (next + 1); >+ vf->msg_index = next; >+ vf->msg_timestamps[next] = cur_cycle; >+ >+ /* read the timestamp of earliest message */ >+ next = (next >= limit) ? 0 : (next + 1); >+ first_cycle = vf->msg_timestamps[next]; Above code block is quite tedious, how about: vf->msg_timestamps[msg_index++ % (pf->vf_msg_cfg - 1)] = cur_cycle; first_cycle = vf->msg_timestamps[msg_index % (pf->vf_msg_cfg - 1)]; >+ >+ /* >+ * If the time span from the arrival time of first message to >+ * the arrival time of current message smaller than `period`, >+ * that mean too much message in this statistic period. >+ */ >+ if (first_cycle && cur_cycle < first_cycle + >+ (uint64_t)pf->vf_msg_cfg.period * rte_get_timer_hz()) { >+ PMD_DRV_LOG(ERR, "VF %u too much messages(%u in %u" I think we can use WARN level for the log. >+ " seconds),\n\tany new message from which" >+ " will be ignored during next %u seconds!", >+ vf_id, limit + 1, >+ (uint32_t)((cur_cycle - first_cycle + >+ rte_get_timer_hz() - 1) / rte_get_timer_hz()), >+ pf->vf_msg_cfg.ignore_second); >+ vf->ignore_end_cycle = rte_get_timer_cycles() + >+ pf->vf_msg_cfg.ignore_second * >+ rte_get_timer_hz(); >+ } > } > > int >@@ -1463,6 +1508,7 @@ > { > struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private); > struct i40e_hw *hw = I40E_PF_TO_HW(pf); >+ size_t size; > int ret, i; > uint32_t val; > >@@ -1489,10 +1535,22 @@ > I40E_WRITE_REG(hw, I40E_PFGEN_PORTMDIO_NUM, val); > I40E_WRITE_FLUSH(hw); > >+ /* calculate the memory size for storing timestamp of messages */ >+ size = pf->vf_msg_cfg.max_msg * sizeof(uint64_t); >+ > for (i = 0; i < pf->vf_num; i++) { > pf->vfs[i].pf = pf; > pf->vfs[i].state = I40E_VF_INACTIVE; > pf->vfs[i].vf_idx = i; >+ >+ if (size) { >+ /* allocate memory for store timestamp of messages */ >+ pf->vfs[i].msg_timestamps = (uint64_t *) >+ rte_zmalloc("i40e_pf_vf", size, 0); Unnecessary cast. >+ if (pf->vfs[i].msg_timestamps == NULL) Need to set ret = -ENOMEM for the failure. >+ goto fail; >+ } >+ > ret = i40e_pf_host_vf_reset(&pf->vfs[i], 0); > if (ret != I40E_SUCCESS) > goto fail; >@@ -1505,6 +1563,9 @@ > return I40E_SUCCESS; > > fail: >+ for (; i >= 0; i--) >+ if (pf->vfs[i].msg_timestamps) >+ rte_free(pf->vfs[i].msg_timestamps); rte_free will check the NULL pointer inside, so the if can be removed. > rte_free(pf->vfs); > i40e_pf_enable_irq0(hw); > >@@ -1517,6 +1578,7 @@ > struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private); > struct i40e_hw *hw = I40E_PF_TO_HW(pf); > uint32_t val; >+ int i; > > PMD_INIT_FUNC_TRACE(); > >@@ -1529,6 +1591,11 @@ > (pf->vf_nb_qps == 0)) > return I40E_SUCCESS; > >+ /* free memory for store timestamp of messages */ >+ for (i = 0; i < pf->vf_num; i++) >+ if (pf->vfs[i].msg_timestamps) >+ rte_free(pf->vfs[i].msg_timestamps); Ditto. >+ > /* free memory to store VF structure */ > rte_free(pf->vfs); > pf->vfs = NULL; >-- >1.8.3.1 >