[PATCH 1/2] net/bonding: add error hint for invald args

[PATCH 1/2] net/bonding: add error hint for invald args

[Wan Junjie]

When invalid args exsit, application exits with no error hint.
Adding a log message here will help users to know the reson.

Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index bfa931098e..aa6519f83c 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -3439,8 +3439,10 @@ bond_probe(struct rte_vdev_device *dev)
 
 	kvlist = rte_kvargs_parse(rte_vdev_device_args(dev),
 		pmd_bond_init_valid_arguments);
-	if (kvlist == NULL)
+	if (kvlist == NULL) {
+		RTE_BOND_LOG(ERR, "Invalid args in %s", rte_vdev_device_args(dev));
 		return -1;
+	}
 
 	/* Parse link bonding mode */
 	if (rte_kvargs_count(kvlist, PMD_BOND_MODE_KVARG) == 1) {
-- 
2.33.0

[PATCH 2/2] net/bonding: fix slaves initilizing on mtu setting

[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;
 }
 
-- 
2.33.0

Re: [PATCH 1/2] net/bonding: add error hint for invald args

[Min Hu (Connor)]

Hi, wan,
	The headline has spelling errors.

在 2022/2/14 16:13, Wan Junjie 写道:
> When invalid args exsit, application exits with no error hint.
> Adding a log message here will help users to know the reson.
> 
> Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>
> ---
>   drivers/net/bonding/rte_eth_bond_pmd.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index bfa931098e..aa6519f83c 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -3439,8 +3439,10 @@ bond_probe(struct rte_vdev_device *dev)
>   
>   	kvlist = rte_kvargs_parse(rte_vdev_device_args(dev),
>   		pmd_bond_init_valid_arguments);
> -	if (kvlist == NULL)
> +	if (kvlist == NULL) {
> +		RTE_BOND_LOG(ERR, "Invalid args in %s", rte_vdev_device_args(dev));
>   		return -1;
> +	}
>   
>   	/* Parse link bonding mode */
>   	if (rte_kvargs_count(kvlist, PMD_BOND_MODE_KVARG) == 1) {
> 

Re: [PATCH 2/2] net/bonding: fix slaves initilizing on mtu setting

[Min Hu (Connor)]

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;
>   }
>   
> 

Re: [External] Re: [PATCH 1/2] net/bonding: add error hint for invald args

[Wan Junjie]

Hi Min Hu,

Thanks for the reply
Will fix it in v2.

Regards,
Wan Junjie

On Tue, Feb 15, 2022 at 5:12 PM Min Hu (Connor) <humin29@huawei.com> wrote:
>
> Hi, wan,
>         The headline has spelling errors.
>
> 在 2022/2/14 16:13, Wan Junjie 写道:
> > When invalid args exsit, application exits with no error hint.
> > Adding a log message here will help users to know the reson.
> >
> > Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>
> > ---
> >   drivers/net/bonding/rte_eth_bond_pmd.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> > index bfa931098e..aa6519f83c 100644
> > --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> > +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> > @@ -3439,8 +3439,10 @@ bond_probe(struct rte_vdev_device *dev)
> >
> >       kvlist = rte_kvargs_parse(rte_vdev_device_args(dev),
> >               pmd_bond_init_valid_arguments);
> > -     if (kvlist == NULL)
> > +     if (kvlist == NULL) {
> > +             RTE_BOND_LOG(ERR, "Invalid args in %s", rte_vdev_device_args(dev));
> >               return -1;
> > +     }
> >
> >       /* Parse link bonding mode */
> >       if (rte_kvargs_count(kvlist, PMD_BOND_MODE_KVARG) == 1) {
> >

Re: [External] Re: [PATCH 2/2] net/bonding: fix slaves initilizing on mtu setting

[Wan Junjie]

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;
> >   }
> >
> >

[PATCH v2 1/2] net/bonding: add error hint for invalid args

[Wan Junjie]

When invalid args exist, application exits with no error hint.
Adding a log message here will help users to know the reason.

Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>
---
v2: fix typo in commit msg
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index bfa931098e..aa6519f83c 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -3439,8 +3439,10 @@ bond_probe(struct rte_vdev_device *dev)
 
 	kvlist = rte_kvargs_parse(rte_vdev_device_args(dev),
 		pmd_bond_init_valid_arguments);
-	if (kvlist == NULL)
+	if (kvlist == NULL) {
+		RTE_BOND_LOG(ERR, "Invalid args in %s", rte_vdev_device_args(dev));
 		return -1;
+	}
 
 	/* Parse link bonding mode */
 	if (rte_kvargs_count(kvlist, PMD_BOND_MODE_KVARG) == 1) {
-- 
2.33.0

[PATCH v2 2/2] net/bonding: fix slaves initializing on mtu setting

[Wan Junjie]

If a initial process for the bonding device is like:
rte_eth_dev_configure
rte_eth_dev_set_mtu
queue setup and start, etc.

Pass the vdev args to application, and init bonding device only.
-a 0000:af:00.0 --vdev="net_bonding0,mode=2,slave=0000:af:00.0"

It will fail and compain for the slave device
"Port 0 must be configured before MTU set"

Test can be reproduced with ovs.

Fixes: b26bee1 ("ethdev: forbid MTU set before device configure")

Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>
---
v2:
  fix typo in commit
  add reproduce procedure
---
 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;
 }
 
-- 
2.33.0

Re: [PATCH v2 1/2] net/bonding: add error hint for invalid args

[Ferruh Yigit]

On 2/15/2022 10:56 AM, Wan Junjie wrote:
> When invalid args exist, application exits with no error hint.
> Adding a log message here will help users to know the reason.
> 
> Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [PATCH v2 1/2] net/bonding: add error hint for invalid args

[Min Hu (Connor)]

Acked-by: Min Hu (Connor) <humin29@huawei.com>

在 2022/2/15 18:56, Wan Junjie 写道:
> When invalid args exist, application exits with no error hint.
> Adding a log message here will help users to know the reason.
> 
> Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>
> ---
> v2: fix typo in commit msg
> ---
>   drivers/net/bonding/rte_eth_bond_pmd.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index bfa931098e..aa6519f83c 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -3439,8 +3439,10 @@ bond_probe(struct rte_vdev_device *dev)
>   
>   	kvlist = rte_kvargs_parse(rte_vdev_device_args(dev),
>   		pmd_bond_init_valid_arguments);
> -	if (kvlist == NULL)
> +	if (kvlist == NULL) {
> +		RTE_BOND_LOG(ERR, "Invalid args in %s", rte_vdev_device_args(dev));
>   		return -1;
> +	}
>   
>   	/* Parse link bonding mode */
>   	if (rte_kvargs_count(kvlist, PMD_BOND_MODE_KVARG) == 1) {
> 

Re: [PATCH v2 2/2] net/bonding: fix slaves initializing on mtu setting

[Ferruh Yigit]

On 2/15/2022 10:59 AM, Wan Junjie wrote:
> If a initial process for the bonding device is like:
> rte_eth_dev_configure
> rte_eth_dev_set_mtu
> queue setup and start, etc.
> 
> Pass the vdev args to application, and init bonding device only.
> -a 0000:af:00.0 --vdev="net_bonding0,mode=2,slave=0000:af:00.0"
> 
> It will fail and compain for the slave device
> "Port 0 must be configured before MTU set"
> 

Change looks good to me, previously configure(), queue setup & start()
all done in .dev_start dev_ops.

Now configure part separated and .dev_configure also calls the configure
part, .dev_start still does the same.

Conner, Chas, if there is no objection from you, I will proceed with
the patch and merge it soon.

> Test can be reproduced with ovs.
> 
> Fixes: b26bee1 ("ethdev: forbid MTU set before device configure")
> 
> Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>

Tested-by: Ferruh Yigit <ferruh.yigit@intel.com>

> ---
> v2:
>    fix typo in commit
>    add reproduce procedure
> ---
>   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;
>   }
>   

Re: [PATCH v2 2/2] net/bonding: fix slaves initializing on mtu setting

[Min Hu (Connor)]

在 2022/2/16 21:22, Ferruh Yigit 写道:
> On 2/15/2022 10:59 AM, Wan Junjie wrote:
>> If a initial process for the bonding device is like:
>> rte_eth_dev_configure
>> rte_eth_dev_set_mtu
>> queue setup and start, etc.
>>
>> Pass the vdev args to application, and init bonding device only.
>> -a 0000:af:00.0 --vdev="net_bonding0,mode=2,slave=0000:af:00.0"
>>
>> It will fail and compain for the slave device
>> "Port 0 must be configured before MTU set"
>>
> 
> Change looks good to me, previously configure(), queue setup & start()
> all done in .dev_start dev_ops.
> 
> Now configure part separated and .dev_configure also calls the configure
> part, .dev_start still does the same.
> 
> Conner, Chas, if there is no objection from you, I will proceed with
> the patch and merge it soon.
> 
Hi, Ferruh,
	yeah, I am OK with it.
	thanks.
>> Test can be reproduced with ovs.
>>
>> Fixes: b26bee1 ("ethdev: forbid MTU set before device configure")
>>
>> Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>
> 
> Tested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
>> ---
>> v2:
>>    fix typo in commit
>>    add reproduce procedure
>> ---
>>   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;
>>   }
> 
> .

Re: [PATCH v2 1/2] net/bonding: add error hint for invalid args

[Ferruh Yigit]

On 2/15/2022 11:56 AM, Ferruh Yigit wrote:
> On 2/15/2022 10:56 AM, Wan Junjie wrote:
>> When invalid args exist, application exits with no error hint.
>> Adding a log message here will help users to know the reason.
>>
>> Signed-off-by: Wan Junjie <wanjunjie@bytedance.com>
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

Series applied to dpdk-next-net/main, thanks.