diff mbox series

[ovs-dev,v5] Configurable Link State Change (LSC) detection mode

Message ID AM3PR07MB1202A16700F61DFE2F6CBA5EEDF40@AM3PR07MB1202.eurprd07.prod.outlook.com
State Changes Requested
Delegated to: Ian Stokes
Headers show
Series [ovs-dev,v5] Configurable Link State Change (LSC) detection mode | expand

Commit Message

Róbert Mulik Feb. 15, 2018, 2:44 p.m. UTC
It is possible to change LSC detection mode to polling or interrupt mode
for DPDK interfaces. The default is polling mode. To set interrupt mode,
option dpdk-lsc-interrupt has to be set to true.

In polling mode more processor time is needed, since the OVS repeatedly reads
the link state with a short period. It can lead to packet loss for certain
systems.

In interrupt mode the hardware itself triggers an interrupt when link state
change happens, so less processing time needs for the OVS.

For detailed description and usage see the dpdk install documentation.

Signed-off-by: Robert Mulik <robert.mulik@ericsson.com>
---
 Documentation/intro/install/dpdk.rst | 35 +++++++++++++++++++++++++++++++++++
 lib/netdev-dpdk.c                    | 22 ++++++++++++++++++++--
 vswitchd/vswitch.xml                 | 17 +++++++++++++++++
 3 files changed, 72 insertions(+), 2 deletions(-)

--
1.9.1

Comments

Jan Scheurich Feb. 15, 2018, 5:10 p.m. UTC | #1
Hi Robert,

Could you please in the future add version information to the patch so that we can see the changes to previous versions? I think you can add this below the "---" line separating the commit message from the rest of the patch.

What, specifically, has changed between v4 and v5?

Thanks, Jan

> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Róbert Mulik
> Sent: Thursday, 15 February, 2018 15:45
> To: dev@openvswitch.org
> Subject: [ovs-dev] [PATCH v5] Configurable Link State Change (LSC) detection mode
> 
> It is possible to change LSC detection mode to polling or interrupt mode
> for DPDK interfaces. The default is polling mode. To set interrupt mode,
> option dpdk-lsc-interrupt has to be set to true.
> 
> In polling mode more processor time is needed, since the OVS repeatedly reads
> the link state with a short period. It can lead to packet loss for certain
> systems.
> 
> In interrupt mode the hardware itself triggers an interrupt when link state
> change happens, so less processing time needs for the OVS.
> 
> For detailed description and usage see the dpdk install documentation.
> 
> Signed-off-by: Robert Mulik <robert.mulik@ericsson.com>
> ---
>  Documentation/intro/install/dpdk.rst | 35 +++++++++++++++++++++++++++++++++++
>  lib/netdev-dpdk.c                    | 22 ++++++++++++++++++++--
>  vswitchd/vswitch.xml                 | 17 +++++++++++++++++
>  3 files changed, 72 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/intro/install/dpdk.rst b/Documentation/intro/install/dpdk.rst
> index ed358d5..e3872e7 100644
> --- a/Documentation/intro/install/dpdk.rst
> +++ b/Documentation/intro/install/dpdk.rst
> @@ -628,6 +628,41 @@ The average number of packets per output batch can be checked in PMD stats::
> 
>      $ ovs-appctl dpif-netdev/pmd-stats-show
> 
> +Link State Change (LSC) detection configuration
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +There are two methods to get the information when Link State Change (LSC)
> +happens on a network interface: by polling or interrupt.
> +
> +With the polling method, the main process checks the link state on a
> +fixed interval. This fixed interval determines how fast a link change is
> +detected. Another problem with the poll mode is that on some hardware a
> +polling cycle takes too much time, which (in the end) leads to packet loss
> +for certain systems.
> +
> +If interrupts are used to get LSC information, the hardware itself triggers
> +an interrupt when link state change happens, the interrupt thread wakes up
> +from sleep, updates the information, and goes back to sleep mode. When no link
> +state change happens (most of the time), the thread remains in sleep mode and
> +doesn`t use processor time at all. The disadvantage of this method is that
> +when an interrupt happens, the processor has to handle it immediately, so it
> +puts the currently running process to background, handles the interrupt, and
> +takes the background process back.
> +
> +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``.
> +
> +Command to set interrupt mode for a specific interface::
> +    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=true
> +
> +Command to set polling mode for a specific interface::
> +    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=false
> +
> +Command to remove ``dpdk-lsc-interrupt`` option::
> +    $ ovs-vsctl remove interface <iface_name> options dpdk-lsc-interrupt
> +
>  Limitations
>  ------------
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 94fb163..d092ef1 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -433,6 +433,12 @@ struct netdev_dpdk {
>          /* DPDK-ETH hardware offload features,
>           * from the enum set 'dpdk_hw_ol_features' */
>          uint32_t hw_ol_features;
> +
> +        /* Properties for link state change detection mode.
> +         * If lsc_interrupt_mode is set to false, poll mode is used,
> +         * otherwise interrupt mode is used. */
> +        bool requested_lsc_interrupt_mode;
> +        bool lsc_interrupt_mode;
>      );
> 
>      PADDED_MEMBERS(CACHE_LINE_SIZE,
> @@ -686,12 +692,14 @@ dpdk_watchdog(void *dummy OVS_UNUSED)
>  }
> 
>  static int
> -dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int n_rxq, int n_txq)
> +dpdk_eth_dev_port_config(struct netdev_dpdk *dev, int n_rxq, int n_txq)
>  {
>      int diag = 0;
>      int i;
>      struct rte_eth_conf conf = port_conf;
> 
> +    conf.intr_conf.lsc = dev->lsc_interrupt_mode;
> +
>      /* For some NICs (e.g. Niantic), scatter_rx mode needs to be explicitly
>       * enabled. */
>      if (dev->mtu > ETHER_MTU) {
> @@ -801,7 +809,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>      n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq);
>      n_txq = MIN(info.max_tx_queues, dev->up.n_txq);
> 
> -    diag = dpdk_eth_dev_queue_setup(dev, n_rxq, n_txq);
> +    diag = dpdk_eth_dev_port_config(dev, n_rxq, n_txq);
>      if (diag) {
>          VLOG_ERR("Interface %s(rxq:%d txq:%d) configure error: %s",
>                   dev->up.name, n_rxq, n_txq, rte_strerror(-diag));
> @@ -897,6 +905,7 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no,
>      dev->flags = 0;
>      dev->requested_mtu = ETHER_MTU;
>      dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);
> +    dev->requested_lsc_interrupt_mode = 0;
>      ovsrcu_index_init(&dev->vid, -1);
>      dev->vhost_reconfigured = false;
>      dev->attached = false;
> @@ -1520,6 +1529,12 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
>          goto out;
>      }
> 
> +    bool lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", false);
> +    if (dev->requested_lsc_interrupt_mode != lsc_interrupt_mode) {
> +        dev->requested_lsc_interrupt_mode = lsc_interrupt_mode;
> +        netdev_request_reconfigure(netdev);
> +    }
> +
>      rx_fc_en = smap_get_bool(args, "rx-flow-ctrl", false);
>      tx_fc_en = smap_get_bool(args, "tx-flow-ctrl", false);
>      autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);
> @@ -3546,6 +3561,7 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
>      if (netdev->n_txq == dev->requested_n_txq
>          && netdev->n_rxq == dev->requested_n_rxq
>          && dev->mtu == dev->requested_mtu
> +        && dev->lsc_interrupt_mode == dev->requested_lsc_interrupt_mode
>          && dev->rxq_size == dev->requested_rxq_size
>          && dev->txq_size == dev->requested_txq_size
>          && dev->socket_id == dev->requested_socket_id) {
> @@ -3561,6 +3577,8 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
>          goto out;
>      }
> 
> +    dev->lsc_interrupt_mode = dev->requested_lsc_interrupt_mode;
> +
>      netdev->n_txq = dev->requested_n_txq;
>      netdev->n_rxq = dev->requested_n_rxq;
> 
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 0c6a43d..3c9e637 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -3631,6 +3631,23 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
>        </column>
>      </group>
> 
> +    <group title="Link State Change detection mode">
> +      <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.
> +        </p>
> +        <p>
> +          If this value is not set, poll mode is configured.
> +        </p>
> +        <p>
> +          This parameter has an effect only on netdev dpdk interfaces.
> +        </p>
> +      </column>
> +    </group>
> +
>      <group title="Common Columns">
>        The overall purpose of these columns is described under <code>Common
>        Columns</code> at the beginning of this document.
> --
> 1.9.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Róbert Mulik Feb. 16, 2018, 3:46 p.m. UTC | #2
Hi Jan,

This is the diff between v4 and v5.

Regards,
Robert

diff --git a/Documentation/intro/install/dpdk.rst b/Documentation/intro/install/dpdk.rst
index 14a6684..e3872e7 100644
--- a/Documentation/intro/install/dpdk.rst
+++ b/Documentation/intro/install/dpdk.rst
@@ -634,47 +634,34 @@ Link State Change (LSC) detection configuration
 There are two methods to get the information when Link State Change (LSC)
 happens on a network interface: by polling or interrupt.

-With polling method, a process is running in the background and repeatedly
-reads the link state with a short period. It continuously needs processor time
-and between 2 reading periods it can`t see the link state change, therefore
-the reaction time depends on the polling period. With higher rate, more
-processor time is needed. Another problem with the poll mode is that on some
-hardware a polling cycle takes too much time, which (in the end) leads to
-packet loss for certain systems.
+With the polling method, the main process checks the link state on a
+fixed interval. This fixed interval determines how fast a link change is
+detected. Another problem with the poll mode is that on some hardware a
+polling cycle takes too much time, which (in the end) leads to packet loss
+for certain systems.

 If interrupts are used to get LSC information, the hardware itself triggers
-an interrupt when link state change happens, the thread wakes up from sleep,
-updates the information, and goes back to sleep mode. When no link state
-change happens (most of the time), the thread remains in sleep mode and
+an interrupt when link state change happens, the interrupt thread wakes up
+from sleep, updates the information, and goes back to sleep mode. When no link
+state change happens (most of the time), the thread remains in sleep mode and
 doesn`t use processor time at all. The disadvantage of this method is that
-when interrupt happens, the processor has to handle it immediately, so it
+when an interrupt happens, the processor has to handle it immediately, so it
 puts the currently running process to background, handles the interrupt, and
-takes the background process back. Another disadvantage is that some hardware
-can`t be configured to generate LSC interrupts.
+takes the background process back.

-The default configuration is polling mode. To set interrupt mode, option
-dpdk-lsc-interrupt has to be set to true.
-
-Global settings
-
-Command to set interrupt mode for all interfaces:
-ovs-vsctl set Open_vSwitch . other_config:dpdk-lsc-interrupt=true
+Note that not all PMD drivers support LSC interrupts.

-Command to set polling mode for all interfaces:
-ovs-vsctl set Open_vSwitch . other_config:dpdk-lsc-interrupt=false
-or:
-ovs-vsctl remove Open_vSwitch . other_config dpdk-lsc-interrupt
-
-Interface specific settings (override global settings)
+The default configuration is polling mode. To set interrupt mode, option
+``dpdk-lsc-interrupt`` has to be set to ``true``.

-Command to set interrupt mode for a specific interface:
-ovs-vsctl set interface <interface_name> options:dpdk-lsc-interrupt=true
+Command to set interrupt mode for a specific interface::
+    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=true

-Command to set polling mode for a specific interface:
-ovs-vsctl set interface <interface_name> options:dpdk-lsc-interrupt=false
+Command to set polling mode for a specific interface::
+    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=false

-Command to reset to globally defined mode for a specific interface:
-ovs-vsctl remove interface <interface_name> options dpdk-lsc-interrupt
+Command to remove ``dpdk-lsc-interrupt`` option::
+    $ ovs-vsctl remove interface <iface_name> options dpdk-lsc-interrupt

 Limitations
 ------------
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 73d0d4b..d092ef1 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -148,7 +148,7 @@ typedef uint16_t dpdk_port_t;
 #define VHOST_ENQ_RETRY_NUM 8
 #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)

-static struct rte_eth_conf port_conf = {
+static const struct rte_eth_conf port_conf = {
     .rxmode = {
         .mq_mode = ETH_MQ_RX_RSS,
         .split_hdr_size = 0,
@@ -167,10 +167,6 @@ static struct rte_eth_conf port_conf = {
     .txmode = {
         .mq_mode = ETH_MQ_TX_NONE,
     },
-    .intr_conf = {
-        /* LSC interrupt mode disabled, polling mode used. */
-        .lsc = 0,
-    },
 };

 /*
@@ -469,18 +465,6 @@ int netdev_dpdk_get_vid(const struct netdev_dpdk *dev);
 struct ingress_policer *
 netdev_dpdk_get_ingress_policer(const struct netdev_dpdk *dev);

-void
-netdev_dpdk_set_default_lsc_detect_mode(bool enabled)
-{
-    port_conf.intr_conf.lsc = enabled;
-}
-
-bool
-netdev_dpdk_get_def_lsc_int_mode_enabled(void)
-{
-    return port_conf.intr_conf.lsc;
-}
-
 static bool
 is_dpdk_class(const struct netdev_class *class)
 {
@@ -921,8 +905,7 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no,
     dev->flags = 0;
     dev->requested_mtu = ETHER_MTU;
     dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);
-    dev->requested_lsc_interrupt_mode =
-        netdev_dpdk_get_def_lsc_int_mode_enabled();
+    dev->requested_lsc_interrupt_mode = 0;
     ovsrcu_index_init(&dev->vid, -1);
     dev->vhost_reconfigured = false;
     dev->attached = false;
@@ -1546,8 +1529,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
         goto out;
     }

-    bool lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt",
-                              netdev_dpdk_get_def_lsc_int_mode_enabled());
+    bool lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", 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/lib/netdev-dpdk.h b/lib/netdev-dpdk.h
index 8eb51bc..b7d02a7 100644
--- a/lib/netdev-dpdk.h
+++ b/lib/netdev-dpdk.h
@@ -27,8 +27,6 @@ struct dp_packet;

 void netdev_dpdk_register(void);
 void free_dpdk_buf(struct dp_packet *);
-void netdev_dpdk_set_def_lsc_int_mode_enabled(bool enabled);
-bool netdev_dpdk_get_def_lsc_int_mode_enabled(void);

 #else

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 87608a5..f44f950 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -68,9 +68,6 @@
 #include "lib/vswitch-idl.h"
 #include "xenserver.h"
 #include "vlan-bitmap.h"
-#ifdef DPDK_NETDEV
-#include "./lib/netdev-provider.h"
-#endif

 VLOG_DEFINE_THIS_MODULE(bridge);

@@ -604,11 +601,6 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
     ofproto_set_vlan_limit(smap_get_int(&ovs_cfg->other_config, "vlan-limit",
                                        LEGACY_MAX_VLAN_HEADERS));

-#ifdef DPDK_NETDEV
-    netdev_dpdk_set_def_lsc_int_mode_enabled(
-        smap_get_bool(&ovs_cfg->other_config, "dpdk-lsc-interrupt", false));
-#endif
-
     ofproto_set_threads(
         smap_get_int(&ovs_cfg->other_config, "n-handler-threads", 0),
         smap_get_int(&ovs_cfg->other_config, "n-revalidator-threads", 0));
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 91a9003..3c9e637 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -320,29 +320,6 @@
         </p>
       </column>

-      <column name="other_config" 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 DPDK
-          interfaces.
-        </p>
-        <p>
-          The default value is <code>false</code>.
-        </p>
-        <p>
-          If this value is <code>false</code> at startup, poll mode is used for
-          all netdev dpdk interfaces.
-        </p>
-        <p>
-          This value can be overridden for a specific interface in the options
-          section of that interface.
-        </p>
-        <p>
-          This parameter has an effect only on netdev dpdk interfaces.
-        </p>
-      </column>
-
       <column name="other_config" key="dpdk-extra"
               type='{"type": "string"}'>
         <p>
@@ -3663,12 +3640,7 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
           interface.
         </p>
         <p>
-          If this value is not set, the value is taken from the global
-          settings.
-        </p>
-        <p>
-          If this value is set, the global LSC interrupt settings are
-          overridden.
+          If this value is not set, poll mode is configured.
         </p>
         <p>
           This parameter has an effect only on netdev dpdk interfaces.
Róbert Mulik Feb. 16, 2018, 3:53 p.m. UTC | #3
Hi Jan,

Specifically, in v5 the global configuration was removed from the code, and the documentation were updated with this information, and changes were done in the docs based on the comments by Eelco.

Regards,
Robert

-----Original Message-----
From: Jan Scheurich 
Sent: Thursday, February 15, 2018 6:10 PM
To: Róbert Mulik <robert.mulik@ericsson.com>; dev@openvswitch.org
Subject: RE: [ovs-dev] [PATCH v5] Configurable Link State Change (LSC) detection mode

Hi Robert,

Could you please in the future add version information to the patch so that we can see the changes to previous versions? I think you can add this below the "---" line separating the commit message from the rest of the patch.

What, specifically, has changed between v4 and v5?

Thanks, Jan

> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Róbert Mulik
> Sent: Thursday, 15 February, 2018 15:45
> To: dev@openvswitch.org
> Subject: [ovs-dev] [PATCH v5] Configurable Link State Change (LSC) detection mode
> 
> It is possible to change LSC detection mode to polling or interrupt mode
> for DPDK interfaces. The default is polling mode. To set interrupt mode,
> option dpdk-lsc-interrupt has to be set to true.
> 
> In polling mode more processor time is needed, since the OVS repeatedly reads
> the link state with a short period. It can lead to packet loss for certain
> systems.
> 
> In interrupt mode the hardware itself triggers an interrupt when link state
> change happens, so less processing time needs for the OVS.
> 
> For detailed description and usage see the dpdk install documentation.
> 
> Signed-off-by: Robert Mulik <robert.mulik@ericsson.com>
> ---
>  Documentation/intro/install/dpdk.rst | 35 +++++++++++++++++++++++++++++++++++
>  lib/netdev-dpdk.c                    | 22 ++++++++++++++++++++--
>  vswitchd/vswitch.xml                 | 17 +++++++++++++++++
>  3 files changed, 72 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/intro/install/dpdk.rst b/Documentation/intro/install/dpdk.rst
> index ed358d5..e3872e7 100644
> --- a/Documentation/intro/install/dpdk.rst
> +++ b/Documentation/intro/install/dpdk.rst
> @@ -628,6 +628,41 @@ The average number of packets per output batch can be checked in PMD stats::
> 
>      $ ovs-appctl dpif-netdev/pmd-stats-show
> 
> +Link State Change (LSC) detection configuration
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +There are two methods to get the information when Link State Change (LSC)
> +happens on a network interface: by polling or interrupt.
> +
> +With the polling method, the main process checks the link state on a
> +fixed interval. This fixed interval determines how fast a link change is
> +detected. Another problem with the poll mode is that on some hardware a
> +polling cycle takes too much time, which (in the end) leads to packet loss
> +for certain systems.
> +
> +If interrupts are used to get LSC information, the hardware itself triggers
> +an interrupt when link state change happens, the interrupt thread wakes up
> +from sleep, updates the information, and goes back to sleep mode. When no link
> +state change happens (most of the time), the thread remains in sleep mode and
> +doesn`t use processor time at all. The disadvantage of this method is that
> +when an interrupt happens, the processor has to handle it immediately, so it
> +puts the currently running process to background, handles the interrupt, and
> +takes the background process back.
> +
> +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``.
> +
> +Command to set interrupt mode for a specific interface::
> +    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=true
> +
> +Command to set polling mode for a specific interface::
> +    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=false
> +
> +Command to remove ``dpdk-lsc-interrupt`` option::
> +    $ ovs-vsctl remove interface <iface_name> options dpdk-lsc-interrupt
> +
>  Limitations
>  ------------
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 94fb163..d092ef1 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -433,6 +433,12 @@ struct netdev_dpdk {
>          /* DPDK-ETH hardware offload features,
>           * from the enum set 'dpdk_hw_ol_features' */
>          uint32_t hw_ol_features;
> +
> +        /* Properties for link state change detection mode.
> +         * If lsc_interrupt_mode is set to false, poll mode is used,
> +         * otherwise interrupt mode is used. */
> +        bool requested_lsc_interrupt_mode;
> +        bool lsc_interrupt_mode;
>      );
> 
>      PADDED_MEMBERS(CACHE_LINE_SIZE,
> @@ -686,12 +692,14 @@ dpdk_watchdog(void *dummy OVS_UNUSED)
>  }
> 
>  static int
> -dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int n_rxq, int n_txq)
> +dpdk_eth_dev_port_config(struct netdev_dpdk *dev, int n_rxq, int n_txq)
>  {
>      int diag = 0;
>      int i;
>      struct rte_eth_conf conf = port_conf;
> 
> +    conf.intr_conf.lsc = dev->lsc_interrupt_mode;
> +
>      /* For some NICs (e.g. Niantic), scatter_rx mode needs to be explicitly
>       * enabled. */
>      if (dev->mtu > ETHER_MTU) {
> @@ -801,7 +809,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>      n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq);
>      n_txq = MIN(info.max_tx_queues, dev->up.n_txq);
> 
> -    diag = dpdk_eth_dev_queue_setup(dev, n_rxq, n_txq);
> +    diag = dpdk_eth_dev_port_config(dev, n_rxq, n_txq);
>      if (diag) {
>          VLOG_ERR("Interface %s(rxq:%d txq:%d) configure error: %s",
>                   dev->up.name, n_rxq, n_txq, rte_strerror(-diag));
> @@ -897,6 +905,7 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no,
>      dev->flags = 0;
>      dev->requested_mtu = ETHER_MTU;
>      dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);
> +    dev->requested_lsc_interrupt_mode = 0;
>      ovsrcu_index_init(&dev->vid, -1);
>      dev->vhost_reconfigured = false;
>      dev->attached = false;
> @@ -1520,6 +1529,12 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
>          goto out;
>      }
> 
> +    bool lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", false);
> +    if (dev->requested_lsc_interrupt_mode != lsc_interrupt_mode) {
> +        dev->requested_lsc_interrupt_mode = lsc_interrupt_mode;
> +        netdev_request_reconfigure(netdev);
> +    }
> +
>      rx_fc_en = smap_get_bool(args, "rx-flow-ctrl", false);
>      tx_fc_en = smap_get_bool(args, "tx-flow-ctrl", false);
>      autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);
> @@ -3546,6 +3561,7 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
>      if (netdev->n_txq == dev->requested_n_txq
>          && netdev->n_rxq == dev->requested_n_rxq
>          && dev->mtu == dev->requested_mtu
> +        && dev->lsc_interrupt_mode == dev->requested_lsc_interrupt_mode
>          && dev->rxq_size == dev->requested_rxq_size
>          && dev->txq_size == dev->requested_txq_size
>          && dev->socket_id == dev->requested_socket_id) {
> @@ -3561,6 +3577,8 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
>          goto out;
>      }
> 
> +    dev->lsc_interrupt_mode = dev->requested_lsc_interrupt_mode;
> +
>      netdev->n_txq = dev->requested_n_txq;
>      netdev->n_rxq = dev->requested_n_rxq;
> 
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 0c6a43d..3c9e637 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -3631,6 +3631,23 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
>        </column>
>      </group>
> 
> +    <group title="Link State Change detection mode">
> +      <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.
> +        </p>
> +        <p>
> +          If this value is not set, poll mode is configured.
> +        </p>
> +        <p>
> +          This parameter has an effect only on netdev dpdk interfaces.
> +        </p>
> +      </column>
> +    </group>
> +
>      <group title="Common Columns">
>        The overall purpose of these columns is described under <code>Common
>        Columns</code> at the beginning of this document.
> --
> 1.9.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Eelco Chaudron Feb. 20, 2018, 2:16 p.m. UTC | #4
Hi Robert,

