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

Ilya Maximets i.maximets at samsung.com
Wed Sep 11 11:03:44 UTC 2019


On 11.09.2019 13:50, Konieczny, TomaszX wrote:
>> -----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. */
> 

Unfortunately, we can't say for sure, i.e. we can't distinguish two cases:
- User wanted to disable flow control relying on the default 'false' values.
  --> We want a warning printed for this case if not supported.
- User didn't configure flow control because he/she doesn't care about flow control.
  --> We're trying to avoid extra warnings for this case.

To solve this we need an extra config option like 'options:configure-flow-control'
or 'options:use-flow-control' (I'm not sure about the name). Another way is to change
the API by saying that if there are no flow control related options in DB, flow control
will not be touched (i.e. check 3 cases: true, false, no record in db).
But this will be an API change anyway that we're normally not backporting to previous
releases.

Best regards, Ilya Maximets.


More information about the dev mailing list