[ovs-dev] [PATCH] netdev-dpdk: Add Flow Control support.
Daniele Di Proietto
diproiettod at ovn.org
Thu Jul 28 00:37:55 UTC 2016
Thanks for the patch.
I agree, I'd be nice to document this in INSTALL.DPDK-ADVANCED.md
We should also document the new fields in vswitchd/vswitch.xml.
Probably it's better to use "true" and "false" rather that "on" and "off",
for consistency with other configuration options and so that we can use
smap_get_bool().
I assume it's ok to call rte_eth_dev_flow_ctrl_get()/_set() while the
device is transmitting/receiving.
Maybe it would be better to cache fc_conf in struct netdev_dpdk and call
_set() only if we have to make a change?
2016-07-22 6:18 GMT-07:00 Sugesh Chandran <sugesh.chandran at intel.com>:
> Add support for flow-control(mac control frame) to DPDK enabled physical
> port types. By default, the flow-control is OFF on both rx and tx side.
> The flow control can be enabled/disabled either when adding a port to OVS
> or at run time.
>
> For eg:
> To enable flow control support at tx side while adding a port, add the
> 'tx-flow-ctrl' option to the 'ovs-vsctl add-port' command-line as below.
>
> 'ovs-vsctl add-port br0 dpdk0 -- \
> set Interface dpdk0 type=dpdk options:tx-flow-ctrl=on'
>
> Similarly to enable rx flow control,
> 'ovs-vsctl add-port br0 dpdk0 -- \
> set Interface dpdk0 type=dpdk options:rx-flow-ctrl=on'
>
> And to enable the flow control auto-negotiation,
> 'ovs-vsctl add-port br0 dpdk0 -- \
> set Interface dpdk0 type=dpdk options:flow-ctrl-autoneg=on'
>
> To turn ON the tx flow control at run time(After the port is being added
> to OVS), the command-line input will be,
> 'ovs-vsctl set Interface dpdk0 options:tx-flow-ctrl=on'
>
> The flow control parameters can be turned off by setting 'off' to the
> respective parameter. To turn off the flow control at tx side,
> 'ovs-vsctl set Interface dpdk0 options:tx-flow-ctrl=off'
>
> Signed-off-by: Sugesh Chandran <sugesh.chandran at intel.com>
> ---
> lib/netdev-dpdk.c | 68
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 85b18fd..74efd25 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -634,6 +634,67 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int
> n_rxq, int n_txq)
> return diag;
> }
>
> +static void
> +dpdk_eth_parse_flow_ctrl(struct netdev_dpdk *dev,
> + const struct smap *args,
> + struct rte_eth_fc_conf *fc_conf)
> + OVS_REQUIRES(dev->mutex)
>
Minor: the other thread safety annotations are not aligned with the
parameters, but are just indented four spaces.
> +{
> + int ret = 0;
> + int rx_fc_en = 0;
> + int tx_fc_en = 0;
> + const char *rx_flow_mode;
> + const char *tx_flow_mode;
> + const char *flow_autoneg;
> + enum rte_eth_fc_mode fc_mode_set[2][2] = {{RTE_FC_NONE,
> RTE_FC_TX_PAUSE},
> + {RTE_FC_RX_PAUSE,
> RTE_FC_FULL}
> + };
> +
> + ret = rte_eth_dev_flow_ctrl_get(dev->port_id, fc_conf);
> + if (ret != 0) {
> + VLOG_DBG("cannot get flow control parameters on port=%d, err=%s",
> + dev->port_id, rte_strerror(ret));
>
I'm not sure, do we need to change 'ret' sign before passing it to
rte_strerror()?
> + return;
> + }
> + rx_flow_mode = smap_get(args, "rx-flow-ctrl");
> + tx_flow_mode = smap_get(args, "tx-flow-ctrl");
> + flow_autoneg = smap_get(args, "flow-ctrl-autoneg");
> + if (rx_flow_mode) {
> + if (!strcmp(rx_flow_mode, "on")) {
> + rx_fc_en = 1;
> + }
> + else if (!strcmp(rx_flow_mode, "off")) {
> + rx_fc_en = 0;
> + }
> + }
> + if (tx_flow_mode) {
> + if (!strcmp(tx_flow_mode, "on")) {
> + tx_fc_en =1;
> + }
> + else if (!strcmp(tx_flow_mode, "off")) {
> + tx_fc_en =0;
> + }
> + }
> + if (flow_autoneg) {
> + if (!strcmp(flow_autoneg, "on")) {
> + fc_conf->autoneg = 1;
> + }
> + else if (!strcmp(flow_autoneg, "off")) {
> + fc_conf->autoneg = 0;
> + }
> + }
> + fc_conf->mode = fc_mode_set[tx_fc_en][rx_fc_en];
> +}
> +
> +static void
> +dpdk_eth_flow_ctrl_config(struct netdev_dpdk *dev,
> + struct rte_eth_fc_conf *fc_conf)
> + OVS_REQUIRES(dev->mutex)
>
Minor: the other thread safety annotations are not aligned with the
parameters, but are just indented four spaces.
> +{
> + if (rte_eth_dev_flow_ctrl_set(dev->port_id, fc_conf) != 0) {
>
I'd drop the != 0
> + VLOG_ERR("Failed to enable flow control on device %d",
> dev->port_id);
>
VLOG_WARN is probably enough
> + }
> +}
>
> static int
> dpdk_eth_dev_init(struct netdev_dpdk *dev) OVS_REQUIRES(dpdk_mutex)
> @@ -991,6 +1052,13 @@ netdev_dpdk_set_config(struct netdev *netdev, const
> struct smap *args)
> dev->requested_n_rxq = new_n_rxq;
> netdev_request_reconfigure(netdev);
> }
> +
> + /* Flow control configuration for DPDK Ethernet ports. */
> + if (dev->type == DPDK_DEV_ETH) {
> + struct rte_eth_fc_conf fc_conf = {0};
> + dpdk_eth_parse_flow_ctrl(dev, args, &fc_conf);
>
I think it'd be better to extract the fields from 'args' here and pass them
to dpdk_eth_parse_flow_ctrl(), which would probably need a new name, but
that's just my preference.
> + dpdk_eth_flow_ctrl_config(dev, &fc_conf);
> + }
> ovs_mutex_unlock(&dev->mutex);
>
> return 0;
> --
> 2.5.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
More information about the dev
mailing list