Regards _Sugesh
> -----Original Message----- > From: Ilya Maximets [mailto:i.maxim...@samsung.com] > Sent: Wednesday, September 21, 2016 10:50 AM > To: Chandran, Sugesh <sugesh.chand...@intel.com>; > dev@openvswitch.org; Daniele Di Proietto <diproiet...@vmware.com> > Cc: Dyasly Sergey <s.dya...@samsung.com>; Heetae Ahn > <heetae82....@samsung.com>; Bodireddy, Bhanuprakash > <bhanuprakash.bodire...@intel.com>; Loftus, Ciara > <ciara.lof...@intel.com> > Subject: Re: [PATCH] netdev-dpdk: Configure flow control only when > necessary. > > On 21.09.2016 11:26, Chandran, Sugesh wrote: > > > > > > Regards > > _Sugesh > > > >> -----Original Message----- > >> From: Ilya Maximets [mailto:i.maxim...@samsung.com] > >> Sent: Tuesday, September 20, 2016 10:52 AM > >> To: Chandran, Sugesh <sugesh.chand...@intel.com>; > >> dev@openvswitch.org; Daniele Di Proietto <diproiet...@vmware.com> > >> Cc: Dyasly Sergey <s.dya...@samsung.com>; Heetae Ahn > >> <heetae82....@samsung.com>; Bodireddy, Bhanuprakash > >> <bhanuprakash.bodire...@intel.com>; Loftus, Ciara > >> <ciara.lof...@intel.com> > >> Subject: Re: [PATCH] netdev-dpdk: Configure flow control only when > >> necessary. > >> > >> 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. > > [Sugesh] I would prefer to specify it explicitly. > > OK. > > >> > >>>> { > >>>> 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. > > [Sugesh] Agreed, if that's the case, it would nice to set default > > value '0' for any other value than '1'. What do you think? In this case you > are setting the flow director for all the values except '0'. > > Do you think that's the expected behavior than setting default for any > invalid inputs? > > 'smap_get_bool' returns boolean. 'true' will be converted to '1' and 'false' > to > 0. > Nothing else. User input will be verified by the database and converted to > boolean value. > Conversion of the boolean to integer is the compiler dependent operation. > That is the only reason for doing this explicitly using logical comparison. [Sugesh] Ok, Thanks for clarifying. Looks OK for me. > > >> > >>>> + 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
