[ovs-dev] [PATCH v1] netdev-dpdk: Fix flow control configuration.

Konieczny, TomaszX tomaszx.konieczny at intel.com
Wed Sep 11 09:53:12 UTC 2019


>-----Original Message-----
>From: Ilya Maximets <i.maximets at samsung.com>
>Sent: 10 September 2019 14:29
>To: Konieczny, TomaszX <tomaszx.konieczny at intel.com>; dev at openvswitch.org
>Cc: Stokes, Ian <ian.stokes at intel.com>
>Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Fix flow control configuration.
>
>
>I looked at the code once more and to the dpdk drivers' implementation.
>You're partially right, but we have to get() flow control configuration in all cases
>because some drivers (e.g. ixgbe) has flow control enabled by default and since
>we have 'false' as a default value, we need to disable the flow control for it. Not
>calling the get() will result in setting zeroized fc_config, which may be discarded
>by the driver since those values are validated inside.
>
>So, I think that we need to call get() function unconditionally.
>Avoiding of failure for devices that doesn't support flow control could be
>implemented like this:
>
>    /* Get the Flow control configuration. */
>    err = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);
>    if (err) {
>        if (err == -ENOTSUP) {
>            VLOG_INFO_ONCE("%s: Flow control is not supported.",
>                           netdev_get_name(netdev));
>            err = 0; /* Not fatal. */
>        } else {
>            VLOG_WARN("%s: Cannot get flow control parameters: %s",
>                      netdev_get_name(netdev), rte_strerror(-err));
>        }
>        goto out;
>    }
>
>What do you think?
>
>
>One minor nit is that it's better to keep an empty line here:
>---
>     autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);
>-
>     fc_mode = fc_mode_set[tx_fc_en][rx_fc_en];
>---
>
>Best regards, Ilya Maximets.

Hi Ilya,

I agree, this approach looks neater and seems to work.
However now if you try to enable flow control on unsupported device it will pass without any warnings or errors. Would it be a desired behavior?

Also, I've tested my patch on ixgbe device and it worked fine, though I still like your solution better.


More information about the dev mailing list