On 11.09.2019 13:50, Konieczny, TomaszX wrote:
>> -----Original Message-----
>> From: Ilya Maximets <i.maxim...@samsung.com>
>> Sent: 11 September 2019 12:26
>> 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.
>>
>> On 11.09.2019 13:16, Konieczny, TomaszX wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Ilya Maximets <i.maxim...@samsung.com>
>>>> Sent: 11 September 2019 12:08
>>>> 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.
>>>>
>>>> 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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
