On 11.09.2019 15:21, Konieczny, TomaszX wrote:
>> -----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.
> 

OK. We could, actually, check if user configured something for flow control
in following way:

    flow_control_requested = true;
    if (!smap_get(args, "rx-flow-ctrl") && !smap_get(args, "tx-flow-ctrl")
        && !smap_get(args, "flow-ctrl-autoneg")) {
        /* FIXME: User didn't ask for flow control configuration.
         *        For now we'll not print a warning if flow control is not
         *        supported by the DPDK port. */
        flow_control_requested = false;
    }

And warn only if something was requested: 

        if (err == -ENOTSUP) {
            if (flow_control_requested) {
                VLOG_WARN("%s: Flow control is not supported.",
                          netdev_get_name(netdev));
            }
            err = 0; /* Not fatal. */
         }

Alternatively we could dynamically choose the log level in case we want
some message printed in both cases:

  VLOG(flow_control_requested ? VLL_WARN : VLL_INFO,
       "%s: Flow control is not supported.", netdev_get_name(netdev));

VLL_INFO could be changed to VLL_DBG if you think we shouldn't bother
users with additional information.

Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to