[v2,2/2] net/bonding: fix MAC address when one port resets
diff mbox series

Message ID 20200417081918.14029-3-huwei013@chinasoftinc.com
State New
Delegated to: Ferruh Yigit
Headers show
Series
  • fixes for bonding
Related show

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/travis-robot warning Travis build: failed
ci/checkpatch success coding style OK

Commit Message

Wei Hu (Xavier) April 17, 2020, 8:19 a.m. UTC
From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com>

Currently, based on a active-backup bond device, in the following 2 cases:
1) The primary port resets. The link status of the primary port changes
   from up to down.
2) When switching the active port, one slave port resets at the same time.
one slave port changes to the primary port, but the new primary port's MAC
address probably cannot change to the bond device's MAC address. And we
can't continue receive packets whose destination MAC addresses are the same
as the bond devices's MAC address.

The current bonding PMD driver call mac_address_slaves_update function to
modify the MAC address of all slaves devices. In mac_address_slaves_update
function, the rte_eth_dev_default_mac_addr_set API function is called to
set the MAC address of the slave devices in turn in the for loop statement.

When one port reset, calling rte_eth_dev_default_mac_addr_set API fails
because the firmware will not respond to the commands from the driver,
and exit the loop, so other slave devices cannot continue to update the
MAC address.

This patch fixes the issue by avoid exiting the loop when calling
rte_eth_dev_default_mac_addr_set fails.

Fixes: 2efb58cbab6e ("bond: new link bonding library")
Cc: stable@dpdk.org

Signed-off-by: Hongbo Zheng <zhenghongbo3@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
Signed-off-by: Chunsong Feng <fengchunsong@huawei.com>
Signed-off-by: Xuan Li <lixuan47@hisilicon.com>
---
v1 -> v2:
	Ignore the failure when updating salves's MAC address in the
	mac_address_slaves_update function, because it doesn't affect
	the bond's functional characteristics. The related link about
	the discussion:
	http://patches.dpdk.org/patch/66033/
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Wei Hu (Xavier) April 17, 2020, 8:58 a.m. UTC | #1
Hi, Chas Williams
    Thanks for your comments on Patch V1. now we have sent Patch V2.
    Could you please give some suggestion on them?
    Thanks.

    Best Regards
Xavier

