DPDK patches and discussions
 help / color / Atom feed
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
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
>

  parent reply index

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 publically 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

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox