DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] examples/bond: wait for slaves to become active
@ 2018-11-14 12:19 Radu Nicolau
  2018-11-27 19:01 ` Chas Williams
  2018-11-28 11:08 ` Ferruh Yigit
  0 siblings, 2 replies; 10+ messages in thread
From: Radu Nicolau @ 2018-11-14 12:19 UTC (permalink / raw)
  To: dev; +Cc: declan.doherty, chas3, Radu Nicolau

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

Signed-off-by: Radu Nicolau <radu.nicolau@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;
-- 
2.7.5

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/bond: wait for slaves to become active
  2018-11-14 12:19 [dpdk-dev] [PATCH] examples/bond: wait for slaves to become active Radu Nicolau
@ 2018-11-27 19:01 ` Chas Williams
  2018-11-29 17:14   ` Ferruh Yigit
  2018-11-28 11:08 ` Ferruh Yigit
  1 sibling, 1 reply; 10+ messages in thread
From: Chas Williams @ 2018-11-27 19:01 UTC (permalink / raw)
  To: Radu Nicolau, dev; +Cc: declan.doherty, chas3



On 11/14/2018 07:19 AM, Radu Nicolau wrote:
> Do not start the packet processing threads until all configured
> slaves become active.
> 
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>

Acked-by: Chas Williams <chas3@att.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;
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/bond: wait for slaves to become active
  2018-11-14 12:19 [dpdk-dev] [PATCH] examples/bond: wait for slaves to become active Radu Nicolau
  2018-11-27 19:01 ` Chas Williams
@ 2018-11-28 11:08 ` Ferruh Yigit
  2018-11-28 13:48   ` Radu Nicolau
  1 sibling, 1 reply; 10+ messages in thread
From: Ferruh Yigit @ 2018-11-28 11:08 UTC (permalink / raw)
  To: Radu Nicolau, dev; +Cc: declan.doherty, chas3

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 ?)

Thanks,
ferruh

> 
> Signed-off-by: Radu Nicolau <radu.nicolau@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;
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/bond: wait for slaves to become active
  2018-11-28 11:08 ` Ferruh Yigit
@ 2018-11-28 13:48   ` Radu Nicolau
  2018-11-28 14:28     ` Chas Williams
  2018-11-29  8:57     ` Ferruh Yigit
  0 siblings, 2 replies; 10+ messages in thread
From: Radu Nicolau @ 2018-11-28 13:48 UTC (permalink / raw)
  To: Ferruh Yigit, dev; +Cc: declan.doherty, chas3

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,
> ferruh
>
>> Signed-off-by: Radu Nicolau <radu.nicolau@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;
>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/bond: wait for slaves to become active
  2018-11-28 13:48   ` Radu Nicolau
@ 2018-11-28 14:28     ` Chas Williams
  2018-11-28 16:04       ` Radu Nicolau
  2018-11-29  8:57     ` Ferruh Yigit
  1 sibling, 1 reply; 10+ messages in thread
From: Chas Williams @ 2018-11-28 14:28 UTC (permalink / raw)
  To: Radu Nicolau, Ferruh Yigit, dev; +Cc: declan.doherty, chas3



On 11/28/2018 08:48 AM, 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.

Just create another patch for the rx routines.  If the active_slave_count
is 0, there's nothing to do really.  It should just return and not
bother with any of the other work.

> 
>>
>> Thanks,
>> ferruh
>>
>>> Signed-off-by: Radu Nicolau <radu.nicolau@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;
>>>
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/bond: wait for slaves to become active
  2018-11-28 14:28     ` Chas Williams
@ 2018-11-28 16:04       ` Radu Nicolau
  2018-11-28 16:14         ` Chas Williams
  0 siblings, 1 reply; 10+ messages in thread
From: Radu Nicolau @ 2018-11-28 16:04 UTC (permalink / raw)
  To: Chas Williams, Ferruh Yigit, dev; +Cc: declan.doherty, chas3



On 11/28/2018 2:28 PM, Chas Williams wrote:
>
>
> On 11/28/2018 08:48 AM, 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.
>
> Just create another patch for the rx routines.  If the active_slave_count
> is 0, there's nothing to do really.  It should just return and not
> bother with any of the other work.
I can do that, and it will be the better fix I mentioned.
But I still think this patch makes the sample app better, at least it 
gives a hint to someone looking to develop its own app to check on the 
slaves' status before proceeding to rx.
>
>>
>>>
>>> Thanks,
>>> ferruh
>>>
>>>> Signed-off-by: Radu Nicolau <radu.nicolau@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;
>>>>
>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/bond: wait for slaves to become active
  2018-11-28 16:04       ` Radu Nicolau