I did not see a reply to my v4 reply, so I ask this question again:
     Did you try what happens if the PMD does not support LSC and its 
enabled?

Also, it would be easier to understand what you are going to change if 
you answer the questions in the review emails.

For example, this patch is missing the suggestion on adding the LSC 
config in netdev_dpdk_get_config(), as suggested by Ilya.

I did a quick visual review below, and will wait for v6 with the above 
change, and do another test run.

//Eelco


On 15/02/18 15:44, Róbert Mulik wrote:
> It is possible to change LSC detection mode to polling or interrupt mode
> for DPDK interfaces. The default is polling mode. To set interrupt mode,
> option dpdk-lsc-interrupt has to be set to true.
>
> In polling mode more processor time is needed, since the OVS repeatedly reads
> the link state with a short period. It can lead to packet loss for certain
> systems.
>
> In interrupt mode the hardware itself triggers an interrupt when link state
> change happens, so less processing time needs for the OVS.
>
> For detailed description and usage see the dpdk install documentation.
>
> Signed-off-by: Robert Mulik <robert.mulik@ericsson.com>
> ---
>   Documentation/intro/install/dpdk.rst | 35 +++++++++++++++++++++++++++++++++++
>   lib/netdev-dpdk.c                    | 22 ++++++++++++++++++++--
>   vswitchd/vswitch.xml                 | 17 +++++++++++++++++
>   3 files changed, 72 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/intro/install/dpdk.rst b/Documentation/intro/install/dpdk.rst
> index ed358d5..e3872e7 100644
> --- a/Documentation/intro/install/dpdk.rst
> +++ b/Documentation/intro/install/dpdk.rst
> @@ -628,6 +628,41 @@ The average number of packets per output batch can be checked in PMD stats::
>
>       $ ovs-appctl dpif-netdev/pmd-stats-show
>
> +Link State Change (LSC) detection configuration
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +There are two methods to get the information when Link State Change (LSC)
> +happens on a network interface: by polling or interrupt.
> +
> +With the polling method, the main process checks the link state on a
> +fixed interval. This fixed interval determines how fast a link change is
> +detected. Another problem with the poll mode is that on some hardware a
> +polling cycle takes too much time, which (in the end) leads to packet loss
> +for certain systems.
Maybe we should stick to facts of the implementation, and not known 
bugs? So remove the "Another problem..."
> +
> +If interrupts are used to get LSC information, the hardware itself triggers
> +an interrupt when link state change happens, the interrupt thread wakes up
> +from sleep, updates the information, and goes back to sleep mode. When no link
> +state change happens (most of the time), the thread remains in sleep mode and
> +doesn`t use processor time at all. The disadvantage of this method is that
> +when an interrupt happens, the processor has to handle it immediately, so it
> +puts the currently running process to background, handles the interrupt, and
> +takes the background process back.
> +
> +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``.
> +
> +Command to set interrupt mode for a specific interface::
> +    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=true
> +
> +Command to set polling mode for a specific interface::
> +    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=false
> +
> +Command to remove ``dpdk-lsc-interrupt`` option::
> +    $ ovs-vsctl remove interface <iface_name> options dpdk-lsc-interrupt
> +
>   Limitations
>   ------------
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 94fb163..d092ef1 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -433,6 +433,12 @@ struct netdev_dpdk {
>           /* DPDK-ETH hardware offload features,
>            * from the enum set 'dpdk_hw_ol_features' */
>           uint32_t hw_ol_features;
> +
> +        /* Properties for link state change detection mode.
> +         * If lsc_interrupt_mode is set to false, poll mode is used,
> +         * otherwise interrupt mode is used. */
> +        bool requested_lsc_interrupt_mode;
> +        bool lsc_interrupt_mode;
>       );
>
>       PADDED_MEMBERS(CACHE_LINE_SIZE,
> @@ -686,12 +692,14 @@ dpdk_watchdog(void *dummy OVS_UNUSED)
>   }
>
>   static int
> -dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int n_rxq, int n_txq)
> +dpdk_eth_dev_port_config(struct netdev_dpdk *dev, int n_rxq, int n_txq)
>   {
>       int diag = 0;
>       int i;
>       struct rte_eth_conf conf = port_conf;
>
> +    conf.intr_conf.lsc = dev->lsc_interrupt_mode;
> +
>       /* For some NICs (e.g. Niantic), scatter_rx mode needs to be explicitly
>        * enabled. */
>       if (dev->mtu > ETHER_MTU) {
> @@ -801,7 +809,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>       n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq);
>       n_txq = MIN(info.max_tx_queues, dev->up.n_txq);
>
> -    diag = dpdk_eth_dev_queue_setup(dev, n_rxq, n_txq);
> +    diag = dpdk_eth_dev_port_config(dev, n_rxq, n_txq);
>       if (diag) {
>           VLOG_ERR("Interface %s(rxq:%d txq:%d) configure error: %s",
>                    dev->up.name, n_rxq, n_txq, rte_strerror(-diag));
> @@ -897,6 +905,7 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no,
>       dev->flags = 0;
>       dev->requested_mtu = ETHER_MTU;
>       dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);
> +    dev->requested_lsc_interrupt_mode = 0;
>       ovsrcu_index_init(&dev->vid, -1);
>       dev->vhost_reconfigured = false;
>       dev->attached = false;
> @@ -1520,6 +1529,12 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
>           goto out;
>       }
>
> +    bool lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", false);
> +    if (dev->requested_lsc_interrupt_mode != lsc_interrupt_mode) {
> +        dev->requested_lsc_interrupt_mode = lsc_interrupt_mode;
> +        netdev_request_reconfigure(netdev);
> +    }
> +
>       rx_fc_en = smap_get_bool(args, "rx-flow-ctrl", false);
>       tx_fc_en = smap_get_bool(args, "tx-flow-ctrl", false);
>       autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);
> @@ -3546,6 +3561,7 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
>       if (netdev->n_txq == dev->requested_n_txq
>           && netdev->n_rxq == dev->requested_n_rxq
>           && dev->mtu == dev->requested_mtu
> +        && dev->lsc_interrupt_mode == dev->requested_lsc_interrupt_mode
>           && dev->rxq_size == dev->requested_rxq_size
>           && dev->txq_size == dev->requested_txq_size
>           && dev->socket_id == dev->requested_socket_id) {
> @@ -3561,6 +3577,8 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
>           goto out;
>       }
>
> +    dev->lsc_interrupt_mode = dev->requested_lsc_interrupt_mode;
> +
>       netdev->n_txq = dev->requested_n_txq;
>       netdev->n_rxq = dev->requested_n_rxq;
>
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 0c6a43d..3c9e637 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -3631,6 +3631,23 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
>         </column>
>       </group>
>
> +    <group title="Link State Change detection mode">
> +      <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.
> +        </p>
> +        <p>
> +          If this value is not set, poll mode is configured.
> +        </p>
> +        <p>
> +          This parameter has an effect only on netdev dpdk interfaces.
> +        </p>
> +      </column>
> +    </group>
> +
>       <group title="Common Columns">
>         The overall purpose of these columns is described under <code>Common
>         Columns</code> at the beginning of this document.
> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Róbert Mulik Feb. 21, 2018, 10:33 a.m. UTC | #5
> -----Original Message-----

> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-

> bounces@openvswitch.org] On Behalf Of Eelco Chaudron

> Sent: Tuesday, February 20, 2018 3:17 PM

> To: ovs-dev@openvswitch.org

> Subject: Re: [ovs-dev] [PATCH v5] Configurable Link State Change (LSC)

> detection mode

> 

> Hi Robert,

> 

> I did not see a reply to my v4 reply, so I ask this question again:

>      Did you try what happens if the PMD does not support LSC and its

> enabled?

Unfortunately I don't have any cards which don't support LSC, so I couldn't test it
> 

> Also, it would be easier to understand what you are going to change if

> you answer the questions in the review emails.

> 

> For example, this patch is missing the suggestion on adding the LSC

> config in netdev_dpdk_get_config(), as suggested by Ilya.

As I understand the reason for this function would be to avoid the confusion when
both global and local configs are configured for an interface, especially if we go with
the implementation where the global config doesn't take effect without service
restart.

Since the global config is not implemented in this patch and the default value remains
the same as it always was (poll mode) and also the documentation tells what the default
value is, I don't really see the importance of this implementation.
> 

> I did a quick visual review below, and will wait for v6 with the above

> change, and do another test run.

> 

> //Eelco

> 

> 

> On 15/02/18 15:44, Róbert Mulik wrote:

> > It is possible to change LSC detection mode to polling or interrupt mode

> > for DPDK interfaces. The default is polling mode. To set interrupt mode,

> > option dpdk-lsc-interrupt has to be set to true.

> >

> > In polling mode more processor time is needed, since the OVS repeatedly

> reads

> > the link state with a short period. It can lead to packet loss for certain

> > systems.

> >

> > In interrupt mode the hardware itself triggers an interrupt when link state

> > change happens, so less processing time needs for the OVS.

> >

> > For detailed description and usage see the dpdk install documentation.

> >

> > Signed-off-by: Robert Mulik <robert.mulik@ericsson.com>

> > ---

> >   Documentation/intro/install/dpdk.rst | 35

> +++++++++++++++++++++++++++++++++++

> >   lib/netdev-dpdk.c                    | 22 ++++++++++++++++++++--

> >   vswitchd/vswitch.xml                 | 17 +++++++++++++++++

> >   3 files changed, 72 insertions(+), 2 deletions(-)

> >

> > diff --git a/Documentation/intro/install/dpdk.rst

> b/Documentation/intro/install/dpdk.rst

> > index ed358d5..e3872e7 100644

> > --- a/Documentation/intro/install/dpdk.rst

> > +++ b/Documentation/intro/install/dpdk.rst

> > @@ -628,6 +628,41 @@ The average number of packets per output batch

> can be checked in PMD stats::

> >

> >       $ ovs-appctl dpif-netdev/pmd-stats-show

> >

> > +Link State Change (LSC) detection configuration

> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> > +

> > +There are two methods to get the information when Link State Change

> (LSC)

> > +happens on a network interface: by polling or interrupt.

> > +

> > +With the polling method, the main process checks the link state on a

> > +fixed interval. This fixed interval determines how fast a link change is

> > +detected. Another problem with the poll mode is that on some hardware

> a

> > +polling cycle takes too much time, which (in the end) leads to packet loss

> > +for certain systems.

> Maybe we should stick to facts of the implementation, and not known

> bugs? So remove the "Another problem..."

> > +

> > +If interrupts are used to get LSC information, the hardware itself triggers

> > +an interrupt when link state change happens, the interrupt thread wakes

> up

> > +from sleep, updates the information, and goes back to sleep mode. When

> no link

> > +state change happens (most of the time), the thread remains in sleep

> mode and

> > +doesn`t use processor time at all. The disadvantage of this method is that

