[ovs-dev] [PATCH v1] netdev-dpdk: Fix flow control configuration.

[Konieczny, TomaszX]

-----Original Message-----
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 
> 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)