On 2020/4/17 16:19, Wei Hu (Xavier) wrote:
> From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com>
> 
> Currently, based on a active-backup bond device, in the following 2 cases:
> 1) The primary port resets. The link status of the primary port changes
>     from up to down.
> 2) When switching the active port, one slave port resets at the same time.
> one slave port changes to the primary port, but the new primary port's MAC
> address probably cannot change to the bond device's MAC address. And we
> can't continue receive packets whose destination MAC addresses are the same
> as the bond devices's MAC address.
> 
> The current bonding PMD driver call mac_address_slaves_update function to
> modify the MAC address of all slaves devices. In mac_address_slaves_update
> function, the rte_eth_dev_default_mac_addr_set API function is called to
> set the MAC address of the slave devices in turn in the for loop statement.
> 
> When one port reset, calling rte_eth_dev_default_mac_addr_set API fails
> because the firmware will not respond to the commands from the driver,
> and exit the loop, so other slave devices cannot continue to update the
> MAC address.
> 
> This patch fixes the issue by avoid exiting the loop when calling
> rte_eth_dev_default_mac_addr_set fails.
> 
> Fixes: 2efb58cbab6e ("bond: new link bonding library")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Hongbo Zheng <zhenghongbo3@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> Signed-off-by: Chunsong Feng <fengchunsong@huawei.com>
> Signed-off-by: Xuan Li <lixuan47@hisilicon.com>
> ---
> v1 -> v2:
> 	Ignore the failure when updating salves's MAC address in the
> 	mac_address_slaves_update function, because it doesn't affect
> 	the bond's functional characteristics. The related link about
> 	the discussion:
> 	http://patches.dpdk.org/patch/66033/
> ---
>   drivers/net/bonding/rte_eth_bond_pmd.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index ddae3518c..01c0f6eb1 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -1502,6 +1502,7 @@ int
>   mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
>   {
>   	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
> +	bool setted;
>   	int i;
>   
>   	/* Update slave devices MAC addresses */
> @@ -1529,6 +1530,7 @@ mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
>   	case BONDING_MODE_TLB:
>   	case BONDING_MODE_ALB:
>   	default:
> +		setted = true;
>   		for (i = 0; i < internals->slave_count; i++) {
>   			if (internals->slaves[i].port_id ==
>   					internals->current_primary_port) {
> @@ -1537,7 +1539,7 @@ mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
>   						bonded_eth_dev->data->mac_addrs)) {
>   					RTE_BOND_LOG(ERR, "Failed to update port Id %d MAC address",
>   							internals->current_primary_port);
> -					return -1;
> +					setted = false;
>   				}
>   			} else {
>   				if (rte_eth_dev_default_mac_addr_set(
> @@ -1545,10 +1547,11 @@ mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
>   						&internals->slaves[i].persisted_mac_addr)) {
>   					RTE_BOND_LOG(ERR, "Failed to update port Id %d MAC address",
>   							internals->slaves[i].port_id);
> -					return -1;
>   				}
>   			}
>   		}
> +		if (!setted)
> +			return -1;
>   	}
>   
>   	return 0;
>
Wei Hu (Xavier) May 15, 2020, 3:11 a.m. UTC | #2
Hi, Ferruh Yigit & Chas Williams
   Could you please give any suggestion?
   Thanks.

   Best Regards
Xavier

On 2020/4/17 16:58, Wei Hu (Xavier) wrote:
> Hi, Chas Williams
>     Thanks for your comments on Patch V1. now we have sent Patch V2.
>     Could you please give some suggestion on them?
>     Thanks.
> 
>     Best Regards
> Xavier
> 
> On 2020/4/17 16:19, Wei Hu (Xavier) wrote:
>> From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com>
>>
>> Currently, based on a active-backup bond device, in the following 2 
>> cases:
>> 1) The primary port resets. The link status of the primary port changes
>>     from up to down.
>> 2) When switching the active port, one slave port resets at the same 
>> time.
>> one slave port changes to the primary port, but the new primary port's 
>> MAC
>> address probably cannot change to the bond device's MAC address. And we
>> can't continue receive packets whose destination MAC addresses are the 
>> same
>> as the bond devices's MAC address.
>>
>> The current bonding PMD driver call mac_address_slaves_update function to
>> modify the MAC address of all slaves devices. In 
>> mac_address_slaves_update
>> function, the rte_eth_dev_default_mac_addr_set API function is called to
>> set the MAC address of the slave devices in turn in the for loop 
>> statement.
>>
>> When one port reset, calling rte_eth_dev_default_mac_addr_set API fails
>> because the firmware will not respond to the commands from the driver,
>> and exit the loop, so other slave devices cannot continue to update the
>> MAC address.
>>
>> This patch fixes the issue by avoid exiting the loop when calling
>> rte_eth_dev_default_mac_addr_set fails.
>>
>> Fixes: 2efb58cbab6e ("bond: new link bonding library")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Hongbo Zheng <zhenghongbo3@huawei.com>
>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>> Signed-off-by: Chunsong Feng <fengchunsong@huawei.com>
>> Signed-off-by: Xuan Li <lixuan47@hisilicon.com>
>> ---
>> v1 -> v2:
>>     Ignore the failure when updating salves's MAC address in the
>>     mac_address_slaves_update function, because it doesn't affect
>>     the bond's functional characteristics. The related link about
>>     the discussion:
>>     http://patches.dpdk.org/patch/66033/
>> ---
>>   drivers/net/bonding/rte_eth_bond_pmd.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>> index ddae3518c..01c0f6eb1 100644
>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>> @@ -1502,6 +1502,7 @@ int
>>   mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
>>   {
>>       struct bond_dev_private *internals = 
>> bonded_eth_dev->data->dev_private;
>> +    bool setted;
>>       int i;
>>       /* Update slave devices MAC addresses */
>> @@ -1529,6 +1530,7 @@ mac_address_slaves_update(struct rte_eth_dev 
>> *bonded_eth_dev)
>>       case BONDING_MODE_TLB:
>>       case BONDING_MODE_ALB:
>>       default:
>> +        setted = true;
>>           for (i = 0; i < internals->slave_count; i++) {
>>               if (internals->slaves[i].port_id ==
>>                       internals->current_primary_port) {
>> @@ -1537,7 +1539,7 @@ mac_address_slaves_update(struct rte_eth_dev 
>> *bonded_eth_dev)
>>                           bonded_eth_dev->data->mac_addrs)) {
>>                       RTE_BOND_LOG(ERR, "Failed to update port Id %d 
>> MAC address",
>>                               internals->current_primary_port);
>> -                    return -1;
>> +                    setted = false;
>>                   }
>>               } else {
>>                   if (rte_eth_dev_default_mac_addr_set(
>> @@ -1545,10 +1547,11 @@ mac_address_slaves_update(struct rte_eth_dev 
>> *bonded_eth_dev)
>>                           &internals->slaves[i].persisted_mac_addr)) {
>>                       RTE_BOND_LOG(ERR, "Failed to update port Id %d 
>> MAC address",
>>                               internals->slaves[i].port_id);
>> -                    return -1;
>>                   }
>>               }
>>           }
>> +        if (!setted)
>> +            return -1;
>>       }
>>       return 0;
>>
Ferruh Yigit May 15, 2020, 10:46 a.m. UTC | #3
On 5/15/2020 4:11 AM, Wei Hu (Xavier) wrote:
> Hi, Ferruh Yigit & Chas Williams
>    Could you please give any suggestion?