> > +when an interrupt happens, the processor has to handle it immediately,

> so it

> > +puts the currently running process to background, handles the interrupt,

> and

> > +takes the background process back.

> > +

> > +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``.

> > +

> > +Command to set interrupt mode for a specific interface::

> > +    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=true

> > +

> > +Command to set polling mode for a specific interface::

> > +    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=false

> > +

> > +Command to remove ``dpdk-lsc-interrupt`` option::

> > +    $ ovs-vsctl remove interface <iface_name> options dpdk-lsc-interrupt

> > +

> >   Limitations

> >   ------------

> >

> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c

> > index 94fb163..d092ef1 100644

> > --- a/lib/netdev-dpdk.c

> > +++ b/lib/netdev-dpdk.c

> > @@ -433,6 +433,12 @@ struct netdev_dpdk {

> >           /* DPDK-ETH hardware offload features,

> >            * from the enum set 'dpdk_hw_ol_features' */

> >           uint32_t hw_ol_features;

> > +

> > +        /* Properties for link state change detection mode.

> > +         * If lsc_interrupt_mode is set to false, poll mode is used,

> > +         * otherwise interrupt mode is used. */

> > +        bool requested_lsc_interrupt_mode;

> > +        bool lsc_interrupt_mode;

> >       );

> >

> >       PADDED_MEMBERS(CACHE_LINE_SIZE,

> > @@ -686,12 +692,14 @@ dpdk_watchdog(void *dummy OVS_UNUSED)

> >   }

> >

> >   static int

> > -dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int n_rxq, int

> n_txq)

> > +dpdk_eth_dev_port_config(struct netdev_dpdk *dev, int n_rxq, int

> n_txq)

> >   {

> >       int diag = 0;

> >       int i;

> >       struct rte_eth_conf conf = port_conf;

> >

> > +    conf.intr_conf.lsc = dev->lsc_interrupt_mode;

> > +

> >       /* For some NICs (e.g. Niantic), scatter_rx mode needs to be explicitly

> >        * enabled. */

> >       if (dev->mtu > ETHER_MTU) {

> > @@ -801,7 +809,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)

> >       n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq);

> >       n_txq = MIN(info.max_tx_queues, dev->up.n_txq);

> >

> > -    diag = dpdk_eth_dev_queue_setup(dev, n_rxq, n_txq);

> > +    diag = dpdk_eth_dev_port_config(dev, n_rxq, n_txq);

> >       if (diag) {

> >           VLOG_ERR("Interface %s(rxq:%d txq:%d) configure error: %s",

> >                    dev->up.name, n_rxq, n_txq, rte_strerror(-diag));

> > @@ -897,6 +905,7 @@ common_construct(struct netdev *netdev,

> dpdk_port_t port_no,

> >       dev->flags = 0;

> >       dev->requested_mtu = ETHER_MTU;

> >       dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);

> > +    dev->requested_lsc_interrupt_mode = 0;

> >       ovsrcu_index_init(&dev->vid, -1);

> >       dev->vhost_reconfigured = false;

> >       dev->attached = false;

> > @@ -1520,6 +1529,12 @@ netdev_dpdk_set_config(struct netdev

> *netdev, const struct smap *args,

> >           goto out;

> >       }

> >

> > +    bool lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt",

> false);

> > +    if (dev->requested_lsc_interrupt_mode != lsc_interrupt_mode) {

> > +        dev->requested_lsc_interrupt_mode = lsc_interrupt_mode;

> > +        netdev_request_reconfigure(netdev);

> > +    }

> > +

> >       rx_fc_en = smap_get_bool(args, "rx-flow-ctrl", false);

> >       tx_fc_en = smap_get_bool(args, "tx-flow-ctrl", false);

> >       autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);

> > @@ -3546,6 +3561,7 @@ netdev_dpdk_reconfigure(struct netdev

> *netdev)

> >       if (netdev->n_txq == dev->requested_n_txq

> >           && netdev->n_rxq == dev->requested_n_rxq

> >           && dev->mtu == dev->requested_mtu

> > +        && dev->lsc_interrupt_mode == dev-

> >requested_lsc_interrupt_mode

> >           && dev->rxq_size == dev->requested_rxq_size

> >           && dev->txq_size == dev->requested_txq_size

> >           && dev->socket_id == dev->requested_socket_id) {

> > @@ -3561,6 +3577,8 @@ netdev_dpdk_reconfigure(struct netdev

> *netdev)

> >           goto out;

> >       }

> >

> > +    dev->lsc_interrupt_mode = dev->requested_lsc_interrupt_mode;

> > +

> >       netdev->n_txq = dev->requested_n_txq;

> >       netdev->n_rxq = dev->requested_n_rxq;

> >

> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml

> > index 0c6a43d..3c9e637 100644

> > --- a/vswitchd/vswitch.xml

> > +++ b/vswitchd/vswitch.xml

> > @@ -3631,6 +3631,23 @@ ovs-vsctl add-port br0 p0 -- set Interface p0

> type=patch options:peer=p1 \

> >         </column>

> >       </group>

> >

> > +    <group title="Link State Change detection mode">

> > +      <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.

> > +        </p>

> > +        <p>

> > +          If this value is not set, poll mode is configured.

> > +        </p>

> > +        <p>

> > +          This parameter has an effect only on netdev dpdk interfaces.

> > +        </p>

> > +      </column>

> > +    </group>

> > +

> >       <group title="Common Columns">

> >         The overall purpose of these columns is described under

> <code>Common

> >         Columns</code> at the beginning of this document.

> > --

> > 1.9.1

> >

> > _______________________________________________

> > dev mailing list

> > dev@openvswitch.org

> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev

> 

> 

> _______________________________________________

> dev mailing list

> dev@openvswitch.org

> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Eelco Chaudron Feb. 21, 2018, 12:29 p.m. UTC | #6
On 21/02/18 11:33, Róbert Mulik wrote:
>
>> -----Original Message-----
>> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
>> bounces@openvswitch.org] On Behalf Of Eelco Chaudron
>> Sent: Tuesday, February 20, 2018 3:17 PM
>> To: ovs-dev@openvswitch.org
>> Subject: Re: [ovs-dev] [PATCH v5] Configurable Link State Change (LSC)
>> detection mode
>>
>> Hi Robert,
>>
>> I did not see a reply to my v4 reply, so I ask this question again:
>>       Did you try what happens if the PMD does not support LSC and its
>> enabled?
> Unfortunately I don't have any cards which don't support LSC, so I couldn't test it
Guess you could change the code for the card you have to return an 
error, or not return it as a supported option.
See my earlier command to get the port supported option and error out 
with an error that the card does not support it.
>> Also, it would be easier to understand what you are going to change if
>> you answer the questions in the review emails.
>>
>> For example, this patch is missing the suggestion on adding the LSC
>> config in netdev_dpdk_get_config(), as suggested by Ilya.
> As I understand the reason for this function would be to avoid the confusion when
> both global and local configs are configured for an interface, especially if we go with
> the implementation where the global config doesn't take effect without service
> restart.
>
> Since the global config is not implemented in this patch and the default value remains
> the same as it always was (poll mode) and also the documentation tells what the default
> value is, I don't really see the importance of this implementation.
I have to disagree here, as the getdev_dpdk_get_config() has all the 
configurable options, and you are adding one.
So please add it.

>> I did a quick visual review below, and will wait for v6 with the above
>> change, and do another test run.
>>
>> //Eelco
>>
>>
>> On 15/02/18 15:44, Róbert Mulik wrote:
>>> It is possible to change LSC detection mode to polling or interrupt mode
>>> for DPDK interfaces. The default is polling mode. To set interrupt mode,
>>> option dpdk-lsc-interrupt has to be set to true.
>>>
>>> In polling mode more processor time is needed, since the OVS repeatedly
>> reads
>>> the link state with a short period. It can lead to packet loss for certain
>>> systems.
>>>
>>> In interrupt mode the hardware itself triggers an interrupt when link state
>>> change happens, so less processing time needs for the OVS.
>>>
>>> For detailed description and usage see the dpdk install documentation.
>>>
>>> Signed-off-by: Robert Mulik <robert.mulik@ericsson.com>
>>> ---
>>>    Documentation/intro/install/dpdk.rst | 35
>> +++++++++++++++++++++++++++++++++++
>>>    lib/netdev-dpdk.c                    | 22 ++++++++++++++++++++--
>>>    vswitchd/vswitch.xml                 | 17 +++++++++++++++++
>>>    3 files changed, 72 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/intro/install/dpdk.rst
>> b/Documentation/intro/install/dpdk.rst
>>> index ed358d5..e3872e7 100644
>>> --- a/Documentation/intro/install/dpdk.rst
>>> +++ b/Documentation/intro/install/dpdk.rst
>>> @@ -628,6 +628,41 @@ The average number of packets per output batch
>> can be checked in PMD stats::
>>>        $ ovs-appctl dpif-netdev/pmd-stats-show
>>>
>>> +Link State Change (LSC) detection configuration
>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +There are two methods to get the information when Link State Change
>> (LSC)
>>> +happens on a network interface: by polling or interrupt.
>>> +
>>> +With the polling method, the main process checks the link state on a
>>> +fixed interval. This fixed interval determines how fast a link change is
>>> +detected. Another problem with the poll mode is that on some hardware
>> a
>>> +polling cycle takes too much time, which (in the end) leads to packet loss
>>> +for certain systems.
>> Maybe we should stick to facts of the implementation, and not known
>> bugs? So remove the "Another problem..."
>>> +
>>> +If interrupts are used to get LSC information, the hardware itself triggers
>>> +an interrupt when link state change happens, the interrupt thread wakes
>> up
>>> +from sleep, updates the information, and goes back to sleep mode. When
>> no link
>>> +state change happens (most of the time), the thread remains in sleep
>> mode and
>>> +doesn`t use processor time at all. The disadvantage of this method is that
>>> +when an interrupt happens, the processor has to handle it immediately,
>> so it
>>> +puts the currently running process to background, handles the interrupt,
>> and
>>> +takes the background process back.
>>> +
>>> +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``.
>>> +
>>> +Command to set interrupt mode for a specific interface::
>>> +    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=true
>>> +
>>> +Command to set polling mode for a specific interface::
>>> +    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=false
>>> +
>>> +Command to remove ``dpdk-lsc-interrupt`` option::
>>> +    $ ovs-vsctl remove interface <iface_name> options dpdk-lsc-interrupt
>>> +
>>>    Limitations
>>>    ------------
>>>
>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>> index 94fb163..d092ef1 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -433,6 +433,12 @@ struct netdev_dpdk {
>>>            /* DPDK-ETH hardware offload features,
>>>             * from the enum set 'dpdk_hw_ol_features' */
>>>            uint32_t hw_ol_features;
>>> +
>>> +        /* Properties for link state change detection mode.
>>> +         * If lsc_interrupt_mode is set to false, poll mode is used,
>>> +         * otherwise interrupt mode is used. */
>>> +        bool requested_lsc_interrupt_mode;
>>> +        bool lsc_interrupt_mode;
>>>        );
>>>
>>>        PADDED_MEMBERS(CACHE_LINE_SIZE,
>>> @@ -686,12 +692,14 @@ dpdk_watchdog(void *dummy OVS_UNUSED)
>>>    }
>>>
>>>    static int
>>> -dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int n_rxq, int
>> n_txq)
>>> +dpdk_eth_dev_port_config(struct netdev_dpdk *dev, int n_rxq, int
>> n_txq)
>>>    {
>>>        int diag = 0;
>>>        int i;
>>>        struct rte_eth_conf conf = port_conf;
>>>
>>> +    conf.intr_conf.lsc = dev->lsc_interrupt_mode;
>>> +
>>>        /* For some NICs (e.g. Niantic), scatter_rx mode needs to be explicitly
>>>         * enabled. */
>>>        if (dev->mtu > ETHER_MTU) {
>>> @@ -801,7 +809,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>>        n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq);
>>>        n_txq = MIN(info.max_tx_queues, dev->up.n_txq);
>>>
>>> -    diag = dpdk_eth_dev_queue_setup(dev, n_rxq, n_txq);
>>> +    diag = dpdk_eth_dev_port_config(dev, n_rxq, n_txq);
>>>        if (diag) {
>>>            VLOG_ERR("Interface %s(rxq:%d txq:%d) configure error: %s",
>>>                     dev->up.name, n_rxq, n_txq, rte_strerror(-diag));
>>> @@ -897,6 +905,7 @@ common_construct(struct netdev *netdev,
>> dpdk_port_t port_no,
>>>        dev->flags = 0;
>>>        dev->requested_mtu = ETHER_MTU;
>>>        dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);
>>> +    dev->requested_lsc_interrupt_mode = 0;
>>>        ovsrcu_index_init(&dev->vid, -1);
>>>        dev->vhost_reconfigured = false;
>>>        dev->attached = false;
>>> @@ -1520,6 +1529,12 @@ netdev_dpdk_set_config(struct netdev
>> *netdev, const struct smap *args,
>>>            goto out;
>>>        }
>>>
>>> +    bool lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt",
>> false);
>>> +    if (dev->requested_lsc_interrupt_mode != lsc_interrupt_mode) {
>>> +        dev->requested_lsc_interrupt_mode = lsc_interrupt_mode;
>>> +        netdev_request_reconfigure(netdev);
>>> +    }
>>> +
>>>        rx_fc_en = smap_get_bool(args, "rx-flow-ctrl", false);
>>>        tx_fc_en = smap_get_bool(args, "tx-flow-ctrl", false);
>>>        autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);
>>> @@ -3546,6 +3561,7 @@ netdev_dpdk_reconfigure(struct netdev
>> *netdev)
>>>        if (netdev->n_txq == dev->requested_n_txq
>>>            && netdev->n_rxq == dev->requested_n_rxq
>>>            && dev->mtu == dev->requested_mtu
>>> +        && dev->lsc_interrupt_mode == dev-
>>> requested_lsc_interrupt_mode
>>>            && dev->rxq_size == dev->requested_rxq_size
>>>            && dev->txq_size == dev->requested_txq_size
>>>            && dev->socket_id == dev->requested_socket_id) {
>>> @@ -3561,6 +3577,8 @@ netdev_dpdk_reconfigure(struct netdev
>> *netdev)
>>>            goto out;
>>>        }
>>>
>>> +    dev->lsc_interrupt_mode = dev->requested_lsc_interrupt_mode;
>>> +
>>>        netdev->n_txq = dev->requested_n_txq;
>>>        netdev->n_rxq = dev->requested_n_rxq;
>>>
>>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
>>> index 0c6a43d..3c9e637 100644
>>> --- a/vswitchd/vswitch.xml
>>> +++ b/vswitchd/vswitch.xml
>>> @@ -3631,6 +3631,23 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
>> type=patch options:peer=p1 \
>>>          </column>
>>>        </group>
>>>
>>> +    <group title="Link State Change detection mode">
>>> +      <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.
>>> +        </p>
>>> +        <p>
>>> +          If this value is not set, poll mode is configured.
>>> +        </p>
>>> +        <p>
>>> +          This parameter has an effect only on netdev dpdk interfaces.
>>> +        </p>
>>> +      </column>
>>> +    </group>
>>> +
>>>        <group title="Common Columns">
>>>          The overall purpose of these columns is described under
>> <code>Common
>>>          Columns</code> at the beginning of this document.
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Róbert Mulik Feb. 28, 2018, 2:23 p.m. UTC | #7
> -----Original Message-----

> From: Eelco Chaudron [mailto:echaudro@redhat.com]

> Sent: Wednesday, February 21, 2018 1:29 PM

> To: Róbert Mulik <robert.mulik@ericsson.com>; ovs-dev@openvswitch.org

> Subject: Re: [ovs-dev] [PATCH v5] Configurable Link State Change (LSC)

> detection mode

> 

> On 21/02/18 11:33, Róbert Mulik wrote:

> >

> >> -----Original Message-----

> >> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-

> >> bounces@openvswitch.org] On Behalf Of Eelco Chaudron

> >> Sent: Tuesday, February 20, 2018 3:17 PM

> >> To: ovs-dev@openvswitch.org

> >> Subject: Re: [ovs-dev] [PATCH v5] Configurable Link State Change (LSC)

> >> detection mode

> >>

> >> Hi Robert,

> >>

> >> I did not see a reply to my v4 reply, so I ask this question again:

> >>       Did you try what happens if the PMD does not support LSC and its

> >> enabled?

> > Unfortunately I don't have any cards which don't support LSC, so I couldn't

> test it

> Guess you could change the code for the card you have to return an

> error, or not return it as a supported option.

> See my earlier command to get the port supported option and error out

> with an error that the card does not support it.

You suggested to check the flag RTE_ETH_DEV_INTR_LSC to see if the NIC
supports the LSC interrupt mode. I couldn't find a way to check this flag with
the current OVS implementation. As I see the DPDK code has to be changed
to support it. Do you have any suggestion what to do now? Or did I miss
something, and the current implementation can see this flag?
> >> Also, it would be easier to understand what you are going to change if

> >> you answer the questions in the review emails.

> >>

> >> For example, this patch is missing the suggestion on adding the LSC

> >> config in netdev_dpdk_get_config(), as suggested by Ilya.

> > As I understand the reason for this function would be to avoid the

> confusion when

> > both global and local configs are configured for an interface, especially if we

> go with

> > the implementation where the global config doesn't take effect without

> service

> > restart.

> >

> > Since the global config is not implemented in this patch and the default

> value remains

> > the same as it always was (poll mode) and also the documentation tells

> what the default

> > value is, I don't really see the importance of this implementation.

> I have to disagree here, as the getdev_dpdk_get_config() has all the

> configurable options, and you are adding one.

> So please add it.

> 

> >> I did a quick visual review below, and will wait for v6 with the above

> >> change, and do another test run.

> >>

> >> //Eelco

> >>

> >>

> >> On 15/02/18 15:44, Róbert Mulik wrote:

> >>> It is possible to change LSC detection mode to polling or interrupt mode

> >>> for DPDK interfaces. The default is polling mode. To set interrupt mode,

> >>> option dpdk-lsc-interrupt has to be set to true.

> >>>

> >>> In polling mode more processor time is needed, since the OVS

> repeatedly

> >> reads

> >>> the link state with a short period. It can lead to packet loss for certain

> >>> systems.

> >>>

> >>> In interrupt mode the hardware itself triggers an interrupt when link

> state

> >>> change happens, so less processing time needs for the OVS.

> >>>

> >>> For detailed description and usage see the dpdk install documentation.

> >>>

> >>> Signed-off-by: Robert Mulik <robert.mulik@ericsson.com>

> >>> ---

> >>>    Documentation/intro/install/dpdk.rst | 35

> >> +++++++++++++++++++++++++++++++++++

> >>>    lib/netdev-dpdk.c                    | 22 ++++++++++++++++++++--

> >>>    vswitchd/vswitch.xml                 | 17 +++++++++++++++++

> >>>    3 files changed, 72 insertions(+), 2 deletions(-)

> >>>

> >>> diff --git a/Documentation/intro/install/dpdk.rst

> >> b/Documentation/intro/install/dpdk.rst

> >>> index ed358d5..e3872e7 100644

> >>> --- a/Documentation/intro/install/dpdk.rst

> >>> +++ b/Documentation/intro/install/dpdk.rst

> >>> @@ -628,6 +628,41 @@ The average number of packets per output

> batch

> >> can be checked in PMD stats::

> >>>        $ ovs-appctl dpif-netdev/pmd-stats-show

> >>>

> >>> +Link State Change (LSC) detection configuration

> >>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> >>> +

> >>> +There are two methods to get the information when Link State Change

> >> (LSC)

> >>> +happens on a network interface: by polling or interrupt.

> >>> +

> >>> +With the polling method, the main process checks the link state on a

> >>> +fixed interval. This fixed interval determines how fast a link change is

> >>> +detected. Another problem with the poll mode is that on some

> hardware

> >> a

> >>> +polling cycle takes too much time, which (in the end) leads to packet

> loss

> >>> +for certain systems.

> >> Maybe we should stick to facts of the implementation, and not known

> >> bugs? So remove the "Another problem..."

> >>> +

> >>> +If interrupts are used to get LSC information, the hardware itself

> triggers

> >>> +an interrupt when link state change happens, the interrupt thread

> wakes

> >> up

> >>> +from sleep, updates the information, and goes back to sleep mode.

> When

> >> no link

> >>> +state change happens (most of the time), the thread remains in sleep

> >> mode and

> >>> +doesn`t use processor time at all. The disadvantage of this method is

