diff mbox

[dpdk-dev,v2] net/bonding: fix update link status on slave add

Message ID 1527777275-9974-1-git-send-email-radu.nicolau@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Radu Nicolau May 31, 2018, 2:34 p.m. UTC 
  Add a call to rte_eth_link_get_nowait on every slave to update
the internal link status struct. Otherwise slave add will fail
for mode 4 if the ports are all stopped but only one of them checked.

Fixes: b77d21cc2364 ("ethdev: add link status get/set helper functions")
Bugzilla entry: https://dpdk.org/tracker/show_bug.cgi?id=52

Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
---
v2: add fix and Bugzilla references

 drivers/net/bonding/rte_eth_bond_api.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Ferruh Yigit May 31, 2018, 3:34 p.m. UTC | #1 
On 5/31/2018 3:34 PM, Radu Nicolau wrote:

I can see you just prefix "fix" to the title without updating it :)

What about following one:
"net/bonding: fix slave add for mode 4" ?

> Add a call to rte_eth_link_get_nowait on every slave to update
> the internal link status struct. Otherwise slave add will fail
> for mode 4 if the ports are all stopped but only one of them checked.

What is the link related expectation from slaves in mode 4?

What does "if the ports are all stopped but only one of them checked" mean, why
checking only one of them?

> 
> Fixes: b77d21cc2364 ("ethdev: add link status get/set helper functions")
> Bugzilla entry: https://dpdk.org/tracker/show_bug.cgi?id=52
> 
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> ---
> v2: add fix and Bugzilla references
> 
>  drivers/net/bonding/rte_eth_bond_api.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
> index d558df8..cad08b9 100644
> --- a/drivers/net/bonding/rte_eth_bond_api.c
> +++ b/drivers/net/bonding/rte_eth_bond_api.c
> @@ -296,6 +296,8 @@ __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t slave_port_id)
>  		return -1;
>  	}
>  
> +	rte_eth_link_get_nowait(slave_port_id, &link_props);
> +

The error seems in link_properties_valid(), does it make sense to get link info
inside that function before link checks?

