[dpdk-dev] [PATCH v2] net/ixgbevf: add an option pflink_fullchk to get link status nowait

[Zhang, Qi Z]

> -----Original Message-----
> From: Wang, Haiyue
> Sent: Friday, June 7, 2019 11:56 PM
> To: dev at dpdk.org; Zhang, Qi Z <qi.z.zhang at intel.com>; Lu, Wenzhuo
> <wenzhuo.lu at intel.com>; Wang, Liang-min <liang-min.wang at intel.com>;
> daniels at research.att.com; ktraynor at redhat.com
> Cc: Wang, Haiyue <haiyue.wang at intel.com>; stable at dpdk.org
> Subject: [PATCH v2] net/ixgbevf: add an option pflink_fullchk to get link status
> nowait
> 
> To get the VF's link status by calling 'rte_eth_link_get_nowait()', the VF not
> only check PF's physical link status, but also check the mailbox running status.
> And mailbox checking will generate mailbox interrupt in PF, it will be worse if
> many VFs are running in the system, the PF will have to handle many
> interrrupts.
> 
> Normally, checking the PF's physical link status is enough for nowait.
> For different scenarios, adding an 'pflink_fullchk' option to control whether to
> check the link fully or not.

Seems the patch change the default behavior which is always "fully check", 
I assume a no fully check does not guarantee the link status is synced correctly, right?
should we implement this devargs in an inverse way to avoid the inconsistent with previous version? 
>From my view correctness should take high priority than performance.


