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

[Ilya Maximets]

On 11.09.2019 13:16, Konieczny, TomaszX wrote:
> 
> 
>> -----Original Message-----
>> From: Ilya Maximets <i.maximets at samsung.com>
>> Sent: 11 September 2019 12:08
>> 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 11.09.2019 12:53, Konieczny, TomaszX wrote:
>>>
>>> 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.
> 
> I was thinking of something like this:
> 
> if (err) {
>         if (err == -ENOTSUP && fc_mode == RTE_FC_NONE && autoneg == false) {
>             VLOG_INFO_ONCE("%s: Flow control is not supported.",
>                            netdev_get_name(netdev));
>             err = 0; /* Not fatal. */
>         } else if (err == -ENOTSUP) {
>             VLOG_WARN("%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;
>     }
> 

As I said, we can't use VLOG_INFO_ONCE, because the message will be printed
only once in a process lifetime, i.e. only for the first netdev that will reach
this code. For all later added netdevs the message will not be printed.