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
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
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.
在 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. > .
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.
在 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. > .
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
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;
>
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.