> that

> >>> +when an interrupt happens, the processor has to handle it

> immediately,

> >> so it

> >>> +puts the currently running process to background, handles the

> interrupt,

> >> and

> >>> +takes the background process back.

> >>> +

> >>> +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``.

> >>> +

> >>> +Command to set interrupt mode for a specific interface::

> >>> +    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-

> interrupt=true

> >>> +

> >>> +Command to set polling mode for a specific interface::

> >>> +    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-

> interrupt=false

> >>> +

> >>> +Command to remove ``dpdk-lsc-interrupt`` option::

> >>> +    $ ovs-vsctl remove interface <iface_name> options dpdk-lsc-

> interrupt

> >>> +

> >>>    Limitations

> >>>    ------------

> >>>

> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c

> >>> index 94fb163..d092ef1 100644

> >>> --- a/lib/netdev-dpdk.c

> >>> +++ b/lib/netdev-dpdk.c

> >>> @@ -433,6 +433,12 @@ struct netdev_dpdk {

> >>>            /* DPDK-ETH hardware offload features,

> >>>             * from the enum set 'dpdk_hw_ol_features' */

> >>>            uint32_t hw_ol_features;

> >>> +

> >>> +        /* Properties for link state change detection mode.

> >>> +         * If lsc_interrupt_mode is set to false, poll mode is used,

> >>> +         * otherwise interrupt mode is used. */

> >>> +        bool requested_lsc_interrupt_mode;

> >>> +        bool lsc_interrupt_mode;

> >>>        );

> >>>

> >>>        PADDED_MEMBERS(CACHE_LINE_SIZE,

> >>> @@ -686,12 +692,14 @@ dpdk_watchdog(void *dummy OVS_UNUSED)

> >>>    }

> >>>

> >>>    static int

> >>> -dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int n_rxq, int

> >> n_txq)

> >>> +dpdk_eth_dev_port_config(struct netdev_dpdk *dev, int n_rxq, int

> >> n_txq)

> >>>    {

> >>>        int diag = 0;

> >>>        int i;

> >>>        struct rte_eth_conf conf = port_conf;

> >>>

> >>> +    conf.intr_conf.lsc = dev->lsc_interrupt_mode;

> >>> +

> >>>        /* For some NICs (e.g. Niantic), scatter_rx mode needs to be

> explicitly

> >>>         * enabled. */

> >>>        if (dev->mtu > ETHER_MTU) {

> >>> @@ -801,7 +809,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)

> >>>        n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq);

> >>>        n_txq = MIN(info.max_tx_queues, dev->up.n_txq);

> >>>

> >>> -    diag = dpdk_eth_dev_queue_setup(dev, n_rxq, n_txq);

> >>> +    diag = dpdk_eth_dev_port_config(dev, n_rxq, n_txq);

> >>>        if (diag) {

> >>>            VLOG_ERR("Interface %s(rxq:%d txq:%d) configure error: %s",

> >>>                     dev->up.name, n_rxq, n_txq, rte_strerror(-diag));

> >>> @@ -897,6 +905,7 @@ common_construct(struct netdev *netdev,

> >> dpdk_port_t port_no,

> >>>        dev->flags = 0;

> >>>        dev->requested_mtu = ETHER_MTU;

> >>>        dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);

> >>> +    dev->requested_lsc_interrupt_mode = 0;

> >>>        ovsrcu_index_init(&dev->vid, -1);

> >>>        dev->vhost_reconfigured = false;

> >>>        dev->attached = false;

> >>> @@ -1520,6 +1529,12 @@ netdev_dpdk_set_config(struct netdev

> >> *netdev, const struct smap *args,

> >>>            goto out;

> >>>        }

> >>>

> >>> +    bool lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-

> interrupt",

> >> false);

> >>> +    if (dev->requested_lsc_interrupt_mode != lsc_interrupt_mode) {

> >>> +        dev->requested_lsc_interrupt_mode = lsc_interrupt_mode;

> >>> +        netdev_request_reconfigure(netdev);

> >>> +    }

> >>> +

> >>>        rx_fc_en = smap_get_bool(args, "rx-flow-ctrl", false);

> >>>        tx_fc_en = smap_get_bool(args, "tx-flow-ctrl", false);

> >>>        autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);

> >>> @@ -3546,6 +3561,7 @@ netdev_dpdk_reconfigure(struct netdev

> >> *netdev)

> >>>        if (netdev->n_txq == dev->requested_n_txq

> >>>            && netdev->n_rxq == dev->requested_n_rxq

> >>>            && dev->mtu == dev->requested_mtu

> >>> +        && dev->lsc_interrupt_mode == dev-

> >>> requested_lsc_interrupt_mode

> >>>            && dev->rxq_size == dev->requested_rxq_size

> >>>            && dev->txq_size == dev->requested_txq_size

> >>>            && dev->socket_id == dev->requested_socket_id) {

> >>> @@ -3561,6 +3577,8 @@ netdev_dpdk_reconfigure(struct netdev

> >> *netdev)

> >>>            goto out;

> >>>        }

> >>>

> >>> +    dev->lsc_interrupt_mode = dev->requested_lsc_interrupt_mode;

> >>> +

> >>>        netdev->n_txq = dev->requested_n_txq;

> >>>        netdev->n_rxq = dev->requested_n_rxq;

> >>>

> >>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml

> >>> index 0c6a43d..3c9e637 100644

> >>> --- a/vswitchd/vswitch.xml

> >>> +++ b/vswitchd/vswitch.xml

> >>> @@ -3631,6 +3631,23 @@ ovs-vsctl add-port br0 p0 -- set Interface p0

> >> type=patch options:peer=p1 \

> >>>          </column>

