Hi, Sugesh, Thanks for review. My comments inline. Bets regards, Ilya Maximets.
On 20.09.2016 11:41, Chandran, Sugesh wrote: > Hi Ilya, > Thank you for sending out the patch. > I verified that the patch is working fine. > Please find some comments below. > > Regards > _Sugesh > > >> -----Original Message----- >> From: Ilya Maximets [mailto:i.maxim...@samsung.com] >> Sent: Monday, September 19, 2016 2:34 PM >> To: dev@openvswitch.org; Daniele Di Proietto <diproiet...@vmware.com> >> Cc: Dyasly Sergey <s.dya...@samsung.com>; Heetae Ahn >> <heetae82....@samsung.com>; Chandran, Sugesh >> <sugesh.chand...@intel.com>; Bodireddy, Bhanuprakash >> <bhanuprakash.bodire...@intel.com>; Loftus, Ciara >> <ciara.lof...@intel.com>; Ilya Maximets <i.maxim...@samsung.com> >> Subject: [PATCH] netdev-dpdk: Configure flow control only when necessary. >> >> It is not necessary to touch the physical device each time, if the >> configuration has not been changed. Also, few style issues fixed. >> >> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com> >> --- >> lib/netdev-dpdk.c | 30 +++++++++++++++++------------- >> 1 file changed, 17 insertions(+), 13 deletions(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 6d334db..1632b97 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -1057,6 +1057,7 @@ netdev_dpdk_get_config(const struct netdev >> *netdev, struct smap *args) >> >> static void >> dpdk_set_rxq_config(struct netdev_dpdk *dev, const struct smap *args) >> + OVS_REQUIRES(dev->mutex) > [Sugesh] I assume this mutex is needed irrelevant of flow director > configuration. > Can you mention this as well in the commit message. You right. I just thought that this annotation can be treated as a style fix. If you disagree, I can mention it in a commit-message. >> { >> int new_n_rxq; >> >> @@ -1071,24 +1072,27 @@ static int >> netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args) >> { >> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >> + uint8_t rx_fc_en, tx_fc_en, autoneg; >> + enum rte_eth_fc_mode fc_mode; >> + static const enum rte_eth_fc_mode fc_mode_set[2][2] = { >> + {RTE_FC_NONE, RTE_FC_TX_PAUSE}, >> + {RTE_FC_RX_PAUSE, RTE_FC_FULL } >> + }; >> >> ovs_mutex_lock(&dev->mutex); >> >> dpdk_set_rxq_config(dev, args); >> >> - /* Flow control support is only available for DPDK Ethernet ports. */ >> - bool rx_fc_en = false; >> - bool tx_fc_en = false; >> - enum rte_eth_fc_mode fc_mode_set[2][2] = >> - {{RTE_FC_NONE, RTE_FC_TX_PAUSE}, >> - {RTE_FC_RX_PAUSE, RTE_FC_FULL} >> - }; >> - rx_fc_en = smap_get_bool(args, "rx-flow-ctrl", false); >> - tx_fc_en = smap_get_bool(args, "tx-flow-ctrl", false); >> - dev->fc_conf.autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false); >> - dev->fc_conf.mode = fc_mode_set[tx_fc_en][rx_fc_en]; >> - >> - dpdk_eth_flow_ctrl_setup(dev); >> + rx_fc_en = smap_get_bool(args, "rx-flow-ctrl", false) ? 1 : 0; > [Sugesh] smap_get_bool itself returns 1, 0 based on the input. I wouldn't use > the conditional operator to check the result. > Same comment for the following smap_get* as well. 'smap_get_bool()' returns 'bool'. And according to "C DIALECT" section of CodingStyle.md: " Most C99 features are OK because they are widely implemented: * bool and <stdbool.h>, but don't assume that bool or _Bool can only take on the values 0 or 1, because this behavior can't be simulated on C89 compilers. " This means that 'bool' must be directly converted to 0 or 1 before using as an index. >> + tx_fc_en = smap_get_bool(args, "tx-flow-ctrl", false) ? 1 : 0; >> + autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false) ? 1 : 0; >> + >> + fc_mode = fc_mode_set[tx_fc_en][rx_fc_en]; >> + if (dev->fc_conf.mode != fc_mode || autoneg != dev->fc_conf.autoneg) >> { >> + dev->fc_conf.mode = fc_mode; >> + dev->fc_conf.autoneg = autoneg; >> + dpdk_eth_flow_ctrl_setup(dev); >> + } >> >> ovs_mutex_unlock(&dev->mutex); >> >> -- >> 2.7.4 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
