[2/2] net/bonding: fix slaves initilizing on mtu setting
Checks
Commit Message
When initializing a bonding device with all slaves in vdev string,
which means bonding device initilizes slaves, instead of initializing
slaves before creating the bonding device, it will fail to call
set_mtu api for the bonding device.
Fixes:b26bee1 ("ethdev: forbid MTU set before device configure")
Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>
---
app/test/test_link_bonding.c | 4 +++
app/test/test_link_bonding_rssconf.c | 4 +++
drivers/net/bonding/eth_bond_private.h | 4 +++
drivers/net/bonding/rte_eth_bond_api.c | 6 ++++
drivers/net/bonding/rte_eth_bond_pmd.c | 43 ++++++++++++++++++++++----
5 files changed, 55 insertions(+), 6 deletions(-)
Comments
Hi, wan,
two comments:
1. wrong spelling in headline
2. comment info should be detailed, such as,
show the testpmd commands about how to operate
in this two situations.
Thanks.
在 2022/2/14 16:13, Wan Junjie 写道:
> When initializing a bonding device with all slaves in vdev string,
> which means bonding device initilizes slaves, instead of initializing
> slaves before creating the bonding device, it will fail to call
> set_mtu api for the bonding device.
>
> Fixes:b26bee1 ("ethdev: forbid MTU set before device configure")
>
> Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>
> ---
> app/test/test_link_bonding.c | 4 +++
> app/test/test_link_bonding_rssconf.c | 4 +++
> drivers/net/bonding/eth_bond_private.h | 4 +++
> drivers/net/bonding/rte_eth_bond_api.c | 6 ++++
> drivers/net/bonding/rte_eth_bond_pmd.c | 43 ++++++++++++++++++++++----
> 5 files changed, 55 insertions(+), 6 deletions(-)
>
> diff --git a/app/test/test_link_bonding.c b/app/test/test_link_bonding.c
> index dc6fc46b9c..12c50ef393 100644
> --- a/app/test/test_link_bonding.c
> +++ b/app/test/test_link_bonding.c
> @@ -181,6 +181,10 @@ configure_ethdev(uint16_t port_id, uint8_t start, uint8_t en_isr)
> test_params->nb_tx_q, &default_pmd_conf),
> "rte_eth_dev_configure for port %d failed", port_id);
>
> + int ret = rte_eth_dev_set_mtu(port_id, 1550);
> + RTE_TEST_ASSERT(ret == 0 || ret == -ENOTSUP,
> + "rte_eth_dev_set_mtu for port %d failed", port_id);
> +
> for (q_id = 0; q_id < test_params->nb_rx_q; q_id++)
> TEST_ASSERT_SUCCESS(rte_eth_rx_queue_setup(port_id, q_id, RX_RING_SIZE,
> rte_eth_dev_socket_id(port_id), &rx_conf_default,
> diff --git a/app/test/test_link_bonding_rssconf.c b/app/test/test_link_bonding_rssconf.c
> index f9eae93973..7228965ced 100644
> --- a/app/test/test_link_bonding_rssconf.c
> +++ b/app/test/test_link_bonding_rssconf.c
> @@ -128,6 +128,10 @@ configure_ethdev(uint16_t port_id, struct rte_eth_conf *eth_conf,
> RXTX_QUEUE_COUNT, eth_conf) == 0, "Failed to configure device %u",
> port_id);
>
> + int ret = rte_eth_dev_set_mtu(port_id, 1550);
> + RTE_TEST_ASSERT(ret == 0 || ret == -ENOTSUP,
> + "rte_eth_dev_set_mtu for port %d failed", port_id);
> +
> for (rxq = 0; rxq < RXTX_QUEUE_COUNT; rxq++) {
> TEST_ASSERT(rte_eth_rx_queue_setup(port_id, rxq, RXTX_RING_SIZE,
> rte_eth_dev_socket_id(port_id), NULL,
> diff --git a/drivers/net/bonding/eth_bond_private.h b/drivers/net/bonding/eth_bond_private.h
> index 156335c425..8222e3cd38 100644
> --- a/drivers/net/bonding/eth_bond_private.h
> +++ b/drivers/net/bonding/eth_bond_private.h
> @@ -246,6 +246,10 @@ int
> slave_configure(struct rte_eth_dev *bonded_eth_dev,
> struct rte_eth_dev *slave_eth_dev);
>
> +int
> +slave_start(struct rte_eth_dev *bonded_eth_dev,
> + struct rte_eth_dev *slave_eth_dev);
> +
> void
> slave_remove(struct bond_dev_private *internals,
> struct rte_eth_dev *slave_eth_dev);
> diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
> index b78867b125..4ac191c468 100644
> --- a/drivers/net/bonding/rte_eth_bond_api.c
> +++ b/drivers/net/bonding/rte_eth_bond_api.c
> @@ -566,6 +566,12 @@ __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t slave_port_id)
> slave_port_id);
> return -1;
> }
> + if (slave_start(bonded_eth_dev, slave_eth_dev) != 0) {
> + internals->slave_count--;
> + RTE_BOND_LOG(ERR, "rte_bond_slaves_start: port=%d",
> + slave_port_id);
> + return -1;
> + }
> }
>
> /* Update all slave devices MACs */
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index aa6519f83c..c31b723071 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -1678,14 +1678,10 @@ int
> slave_configure(struct rte_eth_dev *bonded_eth_dev,
> struct rte_eth_dev *slave_eth_dev)
> {
> - struct bond_rx_queue *bd_rx_q;
> - struct bond_tx_queue *bd_tx_q;
> uint16_t nb_rx_queues;
> uint16_t nb_tx_queues;
>
> int errval;
> - uint16_t q_id;
> - struct rte_flow_error flow_error;
>
> struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
>
> @@ -1758,6 +1754,19 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> slave_eth_dev->data->port_id, errval);
> return errval;
> }
> + return 0;
> +}
> +
> +int
> +slave_start(struct rte_eth_dev *bonded_eth_dev,
> + struct rte_eth_dev *slave_eth_dev)
> +{
> + int errval = 0;
> + struct bond_rx_queue *bd_rx_q;
> + struct bond_tx_queue *bd_tx_q;
> + uint16_t q_id;
> + struct rte_flow_error flow_error;
> + struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
>
> /* Setup Rx Queues */
> for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> @@ -1806,10 +1815,13 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> return errval;
> }
>
> - if (internals->mode4.dedicated_queues.flow[slave_eth_dev->data->port_id] != NULL)
> - rte_flow_destroy(slave_eth_dev->data->port_id,
> + if (internals->mode4.dedicated_queues.flow[slave_eth_dev->data->port_id] != NULL) {
> + errval = rte_flow_destroy(slave_eth_dev->data->port_id,
> internals->mode4.dedicated_queues.flow[slave_eth_dev->data->port_id],
> &flow_error);
> + RTE_BOND_LOG(ERR, "bond_ethdev_8023ad_flow_destroy: port=%d, err (%d)",
> + slave_eth_dev->data->port_id, errval);
> + }
>
> errval = bond_ethdev_8023ad_flow_set(bonded_eth_dev,
> slave_eth_dev->data->port_id);
> @@ -2001,6 +2013,13 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
> internals->slaves[i].port_id);
> goto out_err;
> }
> + if (slave_start(eth_dev, slave_ethdev) != 0) {
> + RTE_BOND_LOG(ERR,
> + "bonded port (%d) failed to start slave device (%d)",
> + eth_dev->data->port_id,
> + internals->slaves[i].port_id);
> + goto out_err;
> + }
> /* We will need to poll for link status if any slave doesn't
> * support interrupts
> */
> @@ -3849,6 +3868,18 @@ bond_ethdev_configure(struct rte_eth_dev *dev)
> return -1;
> }
>
> + /* configure slaves so we can pass mtu setting */
> + for (i = 0; i < internals->slave_count; i++) {
> + struct rte_eth_dev *slave_ethdev =
> + &(rte_eth_devices[internals->slaves[i].port_id]);
> + if (slave_configure(dev, slave_ethdev) != 0) {
> + RTE_BOND_LOG(ERR,
> + "bonded port (%d) failed to configure slave device (%d)",
> + dev->data->port_id,
> + internals->slaves[i].port_id);
> + return -1;
> + }
> + }
> return 0;
> }
>
>
Hi Min Hu,
On Tue, Feb 15, 2022 at 5:26 PM Min Hu (Connor) <humin29@huawei.com> wrote:
>
> Hi, wan,
> two comments:
> 1. wrong spelling in headline
Will fix it in v2.
> 2. comment info should be detailed, such as,
> show the testpmd commands about how to operate
> in this two situations.
>
Testpmd did not show the issue here.
to reproduce, we need call the api by the sequence:
rte_eth_dev_configure
rte_eth_dev_set_mtu
queue setup and start ...
for dpdk application, pass the device
--vdev="net_bonding0,mode=2,slave=0000:af:00.0"
Then application will complain for the slave device, that
"Port 0 must be configured before MTU set"
Will add it to commit msg in v2
Regards,
Wan Junjie
> Thanks.
>
> 在 2022/2/14 16:13, Wan Junjie 写道:
> > When initializing a bonding device with all slaves in vdev string,
> > which means bonding device initilizes slaves, instead of initializing
> > slaves before creating the bonding device, it will fail to call
> > set_mtu api for the bonding device.
> >
> > Fixes:b26bee1 ("ethdev: forbid MTU set before device configure")
> >
> > Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>
> > ---
> > app/test/test_link_bonding.c | 4 +++
> > app/test/test_link_bonding_rssconf.c | 4 +++
> > drivers/net/bonding/eth_bond_private.h | 4 +++
> > drivers/net/bonding/rte_eth_bond_api.c | 6 ++++
> > drivers/net/bonding/rte_eth_bond_pmd.c | 43 ++++++++++++++++++++++----
> > 5 files changed, 55 insertions(+), 6 deletions(-)
> >
> > diff --git a/app/test/test_link_bonding.c b/app/test/test_link_bonding.c
> > index dc6fc46b9c..12c50ef393 100644
> > --- a/app/test/test_link_bonding.c
> > +++ b/app/test/test_link_bonding.c
> > @@ -181,6 +181,10 @@ configure_ethdev(uint16_t port_id, uint8_t start, uint8_t en_isr)
> > test_params->nb_tx_q, &default_pmd_conf),
> > "rte_eth_dev_configure for port %d failed", port_id);
> >
> > + int ret = rte_eth_dev_set_mtu(port_id, 1550);
> > + RTE_TEST_ASSERT(ret == 0 || ret == -ENOTSUP,
> > + "rte_eth_dev_set_mtu for port %d failed", port_id);
> > +
> > for (q_id = 0; q_id < test_params->nb_rx_q; q_id++)
> > TEST_ASSERT_SUCCESS(rte_eth_rx_queue_setup(port_id, q_id, RX_RING_SIZE,
> > rte_eth_dev_socket_id(port_id), &rx_conf_default,
> > diff --git a/app/test/test_link_bonding_rssconf.c b/app/test/test_link_bonding_rssconf.c
> > index f9eae93973..7228965ced 100644
> > --- a/app/test/test_link_bonding_rssconf.c
> > +++ b/app/test/test_link_bonding_rssconf.c
> > @@ -128,6 +128,10 @@ configure_ethdev(uint16_t port_id, struct rte_eth_conf *eth_conf,
> > RXTX_QUEUE_COUNT, eth_conf) == 0, "Failed to configure device %u",
> > port_id);
> >
> > + int ret = rte_eth_dev_set_mtu(port_id, 1550);
> > + RTE_TEST_ASSERT(ret == 0 || ret == -ENOTSUP,
> > + "rte_eth_dev_set_mtu for port %d failed", port_id);
> > +
> > for (rxq = 0; rxq < RXTX_QUEUE_COUNT; rxq++) {
> > TEST_ASSERT(rte_eth_rx_queue_setup(port_id, rxq, RXTX_RING_SIZE,
> > rte_eth_dev_socket_id(port_id), NULL,
> > diff --git a/drivers/net/bonding/eth_bond_private.h b/drivers/net/bonding/eth_bond_private.h
> > index 156335c425..8222e3cd38 100644
> > --- a/drivers/net/bonding/eth_bond_private.h
> > +++ b/drivers/net/bonding/eth_bond_private.h
> > @@ -246,6 +246,10 @@ int
> > slave_configure(struct rte_eth_dev *bonded_eth_dev,
> > struct rte_eth_dev *slave_eth_dev);
> >
> > +int
> > +slave_start(struct rte_eth_dev *bonded_eth_dev,
> > + struct rte_eth_dev *slave_eth_dev);
> > +
> > void
> > slave_remove(struct bond_dev_private *internals,
> > struct rte_eth_dev *slave_eth_dev);
> > diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
> > index b78867b125..4ac191c468 100644
> > --- a/drivers/net/bonding/rte_eth_bond_api.c
> > +++ b/drivers/net/bonding/rte_eth_bond_api.c
> > @@ -566,6 +566,12 @@ __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t slave_port_id)
> > slave_port_id);
> > return -1;
> > }
> > + if (slave_start(bonded_eth_dev, slave_eth_dev) != 0) {
> > + internals->slave_count--;
> > + RTE_BOND_LOG(ERR, "rte_bond_slaves_start: port=%d",
> > + slave_port_id);
> > + return -1;
> > + }
> > }
> >
> > /* Update all slave devices MACs */
> > diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> > index aa6519f83c..c31b723071 100644
> > --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> > +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> > @@ -1678,14 +1678,10 @@ int
> > slave_configure(struct rte_eth_dev *bonded_eth_dev,
> > struct rte_eth_dev *slave_eth_dev)
> > {
> > - struct bond_rx_queue *bd_rx_q;
> > - struct bond_tx_queue *bd_tx_q;
> > uint16_t nb_rx_queues;
> > uint16_t nb_tx_queues;
> >
> > int errval;
> > - uint16_t q_id;
> > - struct rte_flow_error flow_error;
> >
> > struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
> >
> > @@ -1758,6 +1754,19 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> > slave_eth_dev->data->port_id, errval);
> > return errval;
> > }
> > + return 0;
> > +}
> > +
> > +int
> > +slave_start(struct rte_eth_dev *bonded_eth_dev,
> > + struct rte_eth_dev *slave_eth_dev)
> > +{
> > + int errval = 0;
> > + struct bond_rx_queue *bd_rx_q;
> > + struct bond_tx_queue *bd_tx_q;
> > + uint16_t q_id;
> > + struct rte_flow_error flow_error;
> > + struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
> >
> > /* Setup Rx Queues */
> > for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> > @@ -1806,10 +1815,13 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> > return errval;
> > }
> >
> > - if (internals->mode4.dedicated_queues.flow[slave_eth_dev->data->port_id] != NULL)
> > - rte_flow_destroy(slave_eth_dev->data->port_id,
> > + if (internals->mode4.dedicated_queues.flow[slave_eth_dev->data->port_id] != NULL) {
> > + errval = rte_flow_destroy(slave_eth_dev->data->port_id,
> > internals->mode4.dedicated_queues.flow[slave_eth_dev->data->port_id],
> > &flow_error);
> > + RTE_BOND_LOG(ERR, "bond_ethdev_8023ad_flow_destroy: port=%d, err (%d)",
> > + slave_eth_dev->data->port_id, errval);
> > + }
> >
> > errval = bond_ethdev_8023ad_flow_set(bonded_eth_dev,
> > slave_eth_dev->data->port_id);
> > @@ -2001,6 +2013,13 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
> > internals->slaves[i].port_id);
> > goto out_err;
> > }
> > + if (slave_start(eth_dev, slave_ethdev) != 0) {
> > + RTE_BOND_LOG(ERR,
> > + "bonded port (%d) failed to start slave device (%d)",
> > + eth_dev->data->port_id,
> > + internals->slaves[i].port_id);
> > + goto out_err;
> > + }
> > /* We will need to poll for link status if any slave doesn't
> > * support interrupts
> > */
> > @@ -3849,6 +3868,18 @@ bond_ethdev_configure(struct rte_eth_dev *dev)
> > return -1;
> > }
> >
> > + /* configure slaves so we can pass mtu setting */
> > + for (i = 0; i < internals->slave_count; i++) {
> > + struct rte_eth_dev *slave_ethdev =
> > + &(rte_eth_devices[internals->slaves[i].port_id]);
> > + if (slave_configure(dev, slave_ethdev) != 0) {
> > + RTE_BOND_LOG(ERR,
> > + "bonded port (%d) failed to configure slave device (%d)",
> > + dev->data->port_id,
> > + internals->slaves[i].port_id);
> > + return -1;
> > + }
> > + }
> > return 0;
> > }
> >
> >
@@ -181,6 +181,10 @@ configure_ethdev(uint16_t port_id, uint8_t start, uint8_t en_isr)
test_params->nb_tx_q, &default_pmd_conf),
"rte_eth_dev_configure for port %d failed", port_id);
+ int ret = rte_eth_dev_set_mtu(port_id, 1550);
+ RTE_TEST_ASSERT(ret == 0 || ret == -ENOTSUP,
+ "rte_eth_dev_set_mtu for port %d failed", port_id);
+
for (q_id = 0; q_id < test_params->nb_rx_q; q_id++)
TEST_ASSERT_SUCCESS(rte_eth_rx_queue_setup(port_id, q_id, RX_RING_SIZE,
rte_eth_dev_socket_id(port_id), &rx_conf_default,
@@ -128,6 +128,10 @@ configure_ethdev(uint16_t port_id, struct rte_eth_conf *eth_conf,
RXTX_QUEUE_COUNT, eth_conf) == 0, "Failed to configure device %u",
port_id);
+ int ret = rte_eth_dev_set_mtu(port_id, 1550);
+ RTE_TEST_ASSERT(ret == 0 || ret == -ENOTSUP,
+ "rte_eth_dev_set_mtu for port %d failed", port_id);
+
for (rxq = 0; rxq < RXTX_QUEUE_COUNT; rxq++) {
TEST_ASSERT(rte_eth_rx_queue_setup(port_id, rxq, RXTX_RING_SIZE,
rte_eth_dev_socket_id(port_id), NULL,
@@ -246,6 +246,10 @@ int
slave_configure(struct rte_eth_dev *bonded_eth_dev,
struct rte_eth_dev *slave_eth_dev);
+int
+slave_start(struct rte_eth_dev *bonded_eth_dev,
+ struct rte_eth_dev *slave_eth_dev);
+
void
slave_remove(struct bond_dev_private *internals,
struct rte_eth_dev *slave_eth_dev);
@@ -566,6 +566,12 @@ __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t slave_port_id)
slave_port_id);
return -1;
}
+ if (slave_start(bonded_eth_dev, slave_eth_dev) != 0) {
+ internals->slave_count--;
+ RTE_BOND_LOG(ERR, "rte_bond_slaves_start: port=%d",
+ slave_port_id);
+ return -1;
+ }
}
/* Update all slave devices MACs */
@@ -1678,14 +1678,10 @@ int
slave_configure(struct rte_eth_dev *bonded_eth_dev,
struct rte_eth_dev *slave_eth_dev)
{
- struct bond_rx_queue *bd_rx_q;
- struct bond_tx_queue *bd_tx_q;
uint16_t nb_rx_queues;
uint16_t nb_tx_queues;
int errval;
- uint16_t q_id;
- struct rte_flow_error flow_error;
struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
@@ -1758,6 +1754,19 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
slave_eth_dev->data->port_id, errval);
return errval;
}
+ return 0;
+}
+
+int
+slave_start(struct rte_eth_dev *bonded_eth_dev,
+ struct rte_eth_dev *slave_eth_dev)
+{
+ int errval = 0;
+ struct bond_rx_queue *bd_rx_q;
+ struct bond_tx_queue *bd_tx_q;
+ uint16_t q_id;
+ struct rte_flow_error flow_error;
+ struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
/* Setup Rx Queues */
for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
@@ -1806,10 +1815,13 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
return errval;
}
- if (internals->mode4.dedicated_queues.flow[slave_eth_dev->data->port_id] != NULL)
- rte_flow_destroy(slave_eth_dev->data->port_id,
+ if (internals->mode4.dedicated_queues.flow[slave_eth_dev->data->port_id] != NULL) {
+ errval = rte_flow_destroy(slave_eth_dev->data->port_id,
internals->mode4.dedicated_queues.flow[slave_eth_dev->data->port_id],
&flow_error);
+ RTE_BOND_LOG(ERR, "bond_ethdev_8023ad_flow_destroy: port=%d, err (%d)",
+ slave_eth_dev->data->port_id, errval);
+ }
errval = bond_ethdev_8023ad_flow_set(bonded_eth_dev,
slave_eth_dev->data->port_id);
@@ -2001,6 +2013,13 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
internals->slaves[i].port_id);
goto out_err;
}
+ if (slave_start(eth_dev, slave_ethdev) != 0) {
+ RTE_BOND_LOG(ERR,
+ "bonded port (%d) failed to start slave device (%d)",
+ eth_dev->data->port_id,
+ internals->slaves[i].port_id);
+ goto out_err;
+ }
/* We will need to poll for link status if any slave doesn't
* support interrupts
*/
@@ -3849,6 +3868,18 @@ bond_ethdev_configure(struct rte_eth_dev *dev)
return -1;
}
+ /* configure slaves so we can pass mtu setting */
+ for (i = 0; i < internals->slave_count; i++) {
+ struct rte_eth_dev *slave_ethdev =
+ &(rte_eth_devices[internals->slaves[i].port_id]);
+ if (slave_configure(dev, slave_ethdev) != 0) {
+ RTE_BOND_LOG(ERR,
+ "bonded port (%d) failed to configure slave device (%d)",
+ dev->data->port_id,
+ internals->slaves[i].port_id);
+ return -1;
+ }
+ }
return 0;
}