> >>>        </group>

> >>>

> >>> +    <group title="Link State Change detection mode">

> >>> +      <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.

> >>> +        </p>

> >>> +        <p>

> >>> +          If this value is not set, poll mode is configured.

> >>> +        </p>

> >>> +        <p>

> >>> +          This parameter has an effect only on netdev dpdk interfaces.

> >>> +        </p>

> >>> +      </column>

> >>> +    </group>

> >>> +

> >>>        <group title="Common Columns">

> >>>          The overall purpose of these columns is described under

> >> <code>Common

> >>>          Columns</code> at the beginning of this document.

> >>> --

> >>> 1.9.1

> >>>

> >>> _______________________________________________

> >>> dev mailing list

> >>> dev@openvswitch.org

> >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

> >>

> >> _______________________________________________

> >> dev mailing list

> >> dev@openvswitch.org

> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

>
Eelco Chaudron March 1, 2018, 10:37 a.m. UTC | #8
On 28/02/18 15:23, Róbert Mulik wrote:
>
>> -----Original Message-----
>> From: Eelco Chaudron [mailto:echaudro@redhat.com]
>> Sent: Wednesday, February 21, 2018 1:29 PM
>> To: Róbert Mulik <robert.mulik@ericsson.com>; ovs-dev@openvswitch.org
>> Subject: Re: [ovs-dev] [PATCH v5] Configurable Link State Change (LSC)
>> detection mode
>>
>> On 21/02/18 11:33, Róbert Mulik wrote:
>>>> -----Original Message-----
>>>> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
>>>> bounces@openvswitch.org] On Behalf Of Eelco Chaudron
>>>> Sent: Tuesday, February 20, 2018 3:17 PM
>>>> To: ovs-dev@openvswitch.org
>>>> Subject: Re: [ovs-dev] [PATCH v5] Configurable Link State Change (LSC)
>>>> detection mode
>>>>
>>>> Hi Robert,
>>>>
>>>> I did not see a reply to my v4 reply, so I ask this question again:
>>>>        Did you try what happens if the PMD does not support LSC and its
>>>> enabled?
>>> Unfortunately I don't have any cards which don't support LSC, so I couldn't
>> test it
>> Guess you could change the code for the card you have to return an
>> error, or not return it as a supported option.
>> See my earlier command to get the port supported option and error out
>> with an error that the card does not support it.
> You suggested to check the flag RTE_ETH_DEV_INTR_LSC to see if the NIC
> supports the LSC interrupt mode. I couldn't find a way to check this flag with
> the current OVS implementation. As I see the DPDK code has to be changed
> to support it. Do you have any suggestion what to do now? Or did I miss
> something, and the current implementation can see this flag?
Did not look at the testpmd code close enough, but looking at it again 
it seems they use a hack by accessing some data directly :(

2085 <http://127.0.0.1:8080/source/xref/dpdk/app/test-pmd/testpmd.c#2085>		*if*  (lsc_interrupt 
<http://127.0.0.1:8080/source/xref/dpdk/app/test-pmd/testpmd.c#lsc_interrupt>  &&
2086 <http://127.0.0.1:8080/source/xref/dpdk/app/test-pmd/testpmd.c#2086>		    (rte_eth_devices 
<http://127.0.0.1:8080/source/s?defs=rte_eth_devices&project=dpdk>[pid <http://127.0.0.1:8080/source/s?defs=pid&project=dpdk>].data <http://127.0.0.1:8080/source/s?defs=data&project=dpdk>->dev_flags <http://127.0.0.1:8080/source/s?defs=dev_flags&project=dpdk>  &
2087 <http://127.0.0.1:8080/source/xref/dpdk/app/test-pmd/testpmd.c#2087>		RTE_ETH_DEV_INTR_LSC 
<http://127.0.0.1:8080/source/s?defs=RTE_ETH_DEV_INTR_LSC&project=dpdk>))
2088 <http://127.0.0.1:8080/source/xref/dpdk/app/test-pmd/testpmd.c#2088>			port <http://127.0.0.1:8080/source/s?defs=port&project=dpdk>->dev_conf <http://127.0.0.1:8080/source/s?defs=dev_conf&project=dpdk>.intr_conf <http://127.0.0.1:8080/source/s?defs=intr_conf&project=dpdk>.lsc <http://127.0.0.1:8080/source/s?defs=lsc&project=dpdk>  =1;


>>>> Also, it would be easier to understand what you are going to change if
>>>> you answer the questions in the review emails.
>>>>
>>>> For example, this patch is missing the suggestion on adding the LSC
>>>> config in netdev_dpdk_get_config(), as suggested by Ilya.
>>> As I understand the reason for this function would be to avoid the
>> confusion when
>>> both global and local configs are configured for an interface, especially if we
>> go with
>>> the implementation where the global config doesn't take effect without
>> service
>>> restart.
>>>
>>> Since the global config is not implemented in this patch and the default
>> value remains
>>> the same as it always was (poll mode) and also the documentation tells
>> what the default
>>> value is, I don't really see the importance of this implementation.
>> I have to disagree here, as the getdev_dpdk_get_config() has all the
>> configurable options, and you are adding one.
>> So please add it.
>>
>>>> I did a quick visual review below, and will wait for v6 with the above
>>>> change, and do another test run.
>>>>
>>>> //Eelco
>>>>
>>>>
>>>> On 15/02/18 15:44, Róbert Mulik wrote:
>>>>> It is possible to change LSC detection mode to polling or interrupt mode
>>>>> for DPDK interfaces. The default is polling mode. To set interrupt mode,
>>>>> option dpdk-lsc-interrupt has to be set to true.
>>>>>
>>>>> In polling mode more processor time is needed, since the OVS
>> repeatedly
>>>> reads
>>>>> the link state with a short period. It can lead to packet loss for certain
>>>>> systems.
>>>>>
>>>>> In interrupt mode the hardware itself triggers an interrupt when link
>> state
>>>>> change happens, so less processing time needs for the OVS.
>>>>>
>>>>> For detailed description and usage see the dpdk install documentation.
>>>>>
>>>>> Signed-off-by: Robert Mulik <robert.mulik@ericsson.com>
>>>>> ---
>>>>>     Documentation/intro/install/dpdk.rst | 35
>>>> +++++++++++++++++++++++++++++++++++
>>>>>     lib/netdev-dpdk.c                    | 22 ++++++++++++++++++++--
>>>>>     vswitchd/vswitch.xml                 | 17 +++++++++++++++++
>>>>>     3 files changed, 72 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/intro/install/dpdk.rst
>>>> b/Documentation/intro/install/dpdk.rst
>>>>> index ed358d5..e3872e7 100644
>>>>> --- a/Documentation/intro/install/dpdk.rst
>>>>> +++ b/Documentation/intro/install/dpdk.rst
>>>>> @@ -628,6 +628,41 @@ The average number of packets per output
>> batch
>>>> can be checked in PMD stats::
>>>>>         $ ovs-appctl dpif-netdev/pmd-stats-show
>>>>>
>>>>> +Link State Change (LSC) detection configuration
>>>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>> +
>>>>> +There are two methods to get the information when Link State Change
>>>> (LSC)
>>>>> +happens on a network interface: by polling or interrupt.
>>>>> +
>>>>> +With the polling method, the main process checks the link state on a
>>>>> +fixed interval. This fixed interval determines how fast a link change is
>>>>> +detected. Another problem with the poll mode is that on some
>> hardware
>>>> a
>>>>> +polling cycle takes too much time, which (in the end) leads to packet
>> loss
>>>>> +for certain systems.
>>>> Maybe we should stick to facts of the implementation, and not known
>>>> bugs? So remove the "Another problem..."
>>>>> +
>>>>> +If interrupts are used to get LSC information, the hardware itself
>> triggers
>>>>> +an interrupt when link state change happens, the interrupt thread
>> wakes
>>>> up
>>>>> +from sleep, updates the information, and goes back to sleep mode.
>> When
>>>> no link
>>>>> +state change happens (most of the time), the thread remains in sleep
>>>> mode and
>>>>> +doesn`t use processor time at all. The disadvantage of this method is
>> that
>>>>> +when an interrupt happens, the processor has to handle it
>> immediately,
>>>> so it
>>>>> +puts the currently running process to background, handles the
>> interrupt,
>>>> and
>>>>> +takes the background process back.
>>>>> +
>>>>> +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``.
>>>>> +
>>>>> +Command to set interrupt mode for a specific interface::
>>>>> +    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-
>> interrupt=true
>>>>> +
>>>>> +Command to set polling mode for a specific interface::
>>>>> +    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-
>> interrupt=false
>>>>> +
>>>>> +Command to remove ``dpdk-lsc-interrupt`` option::
>>>>> +    $ ovs-vsctl remove interface <iface_name> options dpdk-lsc-
>> interrupt
>>>>> +
>>>>>     Limitations
>>>>>     ------------
>>>>>
>>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>>>> index 94fb163..d092ef1 100644
>>>>> --- a/lib/netdev-dpdk.c
>>>>> +++ b/lib/netdev-dpdk.c
>>>>> @@ -433,6 +433,12 @@ struct netdev_dpdk {
>>>>>             /* DPDK-ETH hardware offload features,
>>>>>              * from the enum set 'dpdk_hw_ol_features' */
>>>>>             uint32_t hw_ol_features;
>>>>> +
>>>>> +        /* Properties for link state change detection mode.
>>>>> +         * If lsc_interrupt_mode is set to false, poll mode is used,
>>>>> +         * otherwise interrupt mode is used. */
>>>>> +        bool requested_lsc_interrupt_mode;
>>>>> +        bool lsc_interrupt_mode;
>>>>>         );
>>>>>
>>>>>         PADDED_MEMBERS(CACHE_LINE_SIZE,
>>>>> @@ -686,12 +692,14 @@ dpdk_watchdog(void *dummy OVS_UNUSED)
>>>>>     }
>>>>>
>>>>>     static int
>>>>> -dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int n_rxq, int
>>>> n_txq)
>>>>> +dpdk_eth_dev_port_config(struct netdev_dpdk *dev, int n_rxq, int
>>>> n_txq)
>>>>>     {
>>>>>         int diag = 0;
>>>>>         int i;
>>>>>         struct rte_eth_conf conf = port_conf;
>>>>>
>>>>> +    conf.intr_conf.lsc = dev->lsc_interrupt_mode;
>>>>> +
>>>>>         /* For some NICs (e.g. Niantic), scatter_rx mode needs to be
>> explicitly
>>>>>          * enabled. */
>>>>>         if (dev->mtu > ETHER_MTU) {
>>>>> @@ -801,7 +809,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>>>>         n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq);
>>>>>         n_txq = MIN(info.max_tx_queues, dev->up.n_txq);
>>>>>
>>>>> -    diag = dpdk_eth_dev_queue_setup(dev, n_rxq, n_txq);
>>>>> +    diag = dpdk_eth_dev_port_config(dev, n_rxq, n_txq);
>>>>>         if (diag) {
>>>>>             VLOG_ERR("Interface %s(rxq:%d txq:%d) configure error: %s",
>>>>>                      dev->up.name, n_rxq, n_txq, rte_strerror(-diag));
>>>>> @@ -897,6 +905,7 @@ common_construct(struct netdev *netdev,
>>>> dpdk_port_t port_no,
>>>>>         dev->flags = 0;
>>>>>         dev->requested_mtu = ETHER_MTU;
>>>>>         dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);
>>>>> +    dev->requested_lsc_interrupt_mode = 0;
>>>>>         ovsrcu_index_init(&dev->vid, -1);
>>>>>         dev->vhost_reconfigured = false;
>>>>>         dev->attached = false;
>>>>> @@ -1520,6 +1529,12 @@ netdev_dpdk_set_config(struct netdev
>>>> *netdev, const struct smap *args,
>>>>>             goto out;
>>>>>         }
>>>>>
>>>>> +    bool lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-
>> interrupt",
>>>> false);
>>>>> +    if (dev->requested_lsc_interrupt_mode != lsc_interrupt_mode) {
>>>>> +        dev->requested_lsc_interrupt_mode = lsc_interrupt_mode;
>>>>> +        netdev_request_reconfigure(netdev);
>>>>> +    }
>>>>> +
>>>>>         rx_fc_en = smap_get_bool(args, "rx-flow-ctrl", false);
>>>>>         tx_fc_en = smap_get_bool(args, "tx-flow-ctrl", false);
>>>>>         autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);
>>>>> @@ -3546,6 +3561,7 @@ netdev_dpdk_reconfigure(struct netdev
>>>> *netdev)
>>>>>         if (netdev->n_txq == dev->requested_n_txq
>>>>>             && netdev->n_rxq == dev->requested_n_rxq
>>>>>             && dev->mtu == dev->requested_mtu
>>>>> +        && dev->lsc_interrupt_mode == dev-
>>>>> requested_lsc_interrupt_mode
>>>>>             && dev->rxq_size == dev->requested_rxq_size
>>>>>             && dev->txq_size == dev->requested_txq_size
>>>>>             && dev->socket_id == dev->requested_socket_id) {
>>>>> @@ -3561,6 +3577,8 @@ netdev_dpdk_reconfigure(struct netdev
>>>> *netdev)
>>>>>             goto out;
>>>>>         }
>>>>>
>>>>> +    dev->lsc_interrupt_mode = dev->requested_lsc_interrupt_mode;
>>>>> +
>>>>>         netdev->n_txq = dev->requested_n_txq;
>>>>>         netdev->n_rxq = dev->requested_n_rxq;
>>>>>
>>>>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
>>>>> index 0c6a43d..3c9e637 100644
>>>>> --- a/vswitchd/vswitch.xml
>>>>> +++ b/vswitchd/vswitch.xml
>>>>> @@ -3631,6 +3631,23 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
>>>> type=patch options:peer=p1 \
>>>>>           </column>
>>>>>         </group>
>>>>>
>>>>> +    <group title="Link State Change detection mode">
>>>>> +      <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.
>>>>> +        </p>
>>>>> +        <p>
>>>>> +          If this value is not set, poll mode is configured.
>>>>> +        </p>
>>>>> +        <p>
>>>>> +          This parameter has an effect only on netdev dpdk interfaces.
>>>>> +        </p>
>>>>> +      </column>
>>>>> +    </group>
>>>>> +
>>>>>         <group title="Common Columns">
>>>>>           The overall purpose of these columns is described under
>>>> <code>Common
>>>>>           Columns</code> at the beginning of this document.
>>>>> --
>>>>> 1.9.1
>>>>>
>>>>> _______________________________________________
>>>>> dev mailing list
>>>>> dev@openvswitch.org
>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>> _______________________________________________
>>>> dev mailing list
>>>> dev@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

diff --git a/Documentation/intro/install/dpdk.rst b/Documentation/intro/install/dpdk.rst
index ed358d5..e3872e7 100644
--- a/Documentation/intro/install/dpdk.rst
+++ b/Documentation/intro/install/dpdk.rst
@@ -628,6 +628,41 @@  The average number of packets per output batch can be checked in PMD stats::

     $ ovs-appctl dpif-netdev/pmd-stats-show

+Link State Change (LSC) detection configuration
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+There are two methods to get the information when Link State Change (LSC)
+happens on a network interface: by polling or interrupt.
+
+With the polling method, the main process checks the link state on a
+fixed interval. This fixed interval determines how fast a link change is
+detected. Another problem with the poll mode is that on some hardware a
+polling cycle takes too much time, which (in the end) leads to packet loss
+for certain systems.
+
+If interrupts are used to get LSC information, the hardware itself triggers
+an interrupt when link state change happens, the interrupt thread wakes up
+from sleep, updates the information, and goes back to sleep mode. When no link
+state change happens (most of the time), the thread remains in sleep mode and
+doesn`t use processor time at all. The disadvantage of this method is that
+when an interrupt happens, the processor has to handle it immediately, so it
+puts the currently running process to background, handles the interrupt, and
+takes the background process back.
+
+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``.
+
+Command to set interrupt mode for a specific interface::
+    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=true
+
+Command to set polling mode for a specific interface::
+    $ ovs-vsctl set interface <iface_name> options:dpdk-lsc-interrupt=false
+
+Command to remove ``dpdk-lsc-interrupt`` option::
+    $ ovs-vsctl remove interface <iface_name> options dpdk-lsc-interrupt
+
 Limitations
 ------------

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 94fb163..d092ef1 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -433,6 +433,12 @@  struct netdev_dpdk {
         /* DPDK-ETH hardware offload features,
          * from the enum set 'dpdk_hw_ol_features' */
         uint32_t hw_ol_features;
+
+        /* Properties for link state change detection mode.
+         * If lsc_interrupt_mode is set to false, poll mode is used,
+         * otherwise interrupt mode is used. */
+        bool requested_lsc_interrupt_mode;
+        bool lsc_interrupt_mode;
     );

     PADDED_MEMBERS(CACHE_LINE_SIZE,
@@ -686,12 +692,14 @@  dpdk_watchdog(void *dummy OVS_UNUSED)
 }

 static int
-dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int n_rxq, int n_txq)
+dpdk_eth_dev_port_config(struct netdev_dpdk *dev, int n_rxq, int n_txq)
 {
     int diag = 0;
     int i;
     struct rte_eth_conf conf = port_conf;

+    conf.intr_conf.lsc = dev->lsc_interrupt_mode;
+
     /* For some NICs (e.g. Niantic), scatter_rx mode needs to be explicitly
      * enabled. */
     if (dev->mtu > ETHER_MTU) {
@@ -801,7 +809,7 @@  dpdk_eth_dev_init(struct netdev_dpdk *dev)
     n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq);
     n_txq = MIN(info.max_tx_queues, dev->up.n_txq);

-    diag = dpdk_eth_dev_queue_setup(dev, n_rxq, n_txq);
+    diag = dpdk_eth_dev_port_config(dev, n_rxq, n_txq);
     if (diag) {
         VLOG_ERR("Interface %s(rxq:%d txq:%d) configure error: %s",
                  dev->up.name, n_rxq, n_txq, rte_strerror(-diag));
@@ -897,6 +905,7 @@  common_construct(struct netdev *netdev, dpdk_port_t port_no,
     dev->flags = 0;
     dev->requested_mtu = ETHER_MTU;
     dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);
+    dev->requested_lsc_interrupt_mode = 0;
     ovsrcu_index_init(&dev->vid, -1);
     dev->vhost_reconfigured = false;
     dev->attached = false;
@@ -1520,6 +1529,12 @@  netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
         goto out;
     }

+    bool lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", false);
+    if (dev->requested_lsc_interrupt_mode != lsc_interrupt_mode) {
+        dev->requested_lsc_interrupt_mode = lsc_interrupt_mode;
+        netdev_request_reconfigure(netdev);
+    }
+
     rx_fc_en = smap_get_bool(args, "rx-flow-ctrl", false);
     tx_fc_en = smap_get_bool(args, "tx-flow-ctrl", false);
     autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);
