[ovs-dev] [PATCH] netdev-dpdk: Configure flow control only when necessary.
Chandran, Sugesh
sugesh.chandran at intel.com
Wed Sep 21 10:42:48 UTC 2016
Regards
_Sugesh
> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets at samsung.com]
> Sent: Wednesday, September 21, 2016 10:50 AM
> To: Chandran, Sugesh <sugesh.chandran at intel.com>;
> dev at openvswitch.org; Daniele Di Proietto <diproiettod at vmware.com>
> Cc: Dyasly Sergey <s.dyasly at samsung.com>; Heetae Ahn
> <heetae82.ahn at samsung.com>; Bodireddy, Bhanuprakash
> <bhanuprakash.bodireddy at intel.com>; Loftus, Ciara
> <ciara.loftus at 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 at samsung.com]
> >> Sent: Tuesday, September 20, 2016 10:52 AM
> >> To: Chandran, Sugesh <sugesh.chandran at intel.com>;
> >> dev at openvswitch.org; Daniele Di Proietto <diproiettod at vmware.com>
> >> Cc: Dyasly Sergey <s.dyasly at samsung.com>; Heetae Ahn
> >> <heetae82.ahn at samsung.com>; Bodireddy, Bhanuprakash
> >> <bhanuprakash.bodireddy at intel.com>; Loftus, Ciara
> >> <ciara.loftus at 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 at samsung.com]
> >>>> Sent: Monday, September 19, 2016 2:34 PM
> >>>> To: dev at openvswitch.org; Daniele Di Proietto
> >> <diproiettod at vmware.com>
> >>>> Cc: Dyasly Sergey <s.dyasly at samsung.com>; Heetae Ahn
> >>>> <heetae82.ahn at samsung.com>; Chandran, Sugesh
> >>>> <sugesh.chandran at intel.com>; Bodireddy, Bhanuprakash
> >>>> <bhanuprakash.bodireddy at intel.com>; Loftus, Ciara
> >>>> <ciara.loftus at intel.com>; Ilya Maximets <i.maximets at 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 at 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
> >
> >
> >
More information about the dev
mailing list