[v7] net/i40e: limit the number of VF messages

  From: Alvin Zhang <alvinx.zhang@intel.com>

PF driver supportes counting VF adminQ messages. If any VF driver
sending much more adminQ messages to it's PF driver in a period of
time, it will trigger the PF's message limiting, then in the
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(-)
  

> -----Original Message-----
> From: Zhang, AlvinX
> Sent: Friday, September 20, 2019 6:23 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Zhang, AlvinX <alvinx.zhang@intel.com>
> Subject: [PATCH v7] net/i40e: limit the number of VF messages
> 
> From: Alvin Zhang <alvinx.zhang@intel.com>
> 
> PF driver supportes counting VF adminQ messages. If any VF driver sending
> much more adminQ messages to it's PF driver in a period of time, it will trigger
> the PF's message limiting, then in the 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>

Acked-by: Qi Zhang <qi.z.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
>