| Message ID | 20190909113950.594-1-tomaszx.konieczny@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v1] netdev-dpdk: Fix flow control configuration. | expand |
On 09.09.2019 14:39, Tomasz Konieczny wrote: > Currently OVS is unable to change flow control configuration in DPDK > because new settings are being overwritten by current settings > with rte_eth_dev_flow_ctrl_get(). The fix restores correct order > of operations and at the same time does not trigger error on devices > without flow control support when flow control not requested. > > Fixes: d95a8d2b0bd6 ("netdev-dpdk: Fix flow control configuration.") > Signed-off-by: Tomasz Konieczny <tomaszx.konieczny@intel.com> > --- Hi Tomasz, Thanks for the fix! I agree that current code in master doesn't make any sense. :) It seems that no-one uses this functionality since it's broken for more than a year already. Fixes tag should point to the commit from master branch, so it should be: Fixes: 7e1de65e8dfb ("netdev-dpdk: Fix failure to configure flow control at netdev-init.") Regarding the fix itself: Can we just move following two lines: dev->fc_conf.mode = fc_mode; dev->fc_conf.autoneg = autoneg; below the rte_eth_dev_flow_ctrl_get() ? Current version of the patch will re-setup flow control on each call if it is not in initial state. Best regards, Ilya Maximets. > lib/netdev-dpdk.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index bc20d68..c02dea1 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1856,17 +1856,19 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, > rx_fc_en = smap_get_bool(args, "rx-flow-ctrl", false); > tx_fc_en = smap_get_bool(args, "tx-flow-ctrl", false); > autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false); > - > fc_mode = fc_mode_set[tx_fc_en][rx_fc_en]; > - if (dev->fc_conf.mode != fc_mode || autoneg != dev->fc_conf.autoneg) { > - dev->fc_conf.mode = fc_mode; > - dev->fc_conf.autoneg = autoneg; > + > + if (fc_mode != RTE_FC_NONE || autoneg != false) { > /* Get the Flow control configuration for DPDK-ETH */ > err = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf); > if (err) { > VLOG_WARN("Cannot get flow control parameters on port " > DPDK_PORT_ID_FMT", err=%d", dev->port_id, err); > } > + } > + if (dev->fc_conf.mode != fc_mode || autoneg != dev->fc_conf.autoneg) { > + dev->fc_conf.mode = fc_mode; > + dev->fc_conf.autoneg = autoneg; > dpdk_eth_flow_ctrl_setup(dev); > } > >
On 10.09.2019 12:29, Ilya Maximets wrote: > On 09.09.2019 14:39, Tomasz Konieczny wrote: >> Currently OVS is unable to change flow control configuration in DPDK >> because new settings are being overwritten by current settings >> with rte_eth_dev_flow_ctrl_get(). The fix restores correct order >> of operations and at the same time does not trigger error on devices >> without flow control support when flow control not requested. >> >> Fixes: d95a8d2b0bd6 ("netdev-dpdk: Fix flow control configuration.") >> Signed-off-by: Tomasz Konieczny <tomaszx.konieczny@intel.com> >> --- > > Hi Tomasz, > Thanks for the fix! > > I agree that current code in master doesn't make any sense. :) > It seems that no-one uses this functionality since it's broken > for more than a year already. > > Fixes tag should point to the commit from master branch, so it > should be: > Fixes: 7e1de65e8dfb ("netdev-dpdk: Fix failure to configure flow control at netdev-init.") > > Regarding the fix itself: > Can we just move following two lines: > > dev->fc_conf.mode = fc_mode; > dev->fc_conf.autoneg = autoneg; > > below the rte_eth_dev_flow_ctrl_get() ? > Current version of the patch will re-setup flow control on each call > if it is not in initial state. One more nit is that it's, probably, better to re-name this patch to be more specific. Especially because we already have equally named patch on other branch. (The patch still needs to have v2 in a subject prefix with re-naming mentioned in a version history.) > > Best regards, Ilya Maximets.
-----Original Message----- From: Ilya Maximets <i.maximets@samsung.com> Sent: 10 September 2019 11:44 To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; dev@openvswitch.org Cc: Stokes, Ian <ian.stokes@intel.com> Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Fix flow control configuration. On 10.09.2019 12:29, Ilya Maximets wrote: > On 09.09.2019 14:39, Tomasz Konieczny wrote: >> Currently OVS is unable to change flow control configuration in DPDK >> because new settings are being overwritten by current settings with >> rte_eth_dev_flow_ctrl_get(). The fix restores correct order of >> operations and at the same time does not trigger error on devices >> without flow control support when flow control not requested. >> >> Fixes: d95a8d2b0bd6 ("netdev-dpdk: Fix flow control configuration.") >> Signed-off-by: Tomasz Konieczny <tomaszx.konieczny@intel.com> >> --- > > Hi Tomasz, > Thanks for the fix! > > I agree that current code in master doesn't make any sense. :) It > seems that no-one uses this functionality since it's broken for more > than a year already. > > Fixes tag should point to the commit from master branch, so it should > be: > Fixes: 7e1de65e8dfb ("netdev-dpdk: Fix failure to configure flow > control at netdev-init.") > > Regarding the fix itself: > Can we just move following two lines: > > dev->fc_conf.mode = fc_mode; > dev->fc_conf.autoneg = autoneg; > > below the rte_eth_dev_flow_ctrl_get() ? > Current version of the patch will re-setup flow control on each call > if it is not in initial state. One more nit is that it's, probably, better to re-name this patch to be more specific. Especially because we already have equally named patch on other branch. (The patch still needs to have v2 in a subject prefix with re-naming mentioned in a version history.) > > Best regards, Ilya Maximets. Hi Ilya, Thank you for your comments. I will adhere to tag and name issues. Regarding the fix itself comment: These lines need to be run separately from get() in order to allow disabling already enabled flow control - get() will not run in this case. Also, as far as I understand, these lines and re-setup will only run when there is some change in flow control settings. if (dev->fc_conf.mode != fc_mode || autoneg != dev->fc_conf.autoneg)
On 10.09.2019 13:43, Konieczny, TomaszX wrote: > -----Original Message----- > From: Ilya Maximets <i.maximets@samsung.com> > Sent: 10 September 2019 11:44 > To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; dev@openvswitch.org > Cc: Stokes, Ian <ian.stokes@intel.com> > Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Fix flow control configuration. > > On 10.09.2019 12:29, Ilya Maximets wrote: >> On 09.09.2019 14:39, Tomasz Konieczny wrote: >>> Currently OVS is unable to change flow control configuration in DPDK >>> because new settings are being overwritten by current settings with >>> rte_eth_dev_flow_ctrl_get(). The fix restores correct order of >>> operations and at the same time does not trigger error on devices >>> without flow control support when flow control not requested. >>> >>> Fixes: d95a8d2b0bd6 ("netdev-dpdk: Fix flow control configuration.") >>> Signed-off-by: Tomasz Konieczny <tomaszx.konieczny@intel.com> >>> --- >> >> Hi Tomasz, >> Thanks for the fix! >> >> I agree that current code in master doesn't make any sense. :) It >> seems that no-one uses this functionality since it's broken for more >> than a year already. >> >> Fixes tag should point to the commit from master branch, so it should >> be: >> Fixes: 7e1de65e8dfb ("netdev-dpdk: Fix failure to configure flow >> control at netdev-init.") >> >> Regarding the fix itself: >> Can we just move following two lines: >> >> dev->fc_conf.mode = fc_mode; >> dev->fc_conf.autoneg = autoneg; >> >> below the rte_eth_dev_flow_ctrl_get() ? >> Current version of the patch will re-setup flow control on each call >> if it is not in initial state. > > One more nit is that it's, probably, better to re-name this patch to be more specific. Especially because we already have equally named patch on other branch. (The patch still needs to have v2 in a subject prefix with re-naming mentioned in a version history.) > >> >> Best regards, Ilya Maximets. > > Hi Ilya, > Thank you for your comments. > > I will adhere to tag and name issues. > > Regarding the fix itself comment: > These lines need to be run separately from get() in order to allow disabling already enabled > flow control - get() will not run in this case. I looked at the code once more and to the dpdk drivers' implementation. You're partially right, but we have to get() flow control configuration in all cases because some drivers (e.g. ixgbe) has flow control enabled by default and since we have 'false' as a default value, we need to disable the flow control for it. Not calling the get() will result in setting zeroized fc_config, which may be discarded by the driver since those values are validated inside. So, I think that we need to call get() function unconditionally. Avoiding of failure for devices that doesn't support flow control could be implemented like this: /* Get the Flow control configuration. */ err = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf); if (err) { if (err == -ENOTSUP) { VLOG_INFO_ONCE("%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; } What do you think? One minor nit is that it's better to keep an empty line here: --- autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false); - fc_mode = fc_mode_set[tx_fc_en][rx_fc_en]; --- Best regards, Ilya Maximets.
>-----Original Message----- >From: Ilya Maximets <i.maximets@samsung.com> >Sent: 10 September 2019 14:29 >To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; dev@openvswitch.org >Cc: Stokes, Ian <ian.stokes@intel.com> >Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Fix flow control configuration. > > >I looked at the code once more and to the dpdk drivers' implementation. >You're partially right, but we have to get() flow control configuration in all cases >because some drivers (e.g. ixgbe) has flow control enabled by default and since >we have 'false' as a default value, we need to disable the flow control for it. Not >calling the get() will result in setting zeroized fc_config, which may be discarded >by the driver since those values are validated inside. > >So, I think that we need to call get() function unconditionally. >Avoiding of failure for devices that doesn't support flow control could be >implemented like this: > > /* Get the Flow control configuration. */ > err = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf); > if (err) { > if (err == -ENOTSUP) { > VLOG_INFO_ONCE("%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; > } > >What do you think? > > >One minor nit is that it's better to keep an empty line here: >--- > autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false); >- > fc_mode = fc_mode_set[tx_fc_en][rx_fc_en]; >--- > >Best regards, Ilya Maximets. 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? Also, I've tested my patch on ixgbe device and it worked fine, though I still like your solution better.
On 11.09.2019 12:53, Konieczny, TomaszX wrote: >> -----Original Message----- >> From: Ilya Maximets <i.maximets@samsung.com> >> Sent: 10 September 2019 14:29 >> To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; dev@openvswitch.org >> Cc: Stokes, Ian <ian.stokes@intel.com> >> Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Fix flow control configuration. >> >> >> I looked at the code once more and to the dpdk drivers' implementation. >> You're partially right, but we have to get() flow control configuration in all cases >> because some drivers (e.g. ixgbe) has flow control enabled by default and since >> we have 'false' as a default value, we need to disable the flow control for it. Not >> calling the get() will result in setting zeroized fc_config, which may be discarded >> by the driver since those values are validated inside. >> >> So, I think that we need to call get() function unconditionally. >> Avoiding of failure for devices that doesn't support flow control could be >> implemented like this: >> >> /* Get the Flow control configuration. */ >> err = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf); >> if (err) { >> if (err == -ENOTSUP) { >> VLOG_INFO_ONCE("%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; >> } >> >> What do you think? >> >> >> One minor nit is that it's better to keep an empty line here: >> --- >> autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false); >> - >> fc_mode = fc_mode_set[tx_fc_en][rx_fc_en]; >> --- >> >> Best regards, Ilya Maximets. > > 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. >
>-----Original Message----- >From: Ilya Maximets <i.maximets@samsung.com> >Sent: 11 September 2019 12:08 >To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; dev@openvswitch.org >Cc: Stokes, Ian <ian.stokes@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; }
On 11.09.2019 13:16, Konieczny, TomaszX wrote: > > >> -----Original Message----- >> From: Ilya Maximets <i.maximets@samsung.com> >> Sent: 11 September 2019 12:08 >> To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; dev@openvswitch.org >> Cc: Stokes, Ian <ian.stokes@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.
>-----Original Message----- >From: Ilya Maximets <i.maximets@samsung.com> >Sent: 11 September 2019 12:26 >To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; dev@openvswitch.org >Cc: Stokes, Ian <ian.stokes@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@samsung.com> >>> Sent: 11 September 2019 12:08 >>> To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; >>> dev@openvswitch.org >>> Cc: Stokes, Ian <ian.stokes@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. */
On 11.09.2019 13:50, Konieczny, TomaszX wrote: >> -----Original Message----- >> From: Ilya Maximets <i.maximets@samsung.com> >> Sent: 11 September 2019 12:26 >> To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; dev@openvswitch.org >> Cc: Stokes, Ian <ian.stokes@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@samsung.com> >>>> Sent: 11 September 2019 12:08 >>>> To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; >>>> dev@openvswitch.org >>>> Cc: Stokes, Ian <ian.stokes@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.
>-----Original Message----- >From: Ilya Maximets <i.maximets@samsung.com> >Sent: 11 September 2019 13:04 >To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; dev@openvswitch.org >Cc: Stokes, Ian <ian.stokes@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.maximets@samsung.com> >>> Sent: 11 September 2019 12:26 >>> To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; >>> dev@openvswitch.org >>> Cc: Stokes, Ian <ian.stokes@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@samsung.com> >>>>> Sent: 11 September 2019 12:08 >>>>> To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; >>>>> dev@openvswitch.org >>>>> Cc: Stokes, Ian <ian.stokes@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.
On 11.09.2019 15:21, Konieczny, TomaszX wrote: >> -----Original Message----- >> From: Ilya Maximets <i.maximets@samsung.com> >> Sent: 11 September 2019 13:04 >> To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; dev@openvswitch.org >> Cc: Stokes, Ian <ian.stokes@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.maximets@samsung.com> >>>> Sent: 11 September 2019 12:26 >>>> To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; >>>> dev@openvswitch.org >>>> Cc: Stokes, Ian <ian.stokes@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@samsung.com> >>>>>> Sent: 11 September 2019 12:08 >>>>>> To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; >>>>>> dev@openvswitch.org >>>>>> Cc: Stokes, Ian <ian.stokes@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.
>-----Original Message----- >From: Ilya Maximets <i.maximets@samsung.com> >Sent: 11 September 2019 15:29 >To: Konieczny, TomaszX <tomaszx.konieczny@intel.com>; dev@openvswitch.org >Cc: Stokes, Ian <ian.stokes@intel.com> >Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Fix flow control configuration. > >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. Ok, that should work, I'll prepare v2. Regards Tomasz Konieczny --------------------------------------------------------------------- Intel Corporation (UK) Ltd. Co. Reg. #1134945 Pipers Way, Swindon SN3 1RJ
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index bc20d68..c02dea1 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1856,17 +1856,19 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, rx_fc_en = smap_get_bool(args, "rx-flow-ctrl", false); tx_fc_en = smap_get_bool(args, "tx-flow-ctrl", false); autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false); - fc_mode = fc_mode_set[tx_fc_en][rx_fc_en]; - if (dev->fc_conf.mode != fc_mode || autoneg != dev->fc_conf.autoneg) { - dev->fc_conf.mode = fc_mode; - dev->fc_conf.autoneg = autoneg; + + if (fc_mode != RTE_FC_NONE || autoneg != false) { /* Get the Flow control configuration for DPDK-ETH */ err = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf); if (err) { VLOG_WARN("Cannot get flow control parameters on port " DPDK_PORT_ID_FMT", err=%d", dev->port_id, err); } + } + if (dev->fc_conf.mode != fc_mode || autoneg != dev->fc_conf.autoneg) { + dev->fc_conf.mode = fc_mode; + dev->fc_conf.autoneg = autoneg; dpdk_eth_flow_ctrl_setup(dev); }
Currently OVS is unable to change flow control configuration in DPDK because new settings are being overwritten by current settings with rte_eth_dev_flow_ctrl_get(). The fix restores correct order of operations and at the same time does not trigger error on devices without flow control support when flow control not requested. Fixes: d95a8d2b0bd6 ("netdev-dpdk: Fix flow control configuration.") Signed-off-by: Tomasz Konieczny <tomaszx.konieczny@intel.com> --- lib/netdev-dpdk.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)