[PATCH] net/bonding: add link speeds configuration

[PATCH] net/bonding: add link speeds configuration

[Huisong Li]

This patch adds link speeds configuration.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 73e6972035..67d353ae09 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1717,6 +1717,8 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 
 	slave_eth_dev->data->dev_conf.rxmode.mtu =
 			bonded_eth_dev->data->dev_conf.rxmode.mtu;
+	slave_eth_dev->data->dev_conf.link_speeds =
+			bonded_eth_dev->data->dev_conf.link_speeds;
 
 	slave_eth_dev->data->dev_conf.txmode.offloads |=
 		bonded_eth_dev->data->dev_conf.txmode.offloads;
-- 
2.22.0

[PATCH V2] net/bonding: add link speeds configuration

[Huisong Li]

This patch adds link speeds configuration.

---
 -v2: resend due to CI compiling failure.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 3191158ca7..0c1dbc3ff6 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1717,6 +1717,8 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 
 	slave_eth_dev->data->dev_conf.rxmode.mtu =
 			bonded_eth_dev->data->dev_conf.rxmode.mtu;
+	slave_eth_dev->data->dev_conf.link_speeds =
+			bonded_eth_dev->data->dev_conf.link_speeds;
 
 	slave_eth_dev->data->dev_conf.txmode.offloads |=
 		bonded_eth_dev->data->dev_conf.txmode.offloads;
-- 
2.22.0

Re: [PATCH V2] net/bonding: add link speeds configuration

[Chas Williams]

On 9/15/22 09:14, Huisong Li wrote:
> This patch adds link speeds configuration.
> 
> ---
>   -v2: resend due to CI compiling failure.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>   drivers/net/bonding/rte_eth_bond_pmd.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 3191158ca7..0c1dbc3ff6 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -1717,6 +1717,8 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>   
>   	slave_eth_dev->data->dev_conf.rxmode.mtu =
>   			bonded_eth_dev->data->dev_conf.rxmode.mtu;
> +	slave_eth_dev->data->dev_conf.link_speeds =
> +			bonded_eth_dev->data->dev_conf.link_speeds;
>   
>   	slave_eth_dev->data->dev_conf.txmode.offloads |=
>   		bonded_eth_dev->data->dev_conf.txmode.offloads;


Where did bonded_eth_dev->data->dev_conf.link_speeds come from? This is 
rather messy since you might need to worry about intersection of 
speed_capa from the set of members in the bond group.

Re: [PATCH V2] net/bonding: add link speeds configuration

[lihuisong (C)]

