* [dpdk-stable] [PATCH v2] net/ixgbevf: add an option pflink_fullchk to get link status nowait
@ 2019-06-07 15:56 Haiyue Wang
2019-06-07 16:08 ` Scott Daniels
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Haiyue Wang @ 2019-06-07 15:56 UTC (permalink / raw)
To: dev, qi.z.zhang, wenzhuo.lu, liang-min.wang, daniels, ktraynor
Cc: Haiyue Wang, stable
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.
Fixes: 91546fb62e67 ("net/ixgbevf: fix link state")
Cc: stable@dpdk.org
Signed-off-by: Haiyue Wang <haiyue.wang@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/ixgbevf: add an option pflink_fullchk to get link status nowait
2019-06-07 15:56 [dpdk-stable] [PATCH v2] net/ixgbevf: add an option pflink_fullchk to get link status nowait Haiyue Wang
@ 2019-06-07 16:08 ` Scott Daniels
2019-06-10 11:17 ` Zhang, Qi Z
2019-06-11 1:29 ` Zhang, Qi Z
2 siblings, 0 replies; 5+ messages in thread
From: Scott Daniels @ 2019-06-07 16:08 UTC (permalink / raw)
To: Haiyue Wang; +Cc: dev, qi.z.zhang, wenzhuo.lu, liang-min.wang, ktraynor, stable
Acked-by: Scott Daniels daniels@research.att.com
On Fri, 7 Jun 2019, Haiyue Wang wrote:
> 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.
>
> Fixes: 91546fb62e67 ("net/ixgbevf: fix link state")
> Cc: stable@dpdk.org
>
> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/ixgbevf: add an option pflink_fullchk to get link status nowait
2019-06-07 15:56 [dpdk-stable] [PATCH v2] net/ixgbevf: add an option pflink_fullchk to get link status nowait Haiyue Wang
2019-06-07 16:08 ` Scott Daniels
@ 2019-06-10 11:17 ` Zhang, Qi Z
2019-06-11 1:00 ` Wang, Haiyue
2019-06-11 1:29 ` Zhang, Qi Z
2 siblings, 1 reply; 5+ messages in thread
From: Zhang, Qi Z @ 2019-06-10 11:17 UTC (permalink / raw)
To: Wang, Haiyue, dev, Lu, Wenzhuo, Wang, Liang-min, daniels, ktraynor; +Cc: stable
> -----Original Message-----
> From: Wang, Haiyue
> Sent: Friday, June 7, 2019 11:56 PM
> To: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Wang, Liang-min <liang-min.wang@intel.com>;
> daniels@research.att.com; ktraynor@redhat.com
> Cc: Wang, Haiyue <haiyue.wang@intel.com>; stable@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@dpdk.org
>
> Signed-off-by: Haiyue Wang <haiyue.wang@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/ixgbevf: add an option pflink_fullchk to get link status nowait
2019-06-10 11:17 ` Zhang, Qi Z
@ 2019-06-11 1:00 ` Wang, Haiyue
0 siblings, 0 replies; 5+ messages in thread
From: Wang, Haiyue @ 2019-06-11 1:00 UTC (permalink / raw)
To: Zhang, Qi Z, dev, Lu, Wenzhuo, Wang, Liang-min, daniels, ktraynor; +Cc: stable
> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Monday, June 10, 2019 19:17
> To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wang,
> Liang-min <liang-min.wang@intel.com>; daniels@research.att.com; ktraynor@redhat.com
> Cc: stable@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@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Lu, Wenzhuo
> > <wenzhuo.lu@intel.com>; Wang, Liang-min <liang-min.wang@intel.com>;
> > daniels@research.att.com; ktraynor@redhat.com
> > Cc: Wang, Haiyue <haiyue.wang@intel.com>; stable@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@dpdk.org
> >
> > Signed-off-by: Haiyue Wang <haiyue.wang@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/ixgbevf: add an option pflink_fullchk to get link status nowait
2019-06-07 15:56 [dpdk-stable] [PATCH v2] net/ixgbevf: add an option pflink_fullchk to get link status nowait Haiyue Wang
2019-06-07 16:08 ` Scott Daniels
2019-06-10 11:17 ` Zhang, Qi Z
@ 2019-06-11 1:29 ` Zhang, Qi Z
2 siblings, 0 replies; 5+ messages in thread
From: Zhang, Qi Z @ 2019-06-11 1:29 UTC (permalink / raw)
To: Wang, Haiyue, dev, Lu, Wenzhuo, Wang, Liang-min, daniels, ktraynor; +Cc: stable
> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Monday, June 10, 2019 7:17 PM
> To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Wang, Liang-min <liang-min.wang@intel.com>;
> daniels@research.att.com; ktraynor@redhat.com
> Cc: stable@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@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Lu, Wenzhuo
> > <wenzhuo.lu@intel.com>; Wang, Liang-min <liang-min.wang@intel.com>;
> > daniels@research.att.com; ktraynor@redhat.com
> > Cc: Wang, Haiyue <haiyue.wang@intel.com>; stable@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.
interrupts
> >
> > 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.
Checked with author, actually below patch change the default behavior on nowait case
Fixes: 91546fb62e67 ("net/ixgbevf: fix link state")
The patch roll it back and add devarg option for specific requirement, that make sense.
Applied to dpdk-next-net-intel.
Thanks
Qi
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-11 1:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-07 15:56 [dpdk-stable] [PATCH v2] net/ixgbevf: add an option pflink_fullchk to get link status nowait Haiyue Wang
2019-06-07 16:08 ` Scott Daniels
2019-06-10 11:17 ` Zhang, Qi Z
2019-06-11 1:00 ` Wang, Haiyue
2019-06-11 1:29 ` Zhang, Qi Z
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).