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

Ilya Maximets i.maximets at samsung.com
Wed Sep 11 10:08:21 UTC 2019


On 11.09.2019 12:53, Konieczny, TomaszX wrote:
>> -----Original Message-----
>> From: Ilya Maximets <i.maximets at samsung.com>
>> Sent: 10 September 2019 14:29
>> 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.
>>
>>
>> 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.
> 
> Hi Ilya,
> 
> I agree, this approach looks neater and seems to work.
> However now if you try to enable flow control on unsupported device it will pass without any warnings or errors. Would it be a desired behavior?

We could change VLOG_INFO_ONCE to VLOG_WARN, in this case where will
be one warning in the log for each configuration attempt.
Basically, *_ONCE logging is not suitable here, because it will warn
only on the first device. My bad.

Ideally, we need a separate configuration knob to control if OVS
needs to touch flow control at all. And print a warning only if
user enabled this feature. I'm not sure how user-friendly is this.
For now, WARN on each configuration attempt might be fine.
So, s/VLOG_INFO_ONCE/VLOG_WARN/ .

What do you think?
Ian, maybe you have something to add?

> 
> Also, I've tested my patch on ixgbe device and it worked fine, though I still like your solution better.
> 


More information about the dev mailing list