| Message ID | 1474292013-715-1-git-send-email-i.maximets@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Daniele Di Proietto |
| Headers | show |
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.maximets@samsung.com] > Sent: Monday, September 19, 2016 2:34 PM > To: dev@openvswitch.org; Daniele Di Proietto <diproiettod@vmware.com> > Cc: Dyasly Sergey <s.dyasly@samsung.com>; Heetae Ahn > <heetae82.ahn@samsung.com>; Chandran, Sugesh > <sugesh.chandran@intel.com>; Bodireddy, Bhanuprakash > <bhanuprakash.bodireddy@intel.com>; Loftus, Ciara > <ciara.loftus@intel.com>; Ilya Maximets <i.maximets@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.maximets@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. > { > 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. > + 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
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.maximets@samsung.com] >> Sent: Monday, September 19, 2016 2:34 PM >> To: dev@openvswitch.org; Daniele Di Proietto <diproiettod@vmware.com> >> Cc: Dyasly Sergey <s.dyasly@samsung.com>; Heetae Ahn >> <heetae82.ahn@samsung.com>; Chandran, Sugesh >> <sugesh.chandran@intel.com>; Bodireddy, Bhanuprakash >> <bhanuprakash.bodireddy@intel.com>; Loftus, Ciara >> <ciara.loftus@intel.com>; Ilya Maximets <i.maximets@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.maximets@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
Regards _Sugesh > -----Original Message----- > From: Ilya Maximets [mailto:i.maximets@samsung.com] > Sent: Tuesday, September 20, 2016 10:52 AM > To: Chandran, Sugesh <sugesh.chandran@intel.com>; > dev@openvswitch.org; Daniele Di Proietto <diproiettod@vmware.com> > Cc: Dyasly Sergey <s.dyasly@samsung.com>; Heetae Ahn > <heetae82.ahn@samsung.com>; Bodireddy, Bhanuprakash > <bhanuprakash.bodireddy@intel.com>; Loftus, Ciara > <ciara.loftus@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.maximets@samsung.com] > >> Sent: Monday, September 19, 2016 2:34 PM > >> To: dev@openvswitch.org; Daniele Di Proietto > <diproiettod@vmware.com> > >> Cc: Dyasly Sergey <s.dyasly@samsung.com>; Heetae Ahn > >> <heetae82.ahn@samsung.com>; Chandran, Sugesh > >> <sugesh.chandran@intel.com>; Bodireddy, Bhanuprakash > >> <bhanuprakash.bodireddy@intel.com>; Loftus, Ciara > >> <ciara.loftus@intel.com>; Ilya Maximets <i.maximets@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.maximets@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. > > >> { > >> 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? > > >> + 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
On 21.09.2016 11:26, Chandran, Sugesh wrote: > > > Regards > _Sugesh > >> -----Original Message----- >> From: Ilya Maximets [mailto:i.maximets@samsung.com] >> Sent: Tuesday, September 20, 2016 10:52 AM >> To: Chandran, Sugesh <sugesh.chandran@intel.com>; >> dev@openvswitch.org; Daniele Di Proietto <diproiettod@vmware.com> >> Cc: Dyasly Sergey <s.dyasly@samsung.com>; Heetae Ahn >> <heetae82.ahn@samsung.com>; Bodireddy, Bhanuprakash >> <bhanuprakash.bodireddy@intel.com>; Loftus, Ciara >> <ciara.loftus@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.maximets@samsung.com] >>>> Sent: Monday, September 19, 2016 2:34 PM >>>> To: dev@openvswitch.org; Daniele Di Proietto >> <diproiettod@vmware.com> >>>> Cc: Dyasly Sergey <s.dyasly@samsung.com>; Heetae Ahn >>>> <heetae82.ahn@samsung.com>; Chandran, Sugesh >>>> <sugesh.chandran@intel.com>; Bodireddy, Bhanuprakash >>>> <bhanuprakash.bodireddy@intel.com>; Loftus, Ciara >>>> <ciara.loftus@intel.com>; Ilya Maximets <i.maximets@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.maximets@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. >> >>>> + 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 > > >
Regards _Sugesh > -----Original Message----- > From: Ilya Maximets [mailto:i.maximets@samsung.com] > Sent: Wednesday, September 21, 2016 10:50 AM > To: Chandran, Sugesh <sugesh.chandran@intel.com>; > dev@openvswitch.org; Daniele Di Proietto <diproiettod@vmware.com> > Cc: Dyasly Sergey <s.dyasly@samsung.com>; Heetae Ahn > <heetae82.ahn@samsung.com>; Bodireddy, Bhanuprakash > <bhanuprakash.bodireddy@intel.com>; Loftus, Ciara > <ciara.loftus@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.maximets@samsung.com] > >> Sent: Tuesday, September 20, 2016 10:52 AM > >> To: Chandran, Sugesh <sugesh.chandran@intel.com>; > >> dev@openvswitch.org; Daniele Di Proietto <diproiettod@vmware.com> > >> Cc: Dyasly Sergey <s.dyasly@samsung.com>; Heetae Ahn > >> <heetae82.ahn@samsung.com>; Bodireddy, Bhanuprakash > >> <bhanuprakash.bodireddy@intel.com>; Loftus, Ciara > >> <ciara.loftus@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.maximets@samsung.com] > >>>> Sent: Monday, September 19, 2016 2:34 PM > >>>> To: dev@openvswitch.org; Daniele Di Proietto > >> <diproiettod@vmware.com> > >>>> Cc: Dyasly Sergey <s.dyasly@samsung.com>; Heetae Ahn > >>>> <heetae82.ahn@samsung.com>; Chandran, Sugesh > >>>> <sugesh.chandran@intel.com>; Bodireddy, Bhanuprakash > >>>> <bhanuprakash.bodireddy@intel.com>; Loftus, Ciara > >>>> <ciara.loftus@intel.com>; Ilya Maximets <i.maximets@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.maximets@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 > > > > > >
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) { 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; + 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);
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.maximets@samsung.com> --- lib/netdev-dpdk.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)