@@ -3546,6 +3561,7 @@  netdev_dpdk_reconfigure(struct netdev *netdev)
     if (netdev->n_txq == dev->requested_n_txq
         && netdev->n_rxq == dev->requested_n_rxq
         && dev->mtu == dev->requested_mtu
+        && dev->lsc_interrupt_mode == dev->requested_lsc_interrupt_mode
         && dev->rxq_size == dev->requested_rxq_size
         && dev->txq_size == dev->requested_txq_size
         && dev->socket_id == dev->requested_socket_id) {
@@ -3561,6 +3577,8 @@  netdev_dpdk_reconfigure(struct netdev *netdev)
         goto out;
     }

+    dev->lsc_interrupt_mode = dev->requested_lsc_interrupt_mode;
+
     netdev->n_txq = dev->requested_n_txq;
     netdev->n_rxq = dev->requested_n_rxq;

diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 0c6a43d..3c9e637 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -3631,6 +3631,23 @@  ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
       </column>
     </group>

+    <group title="Link State Change detection mode">
+      <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.
+        </p>
+        <p>
+          If this value is not set, poll mode is configured.
+        </p>
+        <p>
+          This parameter has an effect only on netdev dpdk interfaces.
+        </p>
+      </column>
+    </group>
+
     <group title="Common Columns">
       The overall purpose of these columns is described under <code>Common
       Columns</code> at the beginning of this document.