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

[Wang, Haiyue]

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Monday, June 10, 2019 19:17
> To: Wang, Haiyue <haiyue.wang at intel.com>; dev at dpdk.org; 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: stable at dpdk.org
> Subject: RE: [PATCH v2] net/ixgbevf: add an option pflink_fullchk to get link status nowait
> 
> 
> 
> > -----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.
> 
> 
In fact, 91546fb62e67 (in Fixes) breaks the default behavior, change it to "fully check"
in nowait. This patch switches to "meaningful check" PF's physical link status by default.
So now, we can meet three scenarios:
	1. Sync with PF's physical link status in nowait, (Default), no mailbox interrupt
	   handling in PF, the fastest way.
	2. Sync with PF's physical link status & mailbox (PF's service running) status in
	   nowait, will consume the host's interrupt handling, if many VFs are running and
	   checking frequently, system load may be heavy.
	3. Wait mode, it will be slow, not only #2 actions, but also wait the link status
	   stable in some time.
> >
> > 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