[ovs-dev] [PATCH] netdev-dpdk: Configure flow control only when necessary.

[Chandran, Sugesh]

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.
> 
> >>  {
> >>      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