[ovs-dev] [PATCH v2] netdev-dpdk: Configurable Link State Change (LSC) detection mode

Eelco Chaudron echaudro at redhat.com
Mon Jan 22 12:37:38 UTC 2018


Hi Robert,

See inline comments below. In addition see Ian's comments on documentation.

//Eelco

On 19/01/18 13: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.
>
> Global settings
> Service restart is necessary for the global settings to take effect.
>
> Command to set interrupt mode for all interfaces:
> ovs-vsctl set Open_vSwitch . other_config:dpdk-lsc-interrupt=true
>
> 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)
> Service restart is not necessary to take effect.
>
> Command to set interrupt mode for a specific interface:
> ovs-vsctl set interface <interface_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 reset to globally defined mode for a specific interface:
> ovs-vsctl remove interface <interface_name> options dpdk-lsc-interrupt
>
> Signed-off-by: Robert Mulik<robert.mulik at ericsson.com>
> ---
>   lib/dpdk.c           |  8 ++++++++
>   lib/netdev-dpdk.c    | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>   lib/netdev-dpdk.h    |  9 +++++++++
>   vswitchd/vswitch.xml | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 109 insertions(+), 1 deletion(-)
>
> diff --git a/lib/dpdk.c b/lib/dpdk.c
> index 6710d10..28687eb 100644
> --- a/lib/dpdk.c
> +++ b/lib/dpdk.c
> @@ -351,6 +351,14 @@ dpdk_init__(const struct smap *ovs_other_config)
>       VLOG_INFO("IOMMU support for vhost-user-client %s.",
>                  vhost_iommu_enabled ? "enabled" : "disabled");
>
> +    if (smap_get_bool(ovs_other_config, "dpdk-lsc-interrupt", false)) {
> +        netdev_dpdk_set_default_lsc_detect_mode(
> +            NETDEV_DPDK_LSC_DETECT_INTERRUPT_MODE);
> +    } else {
> +        netdev_dpdk_set_default_lsc_detect_mode(
> +            NETDEV_DPDK_LSC_DETECT_POLL_MODE);
> +    }
> +
Any change here is not triggering re-initialization of the devices. I 
know you mention this in the documentation, but it would be nice if it 
would take effect, like the PMD mask changes.
Specially as any change in the device, like rxq's might trigger the 
change for this specific device only.
>       argv = grow_argv(&argv, 0, 1);
>       argc = 1;
>       argv[0] = xstrdup(ovs_get_program_name());
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index e32c7f6..cb52222 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 const struct rte_eth_conf port_conf = {
> +static struct rte_eth_conf port_conf = {
>       .rxmode = {
>           .mq_mode = ETH_MQ_RX_RSS,
>           .split_hdr_size = 0,
> @@ -167,6 +167,10 @@ static const struct rte_eth_conf port_conf = {
>       .txmode = {
>           .mq_mode = ETH_MQ_TX_NONE,
>       },
> +    .intr_conf = {
> +        /* LSC interrupt mode disabled, polling mode used. */
> +        .lsc = (uint16_t)NETDEV_DPDK_LSC_DETECT_POLL_MODE,
Don't think we need the uint16_t cast here.
> +    },
>   };
>
>   /*
> @@ -416,6 +420,9 @@ struct netdev_dpdk {
>           int requested_n_rxq;
>           int requested_rxq_size;
>           int requested_txq_size;
> +        enum netdev_dpdk_lsc_detect_mode requested_lsc_detect_mode;
> +
Maybe move below to end of this PADDED_MEMBERS, and add some comment.
> +        enum netdev_dpdk_lsc_detect_mode lsc_detect_mode;
>
>           /* Number of rx/tx descriptors for physical devices */
>           int rxq_size;
> @@ -457,6 +464,19 @@ 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(
> +    enum netdev_dpdk_lsc_detect_mode default_lsc)
> +{
> +    port_conf.intr_conf.lsc = (uint16_t)default_lsc;
Don't think we need the casting here. Also DPDK only supports a 0, or 1, 
so we should check it here.
> +}
> +
> +enum netdev_dpdk_lsc_detect_mode
> +netdev_dpdk_get_default_lsc_detect_mode(void)
> +{
> +    return port_conf.intr_conf.lsc;
> +}
> +
>   static bool
>   is_dpdk_class(const struct netdev_class *class)
>   {
> @@ -690,6 +710,8 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int n_rxq, int n_txq)
>       int i;
>       struct rte_eth_conf conf = port_conf;
>
> +    conf.intr_conf.lsc = dev->lsc_detect_mode;
> +
The dpdk_eth_dev_queue_setup() now becomes a more general setup 
function. Should we change the naming?
>       /* For some NICs (e.g. Niantic), scatter_rx mode needs to be explicitly
>        * enabled. */
>       if (dev->mtu > ETHER_MTU) {
> @@ -895,6 +917,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_detect_mode = netdev_dpdk_get_default_lsc_detect_mode();
>       ovsrcu_index_init(&dev->vid, -1);
>       dev->vhost_reconfigured = false;
>       dev->attached = false;
> @@ -1469,6 +1492,25 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
>                               "The old 'dpdk<port_id>' names are not supported",
>                         netdev_get_name(netdev));
>           err = EINVAL;
> +        goto out;
> +    }
> +
> +    struct smap_node *node = smap_get_node(args, "dpdk-lsc-interrupt");
> +    uint16_t lsc;
lsc should be netdev_dpdk_lsc_detect_mode
> +
> +    if (node) {
> +        lsc = smap_get_bool(args, "dpdk-lsc-interrupt", false);
We should not have lsc take the bool, but either 
NETDEV_DPDK_LSC_DETECT_INTERRUPT_MODE, or POLL
> +    } else {
> +        lsc = netdev_dpdk_get_default_lsc_detect_mode();
> +    }
> +    if (lsc > NETDEV_DPDK_LSC_DETECT_MAX) {
> +        VLOG_WARN("%s: invalid LSC value %d\n", dev->up.name, lsc);
> +        err = EINVAL;
> +        goto out;
I we make sure netdev_dpdk_get_default_lsc_detect_mode() always return a 
valid value (see suggestion earlier) we should be able to remove this check.
> +    } else if (dev->requested_lsc_detect_mode !=
> +               (enum netdev_dpdk_lsc_detect_mode)lsc) {
the above should get ride of all the typecasting above/below.
> +        dev->requested_lsc_detect_mode = (enum netdev_dpdk_lsc_detect_mode)lsc;
> +        netdev_request_reconfigure(netdev);
>       }
This function is not catching this sequence:

ovs-vsctl set interface dpdk0 options:dpdk-lsc-interrupt=true
ovs-vsctl remove interface dpdk0 options dpdk-lsc-interrupt

>       if (err) {
> @@ -3423,6 +3465,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_detect_mode == dev->requested_lsc_detect_mode
>           && dev->rxq_size == dev->requested_rxq_size
>           && dev->txq_size == dev->requested_txq_size
>           && dev->socket_id == dev->requested_socket_id) {
> @@ -3438,6 +3481,8 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
>           goto out;
>       }
>
> +    dev->lsc_detect_mode = dev->requested_lsc_detect_mode;
> +
>       netdev->n_txq = dev->requested_n_txq;
>       netdev->n_rxq = dev->requested_n_rxq;
>
> diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h
> index b7d02a7..93fdd1d 100644
> --- a/lib/netdev-dpdk.h
> +++ b/lib/netdev-dpdk.h
> @@ -25,8 +25,17 @@ struct dp_packet;
>
>   #ifdef DPDK_NETDEV
>
> +enum netdev_dpdk_lsc_detect_mode {
> +    NETDEV_DPDK_LSC_DETECT_POLL_MODE,
> +    NETDEV_DPDK_LSC_DETECT_INTERRUPT_MODE,
> +    NETDEV_DPDK_LSC_DETECT_MAX = NETDEV_DPDK_LSC_DETECT_INTERRUPT_MODE,
> +};
Maybe an enum is to much for just two values? It might also remove the 
complexity by just storing a bool? lsc_interrupt_mode?
> +
>   void netdev_dpdk_register(void);
>   void free_dpdk_buf(struct dp_packet *);
> +void netdev_dpdk_set_default_lsc_detect_mode(
> +         enum netdev_dpdk_lsc_detect_mode default_lsc);
> +enum netdev_dpdk_lsc_detect_mode netdev_dpdk_get_default_lsc_detect_mode(void);
>
>   #else
>
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 58c0ebd..a4a0150 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -320,6 +320,30 @@
>           </p>
>         </column>
>
> +      <column name="other_config" key="dpdk-lsc-interrupt"
> +              type='{"type": "boolean"}'>
> +        <p>
> +          Set this value to <code>true</code> to set interrupt mode for Link
Change "to set interrupt" to "to configure interrupt"
> +          State Change (LSC) detection instead of poll mode for DPDK
> +          interfaces.
> +        </p>
> +        <p>
> +          The default value is <code>false</code>. Changing this value requires
> +          restarting the daemon
See other note on maybe changing this, if not at least at a node that it 
will take effect for dpdk interfaces being reconfigured.
> +        </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 single interface in the options
Change "a single" to "a specific"
> +          section of that interface.
> +        </p>
> +        <p>
> +          This parameter has any effect only on netdev dpdk interfaces.
Change any to an
> +        </p>
> +      </column>
> +
>         <column name="other_config" key="dpdk-extra"
>                 type='{"type": "string"}'>
>           <p>
> @@ -3593,6 +3617,28 @@ 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 set interrupt mode for Link
Change "to set interrupt" to "to configure interrupt"

>            State Change (LSC) detection instead of poll mode for the DPDK
> +          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.
> +        </p>
> +        <p>
> +          This parameter has any effect only on netdev dpdk interfaces.
change any to an
> +        </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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev




More information about the dev mailing list