Hi Xavier,

Unfortunately we are missing reviews in bonding patches. There are a few more
waiting other than this one.

If you are already working on it and have enough information about the code,
what do you think adding you as the additional maintainer?

Thanks,
ferruh

>    Thanks.
> 
>    Best Regards
> Xavier
> 
> On 2020/4/17 16:58, Wei Hu (Xavier) wrote:
>> Hi, Chas Williams
>>     Thanks for your comments on Patch V1. now we have sent Patch V2.
>>     Could you please give some suggestion on them?
>>     Thanks.
>>
>>     Best Regards
>> Xavier
>>
>> On 2020/4/17 16:19, Wei Hu (Xavier) wrote:
>>> From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com>
>>>
>>> Currently, based on a active-backup bond device, in the following 2 
>>> cases:
>>> 1) The primary port resets. The link status of the primary port changes
>>>     from up to down.
>>> 2) When switching the active port, one slave port resets at the same 
>>> time.
>>> one slave port changes to the primary port, but the new primary port's 
>>> MAC
>>> address probably cannot change to the bond device's MAC address. And we
>>> can't continue receive packets whose destination MAC addresses are the 
>>> same
>>> as the bond devices's MAC address.
>>>
>>> The current bonding PMD driver call mac_address_slaves_update function to
>>> modify the MAC address of all slaves devices. In 
>>> mac_address_slaves_update
>>> function, the rte_eth_dev_default_mac_addr_set API function is called to
>>> set the MAC address of the slave devices in turn in the for loop 
>>> statement.
>>>
>>> When one port reset, calling rte_eth_dev_default_mac_addr_set API fails
>>> because the firmware will not respond to the commands from the driver,
>>> and exit the loop, so other slave devices cannot continue to update the
>>> MAC address.
>>>
>>> This patch fixes the issue by avoid exiting the loop when calling
>>> rte_eth_dev_default_mac_addr_set fails.
>>>
>>> Fixes: 2efb58cbab6e ("bond: new link bonding library")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Hongbo Zheng <zhenghongbo3@huawei.com>
>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>> Signed-off-by: Chunsong Feng <fengchunsong@huawei.com>
>>> Signed-off-by: Xuan Li <lixuan47@hisilicon.com>
>>> ---
>>> v1 -> v2:
>>>     Ignore the failure when updating salves's MAC address in the
>>>     mac_address_slaves_update function, because it doesn't affect
>>>     the bond's functional characteristics. The related link about
>>>     the discussion:
>>>     http://patches.dpdk.org/patch/66033/
>>> ---
>>>   drivers/net/bonding/rte_eth_bond_pmd.c | 7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>> index ddae3518c..01c0f6eb1 100644
>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>> @@ -1502,6 +1502,7 @@ int
>>>   mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
>>>   {
>>>       struct bond_dev_private *internals = 
>>> bonded_eth_dev->data->dev_private;
>>> +    bool setted;
>>>       int i;
>>>       /* Update slave devices MAC addresses */
>>> @@ -1529,6 +1530,7 @@ mac_address_slaves_update(struct rte_eth_dev 
>>> *bonded_eth_dev)
>>>       case BONDING_MODE_TLB:
>>>       case BONDING_MODE_ALB:
>>>       default:
>>> +        setted = true;
>>>           for (i = 0; i < internals->slave_count; i++) {
>>>               if (internals->slaves[i].port_id ==
>>>                       internals->current_primary_port) {
>>> @@ -1537,7 +1539,7 @@ mac_address_slaves_update(struct rte_eth_dev 
>>> *bonded_eth_dev)
>>>                           bonded_eth_dev->data->mac_addrs)) {
>>>                       RTE_BOND_LOG(ERR, "Failed to update port Id %d 
>>> MAC address",
>>>                               internals->current_primary_port);
>>> -                    return -1;
>>> +                    setted = false;
>>>                   }
>>>               } else {
>>>                   if (rte_eth_dev_default_mac_addr_set(
>>> @@ -1545,10 +1547,11 @@ mac_address_slaves_update(struct rte_eth_dev 
>>> *bonded_eth_dev)
>>>                           &internals->slaves[i].persisted_mac_addr)) {
>>>                       RTE_BOND_LOG(ERR, "Failed to update port Id %d 
>>> MAC address",
>>>                               internals->slaves[i].port_id);
>>> -                    return -1;
>>>                   }
>>>               }
>>>           }
>>> +        if (!setted)
>>> +            return -1;
>>>       }
>>>       return 0;
>>>
Wei Hu (Xavier) May 16, 2020, 9:31 a.m. UTC | #4
On 2020/5/16 16:59, Wei Hu (Xavier) wrote:
>
>
>
> -------- Forwarded Message --------
> Subject: Re: [dpdk-dev] [PATCH v2 2/2] net/bonding: fix MAC address 
> when one port resets
> Date: Fri, 15 May 2020 11:46:30 +0100
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> To: Wei Hu (Xavier) <huwei013@chinasoftinc.com>, Chas Williams 
> <3chas3@gmail.com>
> CC: dev@dpdk.org, Thomas Monjalon <thomas@monjalon.net>, David 
> Marchand <david.marchand@redhat.com>
>
> On 5/15/2020 4:11 AM, Wei Hu (Xavier) wrote:
>> Hi, Ferruh Yigit & Chas Williams
>>    Could you please give any suggestion?
>
> Hi Xavier,
>
> Unfortunately we are missing reviews in bonding patches. There are a 
> few more
> waiting other than this one.
>
> If you are already working on it and have enough information about the 
> code,
> what do you think adding you as the additional maintainer?
>
Hi,  Ferruh Yigit
I'm glad to do something like this.

