Hi Bhanu and Daniele, Thank you for your time to review the patch, Please find my answers inline prefix with [Sugesh]
Regards _Sugesh From: Daniele Di Proietto [mailto:diproiet...@ovn.org] Sent: Thursday, July 28, 2016 1:38 AM To: Chandran, Sugesh <sugesh.chand...@intel.com>; Bodireddy, Bhanuprakash <bhanuprakash.bodire...@intel.com> Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH] netdev-dpdk: Add Flow Control support. Thanks for the patch. I agree, I'd be nice to document this in INSTALL.DPDK-ADVANCED.md<http://INSTALL.DPDK-ADVANCED.md> [Sugesh] Agreed, Will add that. We should also document the new fields in vswitchd/vswitch.xml. [Sugesh] Will do 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(). [Sugesh] Will update it. I assume it's ok to call rte_eth_dev_flow_ctrl_get()/_set() while the device is transmitting/receiving. [Sugesh], yes, it’s a run time parameter. Maybe it would be better to cache fc_conf in struct netdev_dpdk and call _set() only if we have to make a change? [Sugesh] : Will add it in the ‘netdev_dpdk’. 2016-07-22 6:18 GMT-07:00 Sugesh Chandran <sugesh.chand...@intel.com<mailto:sugesh.chand...@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.chand...@intel.com<mailto:sugesh.chand...@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. [Sugesh] Sorry, Will correct it. +{ + 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()? [Sugesh] : Either I can change it to rte_strerror(abs(ret)), or print the error code as is. All the existing ‘flow_ctrl_get’ driver implementation doesn’t return any error code in DPDK. It returns ‘0’ always. So I think its OK to just print the error code as it is. What do you think? + 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. [Sugesh] Will correct it. +{ + 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 [Sugesh] Ok. + } +} 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. [Sugesh] : Looks fine for me, Will extract the fields here and change the function to dpdk_eth_flow_ctrl_setup() + dpdk_eth_flow_ctrl_config(dev, &fc_conf); + } ovs_mutex_unlock(&dev->mutex); return 0; -- 2.5.0 _______________________________________________ dev mailing list dev@openvswitch.org<mailto:dev@openvswitch.org> http://openvswitch.org/mailman/listinfo/dev [Sugesh] : I will release v2 patch after incorporating the comments. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev