| Message ID | 20240613095835.598797-1-david.marchand@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] netdev-dpdk: Use LSC interrupt mode. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Thu, Jun 13, 2024 at 5:59 AM David Marchand <david.marchand@redhat.com> wrote: > > Querying link status may get delayed for an undeterministic (long) time > with mlx5 ports. This is a consequence of the mlx5 driver calling ethtool > kernel API and getting stuck on the kernel RTNL lock while some other > operation is in progress under this lock. > > One impact for long link status query is that it is called under the bond > lock taken in write mode periodically in bond_run(). > In parallel, datapath threads may block requesting to read bonding related > info (like for example in bond_check_admissibility()). > > The LSC interrupt mode is available with many DPDK drivers and is used by > default with testpmd. > > It seems safe enough to switch on this feature by default in OVS. > We keep the per interface option to disable this feature in case of an > unforeseen bug. > > Signed-off-by: David Marchand <david.marchand@redhat.com> > --- > Documentation/topics/dpdk/phy.rst | 4 ++-- > NEWS | 3 +++ > lib/netdev-dpdk.c | 7 ++++++- > vswitchd/vswitch.xml | 8 ++++---- > 4 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/Documentation/topics/dpdk/phy.rst b/Documentation/topics/dpdk/phy.rst > index efd168cba8..eefc25613d 100644 > --- a/Documentation/topics/dpdk/phy.rst > +++ b/Documentation/topics/dpdk/phy.rst > @@ -546,8 +546,8 @@ the firmware every time to fulfil this request. > > Note that not all PMD drivers support LSC interrupts. > > -The default configuration is polling mode. To set interrupt mode, option > -``dpdk-lsc-interrupt`` has to be set to ``true``. > +The default configuration is interrupt mode. To set polling mode, option > +``dpdk-lsc-interrupt`` has to be set to ``false``. > > Command to set interrupt mode for a specific interface:: > $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=true > diff --git a/NEWS b/NEWS > index 5ae0108d55..1e19beb793 100644 > --- a/NEWS > +++ b/NEWS > @@ -9,6 +9,9 @@ Post-v3.3.0 > https://github.com/openvswitch/ovs.git > - DPDK: > * OVS validated with DPDK 23.11.1. > + * Link status changes are now handled via interrupt mode if the DPDK > + driver supports it. It is possible to revert to polling mode by setting > + per interface 'dpdk-lsc-interrupt' other_config to 'false'. > > > v3.3.0 - 16 Feb 2024 > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 0fa37d5145..833d852319 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2397,7 +2397,12 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, > } > } > > - lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", false); > + lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", true); > + if (lsc_interrupt_mode && !(*info.dev_flags & RTE_ETH_DEV_INTR_LSC)) { > + VLOG_INFO("interface '%s': link status interrupt is not supported.", > + netdev_get_name(netdev)); > + lsc_interrupt_mode = false; > + } I did a quick grep of the dpdk source tree and noticed the following drivers had this define: bnxt dpaa dpaa2 ena failsafe hns3 mlx4 mlx5 netvsc tap vhost virtio This may have been an invalid methodology, but I'm noticing some drivers for very common network cards are missing. Is there a risk here of starting to print these info messages all the time for a large number of users who never set this feature in the first place? It may be preferable to continue defaulting to true, but only print the error if the flag was explicitly set. Cheers, M > if (dev->requested_lsc_interrupt_mode != lsc_interrupt_mode) { > dev->requested_lsc_interrupt_mode = lsc_interrupt_mode; > netdev_request_reconfigure(netdev); > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index 8a1b607d71..e3afb78a4e 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -4647,12 +4647,12 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ > <column name="options" key="dpdk-lsc-interrupt" > type='{"type": "boolean"}'> > <p> > - Set this value to <code>true</code> to configure interrupt mode for > - Link State Change (LSC) detection instead of poll mode for the DPDK > - interface. > + Set this value to <code>false</code> to configure poll mode for > + Link State Change (LSC) detection instead of interrupt mode for the > + DPDK interface. > </p> > <p> > - If this value is not set, poll mode is configured. > + If this value is not set, interrupt mode is configured. > </p> > <p> > This parameter has an effect only on netdev dpdk interfaces. > -- > 2.44.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Thu, Jun 13, 2024 at 4:32 PM Mike Pattrick <mkp@redhat.com> wrote: > > On Thu, Jun 13, 2024 at 5:59 AM David Marchand > <david.marchand@redhat.com> wrote: > > > > Querying link status may get delayed for an undeterministic (long) time > > with mlx5 ports. This is a consequence of the mlx5 driver calling ethtool > > kernel API and getting stuck on the kernel RTNL lock while some other > > operation is in progress under this lock. > > > > One impact for long link status query is that it is called under the bond > > lock taken in write mode periodically in bond_run(). > > In parallel, datapath threads may block requesting to read bonding related > > info (like for example in bond_check_admissibility()). > > > > The LSC interrupt mode is available with many DPDK drivers and is used by > > default with testpmd. > > > > It seems safe enough to switch on this feature by default in OVS. > > We keep the per interface option to disable this feature in case of an > > unforeseen bug. > > > > Signed-off-by: David Marchand <david.marchand@redhat.com> > > --- > > Documentation/topics/dpdk/phy.rst | 4 ++-- > > NEWS | 3 +++ > > lib/netdev-dpdk.c | 7 ++++++- > > vswitchd/vswitch.xml | 8 ++++---- > > 4 files changed, 15 insertions(+), 7 deletions(-) > > > > diff --git a/Documentation/topics/dpdk/phy.rst b/Documentation/topics/dpdk/phy.rst > > index efd168cba8..eefc25613d 100644 > > --- a/Documentation/topics/dpdk/phy.rst > > +++ b/Documentation/topics/dpdk/phy.rst > > @@ -546,8 +546,8 @@ the firmware every time to fulfil this request. > > > > Note that not all PMD drivers support LSC interrupts. > > > > -The default configuration is polling mode. To set interrupt mode, option > > -``dpdk-lsc-interrupt`` has to be set to ``true``. > > +The default configuration is interrupt mode. To set polling mode, option > > +``dpdk-lsc-interrupt`` has to be set to ``false``. > > > > Command to set interrupt mode for a specific interface:: > > $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=true > > diff --git a/NEWS b/NEWS > > index 5ae0108d55..1e19beb793 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -9,6 +9,9 @@ Post-v3.3.0 > > https://github.com/openvswitch/ovs.git > > - DPDK: > > * OVS validated with DPDK 23.11.1. > > + * Link status changes are now handled via interrupt mode if the DPDK > > + driver supports it. It is possible to revert to polling mode by setting > > + per interface 'dpdk-lsc-interrupt' other_config to 'false'. > > > > > > v3.3.0 - 16 Feb 2024 > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > > index 0fa37d5145..833d852319 100644 > > --- a/lib/netdev-dpdk.c > > +++ b/lib/netdev-dpdk.c > > @@ -2397,7 +2397,12 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, > > } > > } > > > > - lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", false); > > + lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", true); > > + if (lsc_interrupt_mode && !(*info.dev_flags & RTE_ETH_DEV_INTR_LSC)) { > > + VLOG_INFO("interface '%s': link status interrupt is not supported.", > > + netdev_get_name(netdev)); > > + lsc_interrupt_mode = false; > > + } > > I did a quick grep of the dpdk source tree and noticed the following > drivers had this define: > > bnxt dpaa dpaa2 ena failsafe hns3 mlx4 mlx5 netvsc tap vhost virtio For "normal" PCI drivers, the flag to look for is RTE_PCI_DRV_INTR_LSC $ git grep -A1 RTE_PCI_DRV_INTR_LSC lib/ethdev/ lib/ethdev/ethdev_pci.h: if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC) lib/ethdev/ethdev_pci.h- eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC; > > This may have been an invalid methodology, but I'm noticing some > drivers for very common network cards are missing. Is there a risk > here of starting to print these info messages all the time for a large > number of users who never set this feature in the first place? $ for dir in drivers/net/*; do git grep -q -E 'RTE_PCI_DRV_INTR_LSC|RTE_ETH_DEV_INTR_LSC' $dir && echo "OK for "$dir || echo "==== Missing for "$dir; done | grep -c OK 34 $ for dir in drivers/net/*; do git grep -q -E 'RTE_PCI_DRV_INTR_LSC|RTE_ETH_DEV_INTR_LSC' $dir && echo "OK for "$dir || echo "==== Missing for "$dir; done | grep -c Missing 25 The majority of net drivers has this feature. > > It may be preferable to continue defaulting to true, but only print > the error if the flag was explicitly set. I had considered this option, but I went with the simpler approach :-). Ok since you thought of it too, let me update for v2.
diff --git a/Documentation/topics/dpdk/phy.rst b/Documentation/topics/dpdk/phy.rst index efd168cba8..eefc25613d 100644 --- a/Documentation/topics/dpdk/phy.rst +++ b/Documentation/topics/dpdk/phy.rst @@ -546,8 +546,8 @@ the firmware every time to fulfil this request. Note that not all PMD drivers support LSC interrupts. -The default configuration is polling mode. To set interrupt mode, option -``dpdk-lsc-interrupt`` has to be set to ``true``. +The default configuration is interrupt mode. To set polling mode, option +``dpdk-lsc-interrupt`` has to be set to ``false``. Command to set interrupt mode for a specific interface:: $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=true diff --git a/NEWS b/NEWS index 5ae0108d55..1e19beb793 100644 --- a/NEWS +++ b/NEWS @@ -9,6 +9,9 @@ Post-v3.3.0 https://github.com/openvswitch/ovs.git - DPDK: * OVS validated with DPDK 23.11.1. + * Link status changes are now handled via interrupt mode if the DPDK + driver supports it. It is possible to revert to polling mode by setting + per interface 'dpdk-lsc-interrupt' other_config to 'false'. v3.3.0 - 16 Feb 2024 diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 0fa37d5145..833d852319 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2397,7 +2397,12 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, } } - lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", false); + lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", true); + if (lsc_interrupt_mode && !(*info.dev_flags & RTE_ETH_DEV_INTR_LSC)) { + VLOG_INFO("interface '%s': link status interrupt is not supported.", + netdev_get_name(netdev)); + lsc_interrupt_mode = false; + } if (dev->requested_lsc_interrupt_mode != lsc_interrupt_mode) { dev->requested_lsc_interrupt_mode = lsc_interrupt_mode; netdev_request_reconfigure(netdev); diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 8a1b607d71..e3afb78a4e 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -4647,12 +4647,12 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ <column name="options" key="dpdk-lsc-interrupt" type='{"type": "boolean"}'> <p> - Set this value to <code>true</code> to configure interrupt mode for - Link State Change (LSC) detection instead of poll mode for the DPDK - interface. + Set this value to <code>false</code> to configure poll mode for + Link State Change (LSC) detection instead of interrupt mode for the + DPDK interface. </p> <p> - If this value is not set, poll mode is configured. + If this value is not set, interrupt mode is configured. </p> <p> This parameter has an effect only on netdev dpdk interfaces.
Querying link status may get delayed for an undeterministic (long) time with mlx5 ports. This is a consequence of the mlx5 driver calling ethtool kernel API and getting stuck on the kernel RTNL lock while some other operation is in progress under this lock. One impact for long link status query is that it is called under the bond lock taken in write mode periodically in bond_run(). In parallel, datapath threads may block requesting to read bonding related info (like for example in bond_check_admissibility()). The LSC interrupt mode is available with many DPDK drivers and is used by default with testpmd. It seems safe enough to switch on this feature by default in OVS. We keep the per interface option to disable this feature in case of an unforeseen bug. Signed-off-by: David Marchand <david.marchand@redhat.com> --- Documentation/topics/dpdk/phy.rst | 4 ++-- NEWS | 3 +++ lib/netdev-dpdk.c | 7 ++++++- vswitchd/vswitch.xml | 8 ++++---- 4 files changed, 15 insertions(+), 7 deletions(-)