Thanks,
Xavier
> Thanks,
> ferruh
>
>>    Thanks.
>>
>>    Best Regards
>> Xavier
>>
>> On 2020/4/17 16:58, Wei Hu (Xavier) wrote:
>>> Hi, Chas Williams
>>>     Thanks for your comments on Patch V1. now we have sent Patch V2.
>>>     Could you please give some suggestion on them?
>>>     Thanks.
>>>
>>>     Best Regards
>>> Xavier
>>>
>>> On 2020/4/17 16:19, Wei Hu (Xavier) wrote:
>>>> From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com>
>>>>
>>>> Currently, based on a active-backup bond device, in the following 2 
>>>> cases:
>>>> 1) The primary port resets. The link status of the primary port 
>>>> changes
>>>>     from up to down.
>>>> 2) When switching the active port, one slave port resets at the 
>>>> same time.
>>>> one slave port changes to the primary port, but the new primary 
>>>> port's MAC
>>>> address probably cannot change to the bond device's MAC address. 
>>>> And we
>>>> can't continue receive packets whose destination MAC addresses are 
>>>> the same
>>>> as the bond devices's MAC address.
>>>>
>>>> The current bonding PMD driver call mac_address_slaves_update 
>>>> function to
>>>> modify the MAC address of all slaves devices. In 
>>>> mac_address_slaves_update
>>>> function, the rte_eth_dev_default_mac_addr_set API function is 
>>>> called to
>>>> set the MAC address of the slave devices in turn in the for loop 
>>>> statement.
>>>>
>>>> When one port reset, calling rte_eth_dev_default_mac_addr_set API 
>>>> fails
>>>> because the firmware will not respond to the commands from the driver,
>>>> and exit the loop, so other slave devices cannot continue to update 
>>>> the
>>>> MAC address.
>>>>
>>>> This patch fixes the issue by avoid exiting the loop when calling
>>>> rte_eth_dev_default_mac_addr_set fails.
>>>>
>>>> Fixes: 2efb58cbab6e ("bond: new link bonding library")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Hongbo Zheng <zhenghongbo3@huawei.com>
>>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>> Signed-off-by: Chunsong Feng <fengchunsong@huawei.com>
>>>> Signed-off-by: Xuan Li <lixuan47@hisilicon.com>
>>>> ---
>>>> v1 -> v2:
>>>>     Ignore the failure when updating salves's MAC address in the
>>>>     mac_address_slaves_update function, because it doesn't affect
>>>>     the bond's functional characteristics. The related link about
>>>>     the discussion:
>>>>     http://patches.dpdk.org/patch/66033/
>>>> ---
>>>>   drivers/net/bonding/rte_eth_bond_pmd.c | 7 +++++--
>>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
>>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> index ddae3518c..01c0f6eb1 100644
>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> @@ -1502,6 +1502,7 @@ int
>>>>   mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
>>>>   {
>>>>       struct bond_dev_private *internals = 
>>>> bonded_eth_dev->data->dev_private;
>>>> +    bool setted;
>>>>       int i;
>>>>       /* Update slave devices MAC addresses */
>>>> @@ -1529,6 +1530,7 @@ mac_address_slaves_update(struct rte_eth_dev 
>>>> *bonded_eth_dev)
>>>>       case BONDING_MODE_TLB:
>>>>       case BONDING_MODE_ALB:
>>>>       default:
>>>> +        setted = true;
>>>>           for (i = 0; i < internals->slave_count; i++) {
>>>>               if (internals->slaves[i].port_id ==
>>>>                       internals->current_primary_port) {
>>>> @@ -1537,7 +1539,7 @@ mac_address_slaves_update(struct rte_eth_dev 
>>>> *bonded_eth_dev)
>>>> bonded_eth_dev->data->mac_addrs)) {
>>>>                       RTE_BOND_LOG(ERR, "Failed to update port Id 
>>>> %d MAC address",
>>>> internals->current_primary_port);
>>>> -                    return -1;
>>>> +                    setted = false;
>>>>                   }
>>>>               } else {
>>>>                   if (rte_eth_dev_default_mac_addr_set(
>>>> @@ -1545,10 +1547,11 @@ mac_address_slaves_update(struct 
>>>> rte_eth_dev *bonded_eth_dev)
>>>> &internals->slaves[i].persisted_mac_addr)) {
>>>>                       RTE_BOND_LOG(ERR, "Failed to update port Id 
>>>> %d MAC address",
>>>> internals->slaves[i].port_id);
>>>> -                    return -1;
>>>>                   }
>>>>               }
>>>>           }
>>>> +        if (!setted)
>>>> +            return -1;
>>>>       }
>>>>       return 0;
>>>>
>
> .
>
Wei Hu (Xavier) May 16, 2020, 9:51 a.m. UTC | #5
On 2020/5/16 16:59, Wei Hu (Xavier) wrote:
>
>
>
> -------- Forwarded Message --------
> Subject: Re: [dpdk-dev] [PATCH v2 2/2] net/bonding: fix MAC address 
> when one port resets
> Date: Fri, 15 May 2020 11:46:30 +0100
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> To: Wei Hu (Xavier) <huwei013@chinasoftinc.com>, Chas Williams 
> <3chas3@gmail.com>
> CC: dev@dpdk.org, Thomas Monjalon <thomas@monjalon.net>, David 
> Marchand <david.marchand@redhat.com>
>
> On 5/15/2020 4:11 AM, Wei Hu (Xavier) wrote:
>> Hi, Ferruh Yigit & Chas Williams
>>    Could you please give any suggestion?
>
> Hi Xavier,
>
> Unfortunately we are missing reviews in bonding patches. There are a 
> few more
> waiting other than this one.
>
> If you are already working on it and have enough information about the 
> code,
> what do you think adding you as the additional maintainer?
>
Hi,  Ferruh Yigit

