[ovs-dev] [PATCH v1] netdev-dpdk: Fix flow control configuration.

Ilya Maximets i.maximets at samsung.com
Tue Sep 10 12:29:08 UTC 2019


On 10.09.2019 13:43, Konieczny, TomaszX wrote:
> -----Original Message-----
> From: Ilya Maximets <i.maximets at samsung.com> 
> Sent: 10 September 2019 11:44
> To: Konieczny, TomaszX <tomaszx.konieczny at intel.com>; dev at openvswitch.org
> Cc: Stokes, Ian <ian.stokes at intel.com>
> Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Fix flow control configuration.
> 
> On 10.09.2019 12:29, Ilya Maximets wrote:
>> On 09.09.2019 14:39, Tomasz Konieczny wrote:
>>> Currently OVS is unable to change flow control configuration in DPDK 
>>> because new settings are being overwritten by current settings with 
>>> rte_eth_dev_flow_ctrl_get(). The fix restores correct order of 
>>> operations and at the same time does not trigger error on devices 
>>> without flow control support when flow control not requested.
>>>
>>> Fixes: d95a8d2b0bd6 ("netdev-dpdk: Fix flow control configuration.")
>>> Signed-off-by: Tomasz Konieczny <tomaszx.konieczny at intel.com>
>>> ---
>>
>> Hi Tomasz,
>> Thanks for the fix!
>>
>> I agree that current code in master doesn't make any sense. :) It 
>> seems that no-one uses this functionality since it's broken for more 
>> than a year already.
>>
>> Fixes tag should point to the commit from master branch, so it should 
>> be:
>> Fixes: 7e1de65e8dfb ("netdev-dpdk: Fix failure to configure flow 
>> control at netdev-init.")
>>
>> Regarding the fix itself:
>> Can we just move following two lines:
>>
>>         dev->fc_conf.mode = fc_mode;
>>         dev->fc_conf.autoneg = autoneg;
>>
>> below the rte_eth_dev_flow_ctrl_get() ?
>> Current version of the patch will re-setup flow control on each call 
>> if it is not in initial state.
> 
> One more nit is that it's, probably, better to re-name this patch to be more specific. Especially because we already have equally named patch on other branch. (The patch still needs to have v2 in a subject prefix with re-naming mentioned in a version history.)
> 
>>
>> Best regards, Ilya Maximets.
> 
> Hi Ilya,
> Thank you for your comments.
> 
> I will adhere to tag and name issues.
> 
> Regarding the fix itself comment:
> These lines need to be run separately from get() in order to allow disabling already enabled
> flow control - get() will not run in this case.

I looked at the code once more and to the dpdk drivers' implementation.
You're partially right, but we have to get() flow control configuration
in all cases because some drivers (e.g. ixgbe) has flow control enabled
by default and since we have 'false' as a default value, we need to disable
the flow control for it. Not calling the get() will result in setting
zeroized fc_config, which may be discarded by the driver since those
values are validated inside.

So, I think that we need to call get() function unconditionally.
Avoiding of failure for devices that doesn't support flow control could
be implemented like this:

    /* Get the Flow control configuration. */
    err = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);
    if (err) {
        if (err == -ENOTSUP) {
            VLOG_INFO_ONCE("%s: Flow control is not supported.",
                           netdev_get_name(netdev));
            err = 0; /* Not fatal. */
        } else {
            VLOG_WARN("%s: Cannot get flow control parameters: %s",
                      netdev_get_name(netdev), rte_strerror(-err));
        }
        goto out;
    }

What do you think?


One minor nit is that it's better to keep an empty line here:
---
     autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);
-
     fc_mode = fc_mode_set[tx_fc_en][rx_fc_en];
---

Best regards, Ilya Maximets.


More information about the dev mailing list