>-----Original Message-----
>From: Ilya Maximets <i.maxim...@samsung.com>
>Sent: 11 September 2019 13:04
>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: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.
That might be an ideal situation, however I think it is slightly out of scope
for this patch to fix flow control working at all.
Besides, there is not much functional difference between flow control not
supported and disabled - it just doesn't work. Only if we try to enable flow
control on unsupported device we get a warning. Or we can just leave
info/warning on each configuration.
So I suggest to apply this as a simple fix patch, as it should be backported to
2.9 onward, and maybe continue developing later on.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
