On 11.09.2019 12:53, Konieczny, TomaszX wrote:
>> -----Original Message-----
>> From: Ilya Maximets <i.maxim...@samsung.com>
>> Sent: 10 September 2019 14:29
>> To: Konieczny, TomaszX <tomaszx.koniec...@intel.com>; d...@openvswitch.org
>> Cc: Stokes, Ian <ian.sto...@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.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