I'm glad to do something like this.

Thanks,
Xavier
> Thanks,
> ferruh
>
>>    Thanks.
>>
>>    Best Regards
>> Xavier
>>
>> On 2020/4/17 16:58, Wei Hu (Xavier) wrote:
>>> Hi, Chas Williams
>>>     Thanks for your comments on Patch V1. now we have sent Patch V2.
>>>     Could you please give some suggestion on them?
>>>     Thanks.
>>>
>>>     Best Regards
>>> Xavier
>>>
>>> On 2020/4/17 16:19, Wei Hu (Xavier) wrote:
>>>> From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com>
>>>>
>>>> Currently, based on a active-backup bond device, in the following 2 
>>>> cases:
>>>> 1) The primary port resets. The link status of the primary port 
>>>> changes
>>>>     from up to down.
>>>> 2) When switching the active port, one slave port resets at the 
>>>> same time.
>>>> one slave port changes to the primary port, but the new primary 
>>>> port's MAC
>>>> address probably cannot change to the bond device's MAC address. 
>>>> And we
>>>> can't continue receive packets whose destination MAC addresses are 
>>>> the same
>>>> as the bond devices's MAC address.
>>>>
>>>> The current bonding PMD driver call mac_address_slaves_update 
>>>> function to
>>>> modify the MAC address of all slaves devices. In 
>>>> mac_address_slaves_update
>>>> function, the rte_eth_dev_default_mac_addr_set API function is 
>>>> called to
>>>> set the MAC address of the slave devices in turn in the for loop 
>>>> statement.
>>>>
>>>> When one port reset, calling rte_eth_dev_default_mac_addr_set API 
>>>> fails
>>>> because the firmware will not respond to the commands from the driver,
>>>> and exit the loop, so other slave devices cannot continue to update 
>>>> the
>>>> MAC address.
>>>>
>>>> This patch fixes the issue by avoid exiting the loop when calling
>>>> rte_eth_dev_default_mac_addr_set fails.
>>>>
>>>> Fixes: 2efb58cbab6e ("bond: new link bonding library")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Hongbo Zheng <zhenghongbo3@huawei.com>
>>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>> Signed-off-by: Chunsong Feng <fengchunsong@huawei.com>
>>>> Signed-off-by: Xuan Li <lixuan47@hisilicon.com>
>>>> ---
>>>> v1 -> v2:
>>>>     Ignore the failure when updating salves's MAC address in the
>>>>     mac_address_slaves_update function, because it doesn't affect
>>>>     the bond's functional characteristics. The related link about
>>>>     the discussion:
>>>>     http://patches.dpdk.org/patch/66033/
>>>> ---
>>>>   drivers/net/bonding/rte_eth_bond_pmd.c | 7 +++++--
>>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
>>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> index ddae3518c..01c0f6eb1 100644
>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> @@ -1502,6 +1502,7 @@ int
>>>>   mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
>>>>   {
>>>>       struct bond_dev_private *internals = 
>>>> bonded_eth_dev->data->dev_private;
>>>> +    bool setted;
>>>>       int i;
>>>>       /* Update slave devices MAC addresses */
>>>> @@ -1529,6 +1530,7 @@ mac_address_slaves_update(struct rte_eth_dev 
>>>> *bonded_eth_dev)
>>>>       case BONDING_MODE_TLB:
>>>>       case BONDING_MODE_ALB:
>>>>       default:
>>>> +        setted = true;
>>>>           for (i = 0; i < internals->slave_count; i++) {
>>>>               if (internals->slaves[i].port_id ==
>>>>                       internals->current_primary_port) {
>>>> @@ -1537,7 +1539,7 @@ mac_address_slaves_update(struct rte_eth_dev 
>>>> *bonded_eth_dev)
>>>> bonded_eth_dev->data->mac_addrs)) {
>>>>                       RTE_BOND_LOG(ERR, "Failed to update port Id 
>>>> %d MAC address",
>>>> internals->current_primary_port);
>>>> -                    return -1;
>>>> +                    setted = false;
>>>>                   }
>>>>               } else {
>>>>                   if (rte_eth_dev_default_mac_addr_set(
>>>> @@ -1545,10 +1547,11 @@ mac_address_slaves_update(struct 
>>>> rte_eth_dev *bonded_eth_dev)
>>>> &internals->slaves[i].persisted_mac_addr)) {
>>>>                       RTE_BOND_LOG(ERR, "Failed to update port Id 
>>>> %d MAC address",
>>>> internals->slaves[i].port_id);
>>>> -                    return -1;
>>>>                   }
>>>>               }
>>>>           }
>>>> +        if (!setted)
>>>> +            return -1;
>>>>       }
>>>>       return 0;
>>>>
>
> .
>
Ferruh Yigit May 18, 2020, 9:55 a.m. UTC | #6
On 5/16/2020 10:51 AM, Wei Hu (Xavier) wrote:
> 
> 
> On 2020/5/16 16:59, Wei Hu (Xavier) wrote:
>>
>>
>>
>> -------- Forwarded Message --------
>> Subject: Re: [dpdk-dev] [PATCH v2 2/2] net/bonding: fix MAC address 
>> when one port resets
>> Date: Fri, 15 May 2020 11:46:30 +0100
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> To: Wei Hu (Xavier) <huwei013@chinasoftinc.com>, Chas Williams 
>> <3chas3@gmail.com>
>> CC: dev@dpdk.org, Thomas Monjalon <thomas@monjalon.net>, David 
>> Marchand <david.marchand@redhat.com>
>>
>> On 5/15/2020 4:11 AM, Wei Hu (Xavier) wrote:
>>> Hi, Ferruh Yigit & Chas Williams
>>>    Could you please give any suggestion?
>>
>> Hi Xavier,
>>
>> Unfortunately we are missing reviews in bonding patches. There are a 
>> few more
>> waiting other than this one.
>>
>> If you are already working on it and have enough information about the 
>> code,
>> what do you think adding you as the additional maintainer?
>>
> Hi,  Ferruh Yigit
> 
> I'm glad to do something like this.
> 

