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

[Ilya Maximets]

On 29.09.2016 02:52, Daniele Di Proietto wrote:
> Why are the variables uint8_t instead of bool?
> 
> I think we shouldn't assume that converting to bool always returns 0 or 1,
> but the return value of smap_get_bool() is always 0 or 1 (as we always go
> through ! or !=).  I would remove the ternary operator

Ok. If you think that we can rely on implementation of 'smap_get_bool()'
let it be so. I'll send v3 soon.

> 
> Thanks,
> 
> Daniele
> 
> 2016-09-21 7:06 GMT-07:00 Chandran, Sugesh <sugesh.chandran at intel.com <mailto:sugesh.chandran at intel.com>>:
> 
>     Hi Ilya,
>     Thank you for the patch. It looks fine for me.
>     Also I verified that the flow control is working fine with the modification.
> 
> 
> 
>     Regards
>     _Sugesh
> 
> 
>     > -----Original Message-----
>     > From: Ilya Maximets [mailto:i.maximets at samsung.com <mailto:i.maximets at samsung.com>]
>     > Sent: Wednesday, September 21, 2016 11:39 AM
>     > To: dev at openvswitch.org <mailto:dev at openvswitch.org>; Daniele Di Proietto <diproiettod at vmware.com <mailto:diproiettod at vmware.com>>
>     > Cc: Dyasly Sergey <s.dyasly at samsung.com <mailto:s.dyasly at samsung.com>>; Heetae Ahn
>     > <heetae82.ahn at samsung.com <mailto:heetae82.ahn at samsung.com>>; Chandran, Sugesh
>     > <sugesh.chandran at intel.com <mailto:sugesh.chandran at intel.com>>; Bodireddy, Bhanuprakash
>     > <bhanuprakash.bodireddy at intel.com <mailto:bhanuprakash.bodireddy at intel.com>>; Loftus, Ciara
>     > <ciara.loftus at intel.com <mailto:ciara.loftus at intel.com>>
>     > Subject: Re: [PATCH v2] netdev-dpdk: Configure flow control only when
>     > necessary.
>     >
>     > On 21.09.2016 13 <tel:21.09.2016%2013>:35, Ilya Maximets wrote:
>     > > It is not necessary to touch the physical device each time, if the
>     > > configuration has not been changed. Also, few style issues fixed.
>     > >
>     > > Thread-safety annotation added to 'dpdk_set_rxq_config()'. It was
>     > > missed while previous refactoring of the flow control configuration.
>     > >
>     > > Signed-off-by: Ilya Maximets <i.maximets at samsung.com <mailto:i.maximets at samsung.com>>
>     > > ---
>     >
>     > Sorry, forget about the changelog:
>     >
>     > Version 2:
>     >
>     >       * Only commit-message updated. (thread-safety annotation)
>     >
>     > >  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
>     > > 89bdc4d..602217f 100644
>     > > --- a/lib/netdev-dpdk.c
>     > > +++ b/lib/netdev-dpdk.c
>     > > @@ -1059,6 +1059,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;
>     > >
>     > > @@ -1073,24 +1074,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);
>     > >
>     > >
>     _______________________________________________
>     dev mailing list
>     dev at openvswitch.org <mailto:dev at openvswitch.org>
>     http://openvswitch.org/mailman/listinfo/dev <http://openvswitch.org/mailman/listinfo/dev>
> 
>