diff mbox series

[V2] net/bonding: add link speeds configuration

Message ID 20220915131424.7337-1-lihuisong@huawei.com (mailing list archive)
State Superseded, archived
Delegated to: Andrew Rybchenko
Headers
Series [V2] net/bonding: add link speeds configuration |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation fail Compilation issues
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed

Commit Message

lihuisong (C) Sept. 15, 2022, 1:14 p.m. UTC 
  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(+)

Comments

Chas Williams Sept. 15, 2022, 1:43 p.m. UTC | #1 
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.
lihuisong (C) Sept. 16, 2022, 2:09 a.m. UTC | #2 
在 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.
> .
Chas Williams Sept. 16, 2022, 3:07 p.m. UTC | #3 
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.
lihuisong (C) Sept. 22, 2022, 1:15 a.m. UTC | #4 
在 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.
> .
diff mbox series

Patch

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;