在 2022/9/15 21:43, Chas Williams 写道:
> On 9/15/22 09:14, Huisong Li wrote:
>> This patch adds link speeds configuration.
>>
>> ---
>>   -v2: resend due to CI compiling failure.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   drivers/net/bonding/rte_eth_bond_pmd.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>> index 3191158ca7..0c1dbc3ff6 100644
>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>> @@ -1717,6 +1717,8 @@ slave_configure(struct rte_eth_dev 
>> *bonded_eth_dev,
>>         slave_eth_dev->data->dev_conf.rxmode.mtu =
>>               bonded_eth_dev->data->dev_conf.rxmode.mtu;
>> +    slave_eth_dev->data->dev_conf.link_speeds =
>> +            bonded_eth_dev->data->dev_conf.link_speeds;
>>         slave_eth_dev->data->dev_conf.txmode.offloads |=
>>           bonded_eth_dev->data->dev_conf.txmode.offloads;
>
>
> Where did bonded_eth_dev->data->dev_conf.link_speeds come from? This 
> is rather messy since you might need to worry about intersection of 
> speed_capa from the set of members in the bond group.
Hi chas,

Thanks for your review.

bonded_eth_dev->data->dev_conf.link_speeds come from user. I don't think 
we need to complicate it.
Not all PMDs report 'speed_capa'. But the slave device knows if the 
specified speed is supported.
So I think that it is better to leave to the slave device to determine 
whether the speed is configured successfully.
> .

Re: [PATCH V2] net/bonding: add link speeds configuration

[Chas Williams]

On 9/15/22 22:09, lihuisong (C) wrote:
> 
> 在 2022/9/15 21:43, Chas Williams 写道:
>> On 9/15/22 09:14, Huisong Li wrote:
>>> This patch adds link speeds configuration.
>>>
>>> ---
>>>   -v2: resend due to CI compiling failure.
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> ---
>>>   drivers/net/bonding/rte_eth_bond_pmd.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>> index 3191158ca7..0c1dbc3ff6 100644
>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>> @@ -1717,6 +1717,8 @@ slave_configure(struct rte_eth_dev 
>>> *bonded_eth_dev,
>>>         slave_eth_dev->data->dev_conf.rxmode.mtu =
>>>               bonded_eth_dev->data->dev_conf.rxmode.mtu;
>>> +    slave_eth_dev->data->dev_conf.link_speeds =
>>> +            bonded_eth_dev->data->dev_conf.link_speeds;
>>>         slave_eth_dev->data->dev_conf.txmode.offloads |=
>>>           bonded_eth_dev->data->dev_conf.txmode.offloads;
>>
>>
>> Where did bonded_eth_dev->data->dev_conf.link_speeds come from? This 
>> is rather messy since you might need to worry about intersection of 
>> speed_capa from the set of members in the bond group.
> Hi chas,
> 
> Thanks for your review.
> 
> bonded_eth_dev->data->dev_conf.link_speeds come from user. I don't think 
> we need to complicate it.
> Not all PMDs report 'speed_capa'. But the slave device knows if the 
> specified speed is supported.
> So I think that it is better to leave to the slave device to determine 
> whether the speed is configured successfully.
>> .

I still think it would be a good idea to create an intersection of the
supported speeds in bonding's dev_conf.speed_capa if we are going to start
trying to set the speeds via the bonding interface. The PMDs that don't
report a valid speed_capa are technically broken.

Re: [PATCH V2] net/bonding: add link speeds configuration

[lihuisong (C)]

在 2022/9/16 23:07, Chas Williams 写道:
>
>
> On 9/15/22 22:09, lihuisong (C) wrote:
>>
>> 在 2022/9/15 21:43, Chas Williams 写道:
>>> On 9/15/22 09:14, Huisong Li wrote:
>>>> This patch adds link speeds configuration.
>>>>
>>>> ---
>>>>   -v2: resend due to CI compiling failure.
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> ---
>>>>   drivers/net/bonding/rte_eth_bond_pmd.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
>>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> index 3191158ca7..0c1dbc3ff6 100644
>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> @@ -1717,6 +1717,8 @@ slave_configure(struct rte_eth_dev 
>>>> *bonded_eth_dev,
>>>>         slave_eth_dev->data->dev_conf.rxmode.mtu =
>>>> bonded_eth_dev->data->dev_conf.rxmode.mtu;
>>>> +    slave_eth_dev->data->dev_conf.link_speeds =
>>>> + bonded_eth_dev->data->dev_conf.link_speeds;
>>>>         slave_eth_dev->data->dev_conf.txmode.offloads |=
>>>> bonded_eth_dev->data->dev_conf.txmode.offloads;
>>>
>>>
>>> Where did bonded_eth_dev->data->dev_conf.link_speeds come from? This 
>>> is rather messy since you might need to worry about intersection of 
>>> speed_capa from the set of members in the bond group.
>> Hi chas,
>>
>> Thanks for your review.
>>
>> bonded_eth_dev->data->dev_conf.link_speeds come from user. I don't 
>> think we need to complicate it.
>> Not all PMDs report 'speed_capa'. But the slave device knows if the 
>> specified speed is supported.
>> So I think that it is better to leave to the slave device to 
>> determine whether the speed is configured successfully.
>>> .
>
> I still think it would be a good idea to create an intersection of the
> supported speeds in bonding's dev_conf.speed_capa if we are going to 
> start
> trying to set the speeds via the bonding interface. The PMDs that don't
> report a valid speed_capa are technically broken.
Will fix in v3. Please review it.
> .

[PATCH V3] net/bonding: add link speeds configuration

[Huisong Li]

This patch adds link speeds configuration.

---
 -v3: add an intersection of the supported speeds to check 'link_speeds'.
 -v2: resend due to CI compiling failure.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/net/bonding/eth_bond_private.h |  3 +++
 drivers/net/bonding/rte_eth_bond_api.c |  3 +++
 drivers/net/bonding/rte_eth_bond_pmd.c | 27 ++++++++++++++++++++++++++
 3 files changed, 33 insertions(+)

diff --git a/drivers/net/bonding/eth_bond_private.h b/drivers/net/bonding/eth_bond_private.h
index 8222e3cd38..d067ea8c9a 100644
--- a/drivers/net/bonding/eth_bond_private.h
+++ b/drivers/net/bonding/eth_bond_private.h
@@ -131,6 +131,9 @@ struct bond_dev_private {
 	uint32_t link_down_delay_ms;
 	uint32_t link_up_delay_ms;
 
+	uint32_t speed_capa;
+	/**< Supported speeds bitmap (RTE_ETH_LINK_SPEED_). */
+
 	uint16_t nb_rx_queues;			/**< Total number of rx queues */
 	uint16_t nb_tx_queues;			/**< Total number of tx queues*/
 
diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index 4ac191c468..e64ec0ed20 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -513,6 +513,8 @@ __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t slave_port_id)
 		internals->primary_port = slave_port_id;
 		internals->current_primary_port = slave_port_id;
 
+		internals->speed_capa = dev_info.speed_capa;
+
 		/* Inherit queues settings from first slave */
 		internals->nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
 		internals->nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
@@ -527,6 +529,7 @@ __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t slave_port_id)
 	} else {
 		int ret;
 
+		internals->speed_capa &= dev_info.speed_capa;
 		eth_bond_slave_inherit_dev_info_rx_next(internals, &dev_info);
 		eth_bond_slave_inherit_dev_info_tx_next(internals, &dev_info);
 
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 3191158ca7..0adbf0e1b2 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1717,6 +1717,8 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 
 	slave_eth_dev->data->dev_conf.rxmode.mtu =
 			bonded_eth_dev->data->dev_conf.rxmode.mtu;
+	slave_eth_dev->data->dev_conf.link_speeds =
+			bonded_eth_dev->data->dev_conf.link_speeds;
 
 	slave_eth_dev->data->dev_conf.txmode.offloads |=
 		bonded_eth_dev->data->dev_conf.txmode.offloads;
@@ -2275,6 +2277,7 @@ bond_ethdev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 
 	dev_info->reta_size = internals->reta_size;
 	dev_info->hash_key_size = internals->rss_key_len;
+	dev_info->speed_capa = internals->speed_capa;
 
 	return 0;
 }
@@ -3591,6 +3594,7 @@ bond_ethdev_configure(struct rte_eth_dev *dev)
 	uint64_t offloads;
 	int arg_count;
 	uint16_t port_id = dev - rte_eth_devices;
+	uint32_t link_speeds;
 	uint8_t agg_mode;
 
 	static const uint8_t default_rss_key[40] = {
@@ -3659,6 +3663,29 @@ bond_ethdev_configure(struct rte_eth_dev *dev)
 		dev->data->dev_conf.txmode.offloads = offloads;
 	}
 
+	link_speeds = dev->data->dev_conf.link_speeds;
+	/*
+	 * The default value of 'link_speeds' is zero. From its definition,
+	 * this value actually means auto-negotiation. But not all PMDs support
+	 * auto-negotiation. So ignore the check for the auto-negotiation and
+	 * only consider fixed speed to reduce the impact on PMDs.
+	 */
+	if (link_speeds & RTE_ETH_LINK_SPEED_FIXED) {
+		if ((link_speeds &
+		    (internals->speed_capa & ~RTE_ETH_LINK_SPEED_FIXED)) == 0) {
+			RTE_BOND_LOG(ERR, "the fixed speed is not supported by all slave devices.");
+			return -EINVAL;
+		}
+		/*
+		 * Two '1' in binary of 'link_speeds': bit0 and a unique
+		 * speed bit.
+		 */
+		if (__builtin_popcountl(link_speeds) != 2) {
+			RTE_BOND_LOG(ERR, "please set a unique speed.");
+			return -EINVAL;
+		}
+	}
+
 	/* set the max_rx_pktlen */
 	internals->max_rx_pktlen = internals->candidate_max_rx_pktlen;
 
-- 
2.22.0

Re: [PATCH V3] net/bonding: add link speeds configuration

[Chas Williams]

Thanks for making the changes!

Signed-off-by: 3chas3@gmail.com

On 9/21/22 21:33, Huisong Li wrote:
> This patch adds link speeds configuration.
> 
> ---
>   -v3: add an intersection of the supported speeds to check 'link_speeds'.
>   -v2: resend due to CI compiling failure.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>   drivers/net/bonding/eth_bond_private.h |  3 +++
>   drivers/net/bonding/rte_eth_bond_api.c |  3 +++
>   drivers/net/bonding/rte_eth_bond_pmd.c | 27 ++++++++++++++++++++++++++
>   3 files changed, 33 insertions(+)
> 
> diff --git a/drivers/net/bonding/eth_bond_private.h b/drivers/net/bonding/eth_bond_private.h
> index 8222e3cd38..d067ea8c9a 100644
> --- a/drivers/net/bonding/eth_bond_private.h
> +++ b/drivers/net/bonding/eth_bond_private.h
> @@ -131,6 +131,9 @@ struct bond_dev_private {
>   	uint32_t link_down_delay_ms;
>   	uint32_t link_up_delay_ms;
>   
> +	uint32_t speed_capa;
> +	/**< Supported speeds bitmap (RTE_ETH_LINK_SPEED_). */
> +
>   	uint16_t nb_rx_queues;			/**< Total number of rx queues */
>   	uint16_t nb_tx_queues;			/**< Total number of tx queues*/
>   
> diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
> index 4ac191c468..e64ec0ed20 100644
> --- a/drivers/net/bonding/rte_eth_bond_api.c
> +++ b/drivers/net/bonding/rte_eth_bond_api.c
> @@ -513,6 +513,8 @@ __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t slave_port_id)
>   		internals->primary_port = slave_port_id;
>   		internals->current_primary_port = slave_port_id;
>   
> +		internals->speed_capa = dev_info.speed_capa;
> +
>   		/* Inherit queues settings from first slave */
>   		internals->nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
>   		internals->nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
> @@ -527,6 +529,7 @@ __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t slave_port_id)
>   	} else {
>   		int ret;
>   
> +		internals->speed_capa &= dev_info.speed_capa;
>   		eth_bond_slave_inherit_dev_info_rx_next(internals, &dev_info);
>   		eth_bond_slave_inherit_dev_info_tx_next(internals, &dev_info);
>   
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 3191158ca7..0adbf0e1b2 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -1717,6 +1717,8 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>   
>   	slave_eth_dev->data->dev_conf.rxmode.mtu =
>   			bonded_eth_dev->data->dev_conf.rxmode.mtu;
> +	slave_eth_dev->data->dev_conf.link_speeds =
> +			bonded_eth_dev->data->dev_conf.link_speeds;
>   
>   	slave_eth_dev->data->dev_conf.txmode.offloads |=
>   		bonded_eth_dev->data->dev_conf.txmode.offloads;
> @@ -2275,6 +2277,7 @@ bond_ethdev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>   
>   	dev_info->reta_size = internals->reta_size;
>   	dev_info->hash_key_size = internals->rss_key_len;
> +	dev_info->speed_capa = internals->speed_capa;
>   
>   	return 0;
>   }
> @@ -3591,6 +3594,7 @@ bond_ethdev_configure(struct rte_eth_dev *dev)
>   	uint64_t offloads;
>   	int arg_count;
>   	uint16_t port_id = dev - rte_eth_devices;
> +	uint32_t link_speeds;
>   	uint8_t agg_mode;
>   
>   	static const uint8_t default_rss_key[40] = {
> @@ -3659,6 +3663,29 @@ bond_ethdev_configure(struct rte_eth_dev *dev)
>   		dev->data->dev_conf.txmode.offloads = offloads;
>   	}
>   
> +	link_speeds = dev->data->dev_conf.link_speeds;
> +	/*
> +	 * The default value of 'link_speeds' is zero. From its definition,
> +	 * this value actually means auto-negotiation. But not all PMDs support
> +	 * auto-negotiation. So ignore the check for the auto-negotiation and
> +	 * only consider fixed speed to reduce the impact on PMDs.
> +	 */
> +	if (link_speeds & RTE_ETH_LINK_SPEED_FIXED) {
> +		if ((link_speeds &
> +		    (internals->speed_capa & ~RTE_ETH_LINK_SPEED_FIXED)) == 0) {
> +			RTE_BOND_LOG(ERR, "the fixed speed is not supported by all slave devices.");
> +			return -EINVAL;
> +		}
> +		/*
> +		 * Two '1' in binary of 'link_speeds': bit0 and a unique
> +		 * speed bit.
> +		 */
> +		if (__builtin_popcountl(link_speeds) != 2) {
> +			RTE_BOND_LOG(ERR, "please set a unique speed.");
> +			return -EINVAL;
> +		}
> +	}
> +
>   	/* set the max_rx_pktlen */
>   	internals->max_rx_pktlen = internals->candidate_max_rx_pktlen;
>   

Re: [PATCH V3] net/bonding: add link speeds configuration

[Andrew Rybchenko]

On 9/25/22 13:35, Chas Williams wrote:
> Thanks for making the changes!
> 
> Signed-off-by: 3chas3@gmail.com

Thanks above sentence into account I treat it as:
Acked-by: Chas Williams <3chas3@gmail.com>

> 
> On 9/21/22 21:33, Huisong Li wrote:
>> This patch adds link speeds configuration.
>>
>> ---
>>   -v3: add an intersection of the supported speeds to check 
>> 'link_speeds'.
>>   -v2: resend due to CI compiling failure.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>

and this one as a Signed-off-by in a right place.

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