[dpdk-dev] [PATCH] examples/bond: wait for slaves to become active

[Nicolau, Radu]

I'm ok with the updates, thanks!

-----Original Message-----
From: Yigit, Ferruh 
Sent: Thursday, November 29, 2018 8:57 AM
To: Nicolau, Radu <radu.nicolau at intel.com>; dev at dpdk.org
Cc: Doherty, Declan <declan.doherty at intel.com>; chas3 at att.com
Subject: [dpdk-dev] [PATCH] examples/bond: wait for slaves to become active

On 11/28/2018 1:48 PM, Radu Nicolau wrote:
> Hi
> 
> 
> On 11/28/2018 11:08 AM, Ferruh Yigit wrote:
>> On 11/14/2018 12:19 PM, Radu Nicolau wrote:
>>> Do not start the packet processing threads until all configured 
>>> slaves become active.
>> Hi Radu,
>>
>> What happens if packet processing threads started before all slaves 
>> active? Exit app, error, crash?
>>
>> So can we say this patch is fixing packet forwarding? (fix in title?)
>>
>> And do we know what break it, why this was not required previously 
>> but required now? (Fixes line ?)
>  From what I see, the problem was always there: bond_ethdev_rx_burst 
> will cycle through slaves, but if called more times with no active 
> slaves the active slave index will point out of bounds, resulting in a 
> segfault.
> While this may require a better fix, this patch is an improvement even 
> if that fix comes - the configured slaves needs to be checked, and if 
> none became active there is no point going further.
> 
> in bond_ethdev_rx_burst:
> 
> slave_count = internals->active_slave_count; ...
>      if (++internals->active_slave == slave_count)
>          internals->active_slave = 0;
> slave_count is zero, the if() will never be true and active_slave will 
> be continuously incremented. It was not written to work with no active 
> slaves.

Thanks for clarification, are you OK with below updated commit log:

"
examples/bond: fix crash when there is no active slave

If bond_ethdev_rx_burst() called more times with no active slaves the active slave index will point out of bounds, resulting in a segfault.
The configured slaves needs to be checked, and if none became active there is no point going further.

Do not start the packet processing threads until all configured slaves become active.

Fixes: cc7e8ae84faa ("examples/bond: add example application for link bonding mode 6")
Cc: stable at dpdk.org

"


> 
>>
>> Thanks,
>> ferruh
>>
>>> Signed-off-by: Radu Nicolau <radu.nicolau at intel.com>
>>> ---
>>>   examples/bond/main.c | 15 +++++++++++++++
>>>   1 file changed, 15 insertions(+)
>>>
>>> diff --git a/examples/bond/main.c b/examples/bond/main.c index 
>>> b282e68..6623cae 100644
>>> --- a/examples/bond/main.c
>>> +++ b/examples/bond/main.c
>>> @@ -220,6 +220,7 @@ bond_port_init(struct rte_mempool *mbuf_pool)
>>>   	struct rte_eth_rxconf rxq_conf;
>>>   	struct rte_eth_txconf txq_conf;
>>>   	struct rte_eth_conf local_port_conf = port_conf;
>>> +	uint16_t wait_counter = 20;
>>>   
>>>   	retval = rte_eth_bond_create("net_bonding0", BONDING_MODE_ALB,
>>>   			0 /*SOCKET_ID_ANY*/);
>>> @@ -274,6 +275,20 @@ bond_port_init(struct rte_mempool *mbuf_pool)
>>>   	if (retval < 0)
>>>   		rte_exit(retval, "Start port %d failed (res=%d)", BOND_PORT, 
>>> retval);
>>>   
>>> +	printf("Waiting for slaves to become active...");
>>> +	while (wait_counter) {
>>> +		uint16_t act_slaves[16] = {0};
>>> +		if (rte_eth_bond_active_slaves_get(BOND_PORT, act_slaves, 16) ==
>>> +				slaves_count) {
>>> +			printf("\n");
>>> +			break;
>>> +		}
>>> +		sleep(1);
>>> +		printf("...");
>>> +		if (--wait_counter == 0)
>>> +			rte_exit(-1, "\nFailed to activate slaves\n");
>>> +	}
>>> +
>>>   	rte_eth_promiscuous_enable(BOND_PORT);
>>>   
>>>   	struct ether_addr addr;
>>>
>