From: Ye Xiaolong <xiaolong.ye@intel.com>
To: alvinx.zhang@intel.com
Cc: qi.z.zhang@intel.com, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v7] net/i40e: limit the number of VF messages
Date: Tue, 24 Sep 2019 07:52:46 +0800 [thread overview]
Message-ID: <20190923235246.GG7338@intel.com> (raw)
In-Reply-To: <20190920102246.44945-1-alvinx.zhang@intel.com>
On 09/20, alvinx.zhang@intel.com wrote:
>From: Alvin Zhang <alvinx.zhang@intel.com>
>
>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 <alvinx.zhang@intel.com>
>---
>
>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
>
next prev parent reply other threads:[~2019-09-23 23:54 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-20 7:18 [dpdk-dev] [PATCH] net/i40e: add checking for messages from VF alvinx.zhang
2019-08-22 4:18 ` Ye Xiaolong
2019-08-28 7:13 ` [dpdk-dev] [PATCH v2] " alvinx.zhang
2019-09-09 10:01 ` [dpdk-dev] [PATCH v3] net/i40e: validate all " alvinx.zhang
2019-09-09 15:11 ` [dpdk-dev] [PATCH v4] " alvinx.zhang
2019-09-09 13:34 ` Ye Xiaolong
2019-09-10 17:12 ` alvinx.zhang
2019-09-18 15:39 ` [dpdk-dev] [PATCH v5] " alvinx.zhang
2019-09-18 21:03 ` [dpdk-dev] [PATCH v6] net/i40e: limit the number of VF messages alvinx.zhang
2019-09-20 10:22 ` [dpdk-dev] [PATCH v7] " alvinx.zhang
2019-09-20 2:20 ` Zhang, Qi Z
2019-09-23 23:52 ` Ye Xiaolong [this message]
2019-09-24 14:24 ` [dpdk-dev] [PATCH v8] " alvinx.zhang
2019-09-24 8:47 ` Ye Xiaolong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190923235246.GG7338@intel.com \
--to=xiaolong.ye@intel.com \
--cc=alvinx.zhang@intel.com \
--cc=dev@dpdk.org \
--cc=qi.z.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).