Great news, thanks.
Can you please send a patch to add your name as bonding maintainer to the
MAINTAINERS file [1]?

For this release it may be late, but hopefully we can close the outstanding
bonding patches in next release.

Thanks,
ferruh

[1]
http://lxr.dpdk.org/dpdk/v20.02/source/MAINTAINERS#L528
Wei Hu (Xavier) May 18, 2020, 11:12 a.m. UTC | #7
On 2020/5/18 17:55, Ferruh Yigit wrote:
> On 5/16/2020 10:51 AM, Wei Hu (Xavier) wrote:
>>
>> On 2020/5/16 16:59, Wei Hu (Xavier) wrote:
>>>
>>>
>>> -------- Forwarded Message --------
>>> Subject: Re: [dpdk-dev] [PATCH v2 2/2] net/bonding: fix MAC address
>>> when one port resets
>>> Date: Fri, 15 May 2020 11:46:30 +0100
>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>> To: Wei Hu (Xavier) <huwei013@chinasoftinc.com>, Chas Williams
>>> <3chas3@gmail.com>
>>> CC: dev@dpdk.org, Thomas Monjalon <thomas@monjalon.net>, David
>>> Marchand <david.marchand@redhat.com>
>>>
>>> On 5/15/2020 4:11 AM, Wei Hu (Xavier) wrote:
>>>> Hi, Ferruh Yigit & Chas Williams
>>>>     Could you please give any suggestion?
>>> Hi Xavier,
>>>
>>> Unfortunately we are missing reviews in bonding patches. There are a
>>> few more
>>> waiting other than this one.
>>>
>>> If you are already working on it and have enough information about the
>>> code,
>>> what do you think adding you as the additional maintainer?
>>>
>> Hi,  Ferruh Yigit
>>
>> I'm glad to do something like this.
>>
> Great news, thanks.
> Can you please send a patch to add your name as bonding maintainer to the
> MAINTAINERS file [1]?
>
> For this release it may be late, but hopefully we can close the outstanding
> bonding patches in next release.
Ok, I will send the patch at once.

