[ovs-dev] [PATCH v1] netdev-dpdk: Fix flow control configuration.
tomaszx.konieczny at intel.com
Tue Sep 10 10:43:19 UTC 2019
From: Ilya Maximets <i.maximets at samsung.com>
Sent: 10 September 2019 11:44
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 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 at 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
> 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.
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)
More information about the dev