>  	slave_add(internals, slave_eth_dev);
>  
>  	/* We need to store slaves reta_size to be able to synchronize RETA for all
>
Ferruh Yigit May 31, 2018, 3:36 p.m. UTC | #2 
On 5/31/2018 4:34 PM, Ferruh Yigit wrote:
> On 5/31/2018 3:34 PM, Radu Nicolau wrote:
> 
> I can see you just prefix "fix" to the title without updating it :)
> 
> What about following one:
> "net/bonding: fix slave add for mode 4" ?
> 
>> Add a call to rte_eth_link_get_nowait on every slave to update
>> the internal link status struct. Otherwise slave add will fail
>> for mode 4 if the ports are all stopped but only one of them checked.
> 
> What is the link related expectation from slaves in mode 4?
> 
> What does "if the ports are all stopped but only one of them checked" mean, why
> checking only one of them?
> 
>>
>> Fixes: b77d21cc2364 ("ethdev: add link status get/set helper functions")
>> Bugzilla entry: https://dpdk.org/tracker/show_bug.cgi?id=52

Bugzilla ID: 52

btw, can you please send new version as reply to previous version?

>>
>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>> ---
>> v2: add fix and Bugzilla references
>>
>>  drivers/net/bonding/rte_eth_bond_api.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
>> index d558df8..cad08b9 100644
>> --- a/drivers/net/bonding/rte_eth_bond_api.c
>> +++ b/drivers/net/bonding/rte_eth_bond_api.c
>> @@ -296,6 +296,8 @@ __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t slave_port_id)
>>  		return -1;
>>  	}
>>  
>> +	rte_eth_link_get_nowait(slave_port_id, &link_props);
>> +
> 
> The error seems in link_properties_valid(), does it make sense to get link info
> inside that function before link checks?
> 
>>  	slave_add(internals, slave_eth_dev);
>>  
>>  	/* We need to store slaves reta_size to be able to synchronize RETA for all
>>
>
Radu Nicolau May 31, 2018, 4:13 p.m. UTC | #3 
On 5/31/2018 4:36 PM, Ferruh Yigit wrote:
> On 5/31/2018 4:34 PM, Ferruh Yigit wrote:
>> On 5/31/2018 3:34 PM, Radu Nicolau wrote:
>>
>> I can see you just prefix "fix" to the title without updating it :)
>>
>> What about following one:
>> "net/bonding: fix slave add for mode 4" ?
Great, I'll use it for v3 :)

>>
>>> Add a call to rte_eth_link_get_nowait on every slave to update
>>> the internal link status struct. Otherwise slave add will fail
>>> for mode 4 if the ports are all stopped but only one of them checked.
>> What is the link related expectation from slaves in mode 4?
To be identical across all ports
>>
>> What does "if the ports are all stopped but only one of them checked" mean, why
>> checking only one of them?
This is the behavior of testpmd, stop getting the link status after the 
first down port; but this should not affect bonding, so there is no need 
to update testpmd.

>>
>>> Fixes: b77d21cc2364 ("ethdev: add link status get/set helper functions")
>>> Bugzilla entry: https://dpdk.org/tracker/show_bug.cgi?id=52
> Bugzilla ID: 52
>
> btw, can you please send new version as reply to previous version?
Sure.

>
>>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>>> ---
>>> v2: add fix and Bugzilla references
>>>
>>>   drivers/net/bonding/rte_eth_bond_api.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
>>> index d558df8..cad08b9 100644
>>> --- a/drivers/net/bonding/rte_eth_bond_api.c
>>> +++ b/drivers/net/bonding/rte_eth_bond_api.c
>>> @@ -296,6 +296,8 @@ __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t slave_port_id)
>>>   		return -1;
>>>   	}
>>>   
>>> +	rte_eth_link_get_nowait(slave_port_id, &link_props);
>>> +
>> The error seems in link_properties_valid(), does it make sense to get link info
>> inside that function before link checks?
Not really, as one might expect that link_properties_valid will only 
test the struct rte_eth_link *slave_link argument, not update it.

>>
>>>   	slave_add(internals, slave_eth_dev);
>>>   
>>>   	/* We need to store slaves reta_size to be able to synchronize RETA for all
>>>
Ferruh Yigit May 31, 2018, 4:32 p.m. UTC | #4 
On 5/31/2018 5:13 PM, Radu Nicolau wrote:
> 
> 
> On 5/31/2018 4:36 PM, Ferruh Yigit wrote:
>> On 5/31/2018 4:34 PM, Ferruh Yigit wrote:
>>> On 5/31/2018 3:34 PM, Radu Nicolau wrote:
>>>
>>> I can see you just prefix "fix" to the title without updating it :)
>>>
>>> What about following one:
>>> "net/bonding: fix slave add for mode 4" ?
> Great, I'll use it for v3 :)
> 
>>>
>>>> Add a call to rte_eth_link_get_nowait on every slave to update
>>>> the internal link status struct. Otherwise slave add will fail
>>>> for mode 4 if the ports are all stopped but only one of them checked.
>>> What is the link related expectation from slaves in mode 4?
> To be identical across all ports
>>>
>>> What does "if the ports are all stopped but only one of them checked" mean, why
>>> checking only one of them?
> This is the behavior of testpmd, stop getting the link status after the 
> first down port; but this should not affect bonding, so there is no need 
> to update testpmd.

I see, when this link updating happens in this bonding issue context? When
bonding device created?

Should we update testpmd behavior too?

> 
>>>
>>>> Fixes: b77d21cc2364 ("ethdev: add link status get/set helper functions")
>>>> Bugzilla entry: https://dpdk.org/tracker/show_bug.cgi?id=52
>> Bugzilla ID: 52
>>
>> btw, can you please send new version as reply to previous version?
> Sure.
> 
>>
>>>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>>>> ---
>>>> v2: add fix and Bugzilla references
>>>>
>>>>   drivers/net/bonding/rte_eth_bond_api.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
>>>> index d558df8..cad08b9 100644
>>>> --- a/drivers/net/bonding/rte_eth_bond_api.c
>>>> +++ b/drivers/net/bonding/rte_eth_bond_api.c
>>>> @@ -296,6 +296,8 @@ __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t slave_port_id)
>>>>   		return -1;
>>>>   	}
>>>>   
>>>> +	rte_eth_link_get_nowait(slave_port_id, &link_props);
>>>> +
>>> The error seems in link_properties_valid(), does it make sense to get link info
>>> inside that function before link checks?
> Not really, as one might expect that link_properties_valid will only 
> test the struct rte_eth_link *slave_link argument, not update it.

Fair enough, I just thought to be sure the tested link is up to date, but that
function seems only called by __eth_bond_slave_add_lock_free() which you are
updating, so this is ok.

> 
>>>
>>>>   	slave_add(internals, slave_eth_dev);
>>>>   
>>>>   	/* We need to store slaves reta_size to be able to synchronize RETA for all
>>>>
>
Radu Nicolau June 1, 2018, 10:18 a.m. UTC | #5 
On 5/31/2018 5:32 PM, Ferruh Yigit wrote:
> On 5/31/2018 5:13 PM, Radu Nicolau wrote:
>>
>> On 5/31/2018 4:36 PM, Ferruh Yigit wrote:
>>> On 5/31/2018 4:34 PM, Ferruh Yigit wrote:
>>>> On 5/31/2018 3:34 PM, Radu Nicolau wrote:
>>>>
>>>> I can see you just prefix "fix" to the title without updating it :)
>>>>
>>>> What about following one:
>>>> "net/bonding: fix slave add for mode 4" ?
>> Great, I'll use it for v3 :)
>>
>>>>> Add a call to rte_eth_link_get_nowait on every slave to update
>>>>> the internal link status struct. Otherwise slave add will fail
>>>>> for mode 4 if the ports are all stopped but only one of them checked.
>>>> What is the link related expectation from slaves in mode 4?
>> To be identical across all ports
>>>> What does "if the ports are all stopped but only one of them checked" mean, why
>>>> checking only one of them?
>> This is the behavior of testpmd, stop getting the link status after the
>> first down port; but this should not affect bonding, so there is no need
>> to update testpmd.
> I see, when this link updating happens in this bonding issue context? When
> bonding device created?
>
> Should we update testpmd behavior too?
Yes, I think that stop_port(portid_t pid) may need some rework. I'm not 
sure I understand the reason it calls check_all_ports_link_status(), for 
example.
Also, check_all_ports_link_status should do what it implies it does, 
check all ports, not stop at the first down port.

>
>>>>> Fixes: b77d21cc2364 ("ethdev: add link status get/set helper functions")
>>>>> Bugzilla entry: https://dpdk.org/tracker/show_bug.cgi?id=52
>>> Bugzilla ID: 52
>>>
>>> btw, can you please send new version as reply to previous version?
>> Sure.
>>
>>>>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>>>>> ---
>>>>> v2: add fix and Bugzilla references
>>>>>
>>>>>    drivers/net/bonding/rte_eth_bond_api.c | 2 ++
>>>>>    1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
>>>>> index d558df8..cad08b9 100644
>>>>> --- a/drivers/net/bonding/rte_eth_bond_api.c
>>>>> +++ b/drivers/net/bonding/rte_eth_bond_api.c
>>>>> @@ -296,6 +296,8 @@ __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t slave_port_id)
>>>>>    		return -1;
>>>>>    	}
>>>>>    
>>>>> +	rte_eth_link_get_nowait(slave_port_id, &link_props);
>>>>> +
>>>> The error seems in link_properties_valid(), does it make sense to get link info
>>>> inside that function before link checks?
>> Not really, as one might expect that link_properties_valid will only
>> test the struct rte_eth_link *slave_link argument, not update it.
> Fair enough, I just thought to be sure the tested link is up to date, but that
> function seems only called by __eth_bond_slave_add_lock_free() which you are
> updating, so this is ok.
>
>>>>>    	slave_add(internals, slave_eth_dev);
>>>>>    
>>>>>    	/* We need to store slaves reta_size to be able to synchronize RETA for all
>>>>>
diff mbox

Patch

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index d558df8..cad08b9 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -296,6 +296,8 @@  __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t slave_port_id)
 		return -1;
 	}
 
+	rte_eth_link_get_nowait(slave_port_id, &link_props);
+
 	slave_add(internals, slave_eth_dev);
 
 	/* We need to store slaves reta_size to be able to synchronize RETA for all