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

Konieczny, TomaszX tomaszx.konieczny at intel.com
Wed Sep 11 10:50:11 UTC 2019


>-----Original Message-----
>From: Ilya Maximets <i.maximets at samsung.com>
>Sent: 11 September 2019 12:26
>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 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.

So we might as well omit any VLOG if flow control not supported and not requested.
         if (err == -ENOTSUP && fc_mode == RTE_FC_NONE && autoneg == false) {
             err = 0; /* Not fatal. Not requested. */


More information about the dev mailing list