@ 2018-11-28 16:14         ` Chas Williams
  0 siblings, 0 replies; 10+ messages in thread
From: Chas Williams @ 2018-11-28 16:14 UTC (permalink / raw)
  To: Radu Nicolau, Ferruh Yigit, dev; +Cc: declan.doherty, chas3



On 11/28/18 11:04 AM, Radu Nicolau wrote:
> 
> 
> On 11/28/2018 2:28 PM, Chas Williams wrote:
>>
>>
>> On 11/28/2018 08:48 AM, 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.
>>
>> Just create another patch for the rx routines.  If the active_slave_count
>> is 0, there's nothing to do really.  It should just return and not
>> bother with any of the other work.
> I can do that, and it will be the better fix I mentioned.
> But I still think this patch makes the sample app better, at least it 
> gives a hint to someone looking to develop its own app to check on the 
> slaves' status before proceeding to rx.

Yes, I agree this patch is still valid. If you are writing some sort of 
test, you should wait until the bonding interface and slaves are ready 
before you start sending traffic.

>>
>>>
>>>>
>>>> Thanks,
>>>> ferruh
>>>>
>>>>> Signed-off-by: Radu Nicolau <radu.nicolau@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;
>>>>>
>>>
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [dpdk-dev] [PATCH] examples/bond: wait for slaves to become active
  2018-11-28 13:48   ` Radu Nicolau
  2018-11-28 14:28     ` Chas Williams
@ 2018-11-29  8:57     ` Ferruh Yigit
  2018-11-29 12:11       ` Nicolau, Radu
  1 sibling, 1 reply; 10+ messages in thread
From: Ferruh Yigit @ 2018-11-29  8:57 UTC (permalink / raw)
  To: Radu Nicolau, dev; +Cc: declan.doherty, chas3

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@dpdk.org

"


> 
>>
>> Thanks,
>> ferruh
>>
>>> Signed-off-by: Radu Nicolau <radu.nicolau@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;
>>>
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/bond: wait for slaves to become active
  2018-11-29  8:57     ` Ferruh Yigit
@ 2018-11-29 12:11       ` Nicolau, Radu
  0 siblings, 0 replies; 10+ messages in thread
From: Nicolau, Radu @ 2018-11-29 12:11 UTC (permalink / raw)
  To: Yigit, Ferruh, dev; +Cc: Doherty, Declan, chas3

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@intel.com>; dev@dpdk.org
Cc: Doherty, Declan <declan.doherty@intel.com>; chas3@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@dpdk.org

"


> 
>>
>> Thanks,
>> ferruh
>>
>>> Signed-off-by: Radu Nicolau <radu.nicolau@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;
>>>
> 




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/bond: wait for slaves to become active
  2018-11-27 19:01 ` Chas Williams
@ 2018-11-29 17:14   ` Ferruh Yigit
  0 siblings, 0 replies; 10+ messages in thread
From: Ferruh Yigit @ 2018-11-29 17:14 UTC (permalink / raw)
  To: Chas Williams, Radu Nicolau, dev; +Cc: declan.doherty, chas3

On 11/27/2018 7:01 PM, Chas Williams wrote:
> 
> 
> On 11/14/2018 07:19 AM, Radu Nicolau wrote:
>> Do not start the packet processing threads until all configured
>> slaves become active.
>>
>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> 
> Acked-by: Chas Williams <chas3@att.com>

Applied to dpdk-next-net/master, thanks.

(commit log updated as discussed in mail thread)

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-11-29 17:14 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-14 12:19 [dpdk-dev] [PATCH] examples/bond: wait for slaves to become active Radu Nicolau
2018-11-27 19:01 ` Chas Williams
2018-11-29 17:14   ` Ferruh Yigit
2018-11-28 11:08 ` Ferruh Yigit
2018-11-28 13:48   ` Radu Nicolau
2018-11-28 14:28     ` Chas Williams
2018-11-28 16:04       ` Radu Nicolau
2018-11-28 16:14         ` Chas Williams
2018-11-29  8:57     ` Ferruh Yigit
2018-11-29 12:11       ` Nicolau, Radu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).