> 
> Fixes: 91546fb62e67 ("net/ixgbevf: fix link state")
> Cc: stable at dpdk.org
> 
> Signed-off-by: Haiyue Wang <haiyue.wang at intel.com>
> ---
>  doc/guides/nics/ixgbe.rst        | 25 ++++++++++++++++
>  drivers/net/ixgbe/ixgbe_ethdev.c | 63
> ++++++++++++++++++++++++++++++++++++++++
>  drivers/net/ixgbe/ixgbe_ethdev.h |  5 ++++
>  3 files changed, 93 insertions(+)
> 
> diff --git a/doc/guides/nics/ixgbe.rst b/doc/guides/nics/ixgbe.rst index
> 975143f..5c3a7e4 100644
> --- a/doc/guides/nics/ixgbe.rst
> +++ b/doc/guides/nics/ixgbe.rst
> @@ -82,6 +82,31 @@ To guarantee the constraint, capabilities in
> dev_conf.rxmode.offloads will be ch
> 
>  fdir_conf->mode will also be checked.
> 
> +VF Runtime Options
> +^^^^^^^^^^^^^^^^^^
> +
> +The following ``devargs`` options can be enabled at runtime. They must
> +be passed as part of EAL arguments. For example,
> +
> +.. code-block:: console
> +
> +   testpmd -w af:10.0,pflink_fullchk=1 -- -i
> +
> +- ``pflink_fullchk`` (default **0**)
> +
> +  When calling ``rte_eth_link_get_nowait()`` to get VF link status,
> + this option is used to control how VF synchronizes its status with
> + PF's. If set, VF will not only check the PF's physical link status  by
> + reading related register, but also check the mailbox status. We  call
> + this behavior as fully checking. And checking mailbox will  trigger
> + PF's mailbox interrupt generation. If unset, the application  can get
> + the VF's link status quickly by just reading the PF's link  status
> + register, this will avoid the whole system's mailbox interrupt
> + generation.
> +
> +  ``rte_eth_link_get()`` will still use the mailbox method regardless
> + of the pflink_fullchk setting.
> +
>  RX Burst Size
>  ^^^^^^^^^^^^^
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 3636b50..ade119d 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -23,6 +23,7 @@
>  #include <rte_bus_pci.h>
>  #include <rte_branch_prediction.h>
>  #include <rte_memory.h>
> +#include <rte_kvargs.h>
>  #include <rte_eal.h>
>  #include <rte_alarm.h>
>  #include <rte_ether.h>
> @@ -127,6 +128,13 @@
>  #define IXGBE_EXVET_VET_EXT_SHIFT              16
>  #define IXGBE_DMATXCTL_VT_MASK                 0xFFFF0000
> 
> +#define IXGBEVF_DEVARG_PFLINK_FULLCHK		"pflink_fullchk"
> +
> +static const char * const ixgbevf_valid_arguments[] = {
> +	IXGBEVF_DEVARG_PFLINK_FULLCHK,
> +	NULL
> +};
> +
>  static int eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params);
> static int eth_ixgbe_dev_uninit(struct rte_eth_dev *eth_dev);  static int
> ixgbe_fdir_filter_init(struct rte_eth_dev *eth_dev); @@ -1550,6 +1558,45
> @@ generate_random_mac_addr(struct rte_ether_addr *mac_addr)
>  	memcpy(&mac_addr->addr_bytes[3], &random, 3);  }
> 
> +static int
> +devarg_handle_int(__rte_unused const char *key, const char *value,
> +		  void *extra_args)
> +{
> +	uint16_t *n = extra_args;
> +
> +	if (value == NULL || extra_args == NULL)
> +		return -EINVAL;
> +
> +	*n = (uint16_t)strtoul(value, NULL, 0);
> +	if (*n == USHRT_MAX && errno == ERANGE)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static void
> +ixgbevf_parse_devargs(struct ixgbe_adapter *adapter,
> +		      struct rte_devargs *devargs)
> +{
> +	struct rte_kvargs *kvlist;
> +	uint16_t pflink_fullchk;
> +
> +	if (devargs == NULL)
> +		return;
> +
> +	kvlist = rte_kvargs_parse(devargs->args, ixgbevf_valid_arguments);
> +	if (kvlist == NULL)
> +		return;
> +
> +	if (rte_kvargs_count(kvlist, IXGBEVF_DEVARG_PFLINK_FULLCHK) == 1 &&
> +	    rte_kvargs_process(kvlist, IXGBEVF_DEVARG_PFLINK_FULLCHK,
> +			       devarg_handle_int, &pflink_fullchk) == 0 &&
> +	    pflink_fullchk == 1)
> +		adapter->pflink_fullchk = 1;
> +
> +	rte_kvargs_free(kvlist);
> +}
> +
>  /*
>   * Virtual Function device init
>   */
> @@ -1598,6 +1645,9 @@ eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev)
>  		return 0;
>  	}
> 
> +	ixgbevf_parse_devargs(eth_dev->data->dev_private,
> +			      pci_dev->device.devargs);
> +
>  	rte_eth_copy_pci_info(eth_dev, pci_dev);
> 
>  	hw->device_id = pci_dev->id.device_id; @@ -3910,6 +3960,8 @@ static
> int  ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
>  		   int *link_up, int wait_to_complete)  {
> +	struct ixgbe_adapter *adapter = container_of(hw,
> +						     struct ixgbe_adapter, hw);
>  	struct ixgbe_mbx_info *mbx = &hw->mbx;
>  	struct ixgbe_mac_info *mac = &hw->mac;
>  	uint32_t links_reg, in_msg;
> @@ -3970,6 +4022,15 @@ ixgbevf_check_link(struct ixgbe_hw *hw,
> ixgbe_link_speed *speed,
>  		*speed = IXGBE_LINK_SPEED_UNKNOWN;
>  	}
> 
> +	if (wait_to_complete == 0 && adapter->pflink_fullchk == 0) {
> +		if (*speed == IXGBE_LINK_SPEED_UNKNOWN)
> +			mac->get_link_status = true;
> +		else
> +			mac->get_link_status = false;
> +
> +		goto out;
> +	}
> +
>  	/* if the read failed it could just be a mailbox collision, best wait
>  	 * until we are called again and don't report an error
>  	 */
> @@ -8671,6 +8732,8 @@ RTE_PMD_REGISTER_KMOD_DEP(net_ixgbe, "*
> igb_uio | uio_pci_generic | vfio-pci");  RTE_PMD_REGISTER_PCI(net_ixgbe_vf,
> rte_ixgbevf_pmd);  RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe_vf,
> pci_id_ixgbevf_map);  RTE_PMD_REGISTER_KMOD_DEP(net_ixgbe_vf, "*
> igb_uio | vfio-pci");
> +RTE_PMD_REGISTER_PARAM_STRING(net_ixgbe_vf,
> +			      IXGBEVF_DEVARG_PFLINK_FULLCHK "=<0|1>");
> 
>  RTE_INIT(ixgbe_init_log)
>  {
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h
> b/drivers/net/ixgbe/ixgbe_ethdev.h
> index fdad94d..6e9ed2e 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.h
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.h
> @@ -498,6 +498,11 @@ struct ixgbe_adapter {
> 
>  	/* For RSS reta table update */
>  	uint8_t rss_reta_updated;
> +
> +	/* Used for VF link sync with PF's physical and logical (by checking
> +	 * mailbox status) link status.
> +	 */
> +	uint8_t pflink_fullchk;
>  };
> 
>  struct ixgbe_vf_representor {
> --
> 2.7.4