Thanks,
Xavier
> Thanks,
> ferruh
>
> [1]
> http://lxr.dpdk.org/dpdk/v20.02/source/MAINTAINERS#L528
>

Patch
diff mbox series

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index ddae3518c..01c0f6eb1 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1502,6 +1502,7 @@  int
 mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
 {
 	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	bool setted;
 	int i;
 
 	/* Update slave devices MAC addresses */
@@ -1529,6 +1530,7 @@  mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
 	case BONDING_MODE_TLB:
 	case BONDING_MODE_ALB:
 	default:
+		setted = true;
 		for (i = 0; i < internals->slave_count; i++) {
 			if (internals->slaves[i].port_id ==
 					internals->current_primary_port) {
@@ -1537,7 +1539,7 @@  mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
 						bonded_eth_dev->data->mac_addrs)) {
 					RTE_BOND_LOG(ERR, "Failed to update port Id %d MAC address",
 							internals->current_primary_port);
-					return -1;
+					setted = false;
 				}
 			} else {
 				if (rte_eth_dev_default_mac_addr_set(
@@ -1545,10 +1547,11 @@  mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
 						&internals->slaves[i].persisted_mac_addr)) {
 					RTE_BOND_LOG(ERR, "Failed to update port Id %d MAC address",
 							internals->slaves[i].port_id);
-					return -1;
 				}
 			}
 		}
+		if (!setted)
+			return -1;
 	}
 
 	return 0;