From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1CE8AA0032; Thu, 17 Feb 2022 09:55:41 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0AD4C40150; Thu, 17 Feb 2022 09:55:41 +0100 (CET) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 0ED8840042 for ; Thu, 17 Feb 2022 09:55:40 +0100 (CET) Received: from dggeme756-chm.china.huawei.com (unknown [172.30.72.55]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4JzpTG3XkBzZfkc; Thu, 17 Feb 2022 16:51:14 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by dggeme756-chm.china.huawei.com (10.3.19.102) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2308.21; Thu, 17 Feb 2022 16:55:37 +0800 Subject: Re: [PATCH v2 2/2] net/bonding: fix slaves initializing on mtu setting To: Ferruh Yigit , Wan Junjie , CC: Chas Williams References: <20220214081344.13637-2-wanjunjie@bytedance.com> <20220215105940.60051-1-wanjunjie@bytedance.com> <433559c8-d367-d22b-45b8-8217c190a520@intel.com> From: "Min Hu (Connor)" Message-ID: <3116d0c0-b93f-b134-5a72-de10e84f1777@huawei.com> Date: Thu, 17 Feb 2022 16:55:37 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: <433559c8-d367-d22b-45b8-8217c190a520@intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To dggeme756-chm.china.huawei.com (10.3.19.102) X-CFilter-Loop: Reflected X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 在 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 > > Tested-by: Ferruh Yigit > >> --- >> 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; >>   } > > .