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 0CE3AA034C for ; Fri, 29 Apr 2022 08:52:45 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F1F7F42829; Fri, 29 Apr 2022 08:52:44 +0200 (CEST) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id D98BC40E50; Fri, 29 Apr 2022 08:52:42 +0200 (CEST) Received: from kwepemi500012.china.huawei.com (unknown [172.30.72.57]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4KqNTJ1gR2zhYtD; Fri, 29 Apr 2022 14:52:20 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by kwepemi500012.china.huawei.com (7.221.188.12) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Fri, 29 Apr 2022 14:52:41 +0800 Subject: Re: [PATCH V2 2/4] net/bonding: fix non-terminable while loop To: Ferruh Yigit , CC: Huisong Li , , Chas Williams , Andrew Rybchenko , Ivan Ilchenko References: <20211025063922.34066-1-humin29@huawei.com> <20220324030036.4761-1-humin29@huawei.com> <20220324030036.4761-3-humin29@huawei.com> From: "Min Hu (Connor)" Message-ID: <2e41f4b4-aa52-b219-b597-3e301005e7df@huawei.com> Date: Fri, 29 Apr 2022 14:52:40 +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: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To kwepemi500012.china.huawei.com (7.221.188.12) X-CFilter-Loop: Reflected X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Hi, Ferruh, 在 2022/4/27 2:19, Ferruh Yigit 写道: > On 3/24/2022 3:00 AM, Min Hu (Connor) wrote: >> From: Huisong Li >> >> All slaves will be stopped and removed when closing a bonded port. But >> the >> while loop can not stop if both rte_eth_dev_stop and >> rte_eth_bond_slave_remove fail to run. >> > > Agree that this is a defect introduced in below commit. Thanks for the fix. thanks. > >> Fixes: fb0379bc5db3 ("net/bonding: check stop call status") >> Cc: stable@dpdk.org >> >> Signed-off-by: Huisong Li >> Signed-off-by: Min Hu (Connor) >> --- >>   drivers/net/bonding/rte_eth_bond_pmd.c | 3 ++- >>   1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c >> b/drivers/net/bonding/rte_eth_bond_pmd.c >> index 469dc71170..00d4deda44 100644 >> --- a/drivers/net/bonding/rte_eth_bond_pmd.c >> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c >> @@ -2149,13 +2149,14 @@ bond_ethdev_close(struct rte_eth_dev *dev) >>           return 0; >>       RTE_BOND_LOG(INFO, "Closing bonded device %s", dev->device->name); >> -    while (internals->slave_count != skipped) { >> +    while (skipped < internals->slave_count) { > > When below fixed with adding 'continue', no need to change the check, > right? Although new one is also correct. Agreed. > >>           uint16_t port_id = internals->slaves[skipped].port_id; >>           if (rte_eth_dev_stop(port_id) != 0) { >>               RTE_BOND_LOG(ERR, "Failed to stop device on port %u", >>                        port_id); >>               skipped++; >> +            continue; > > Can't we remove the slave even if 'stop()' failed? If so I think better > to just log the error and keep continue in that case, what do you think? NO, if slave stop failed, we cannot remove the slave. just see the function stack: rte_eth_bond_slave_remove __eth_bond_slave_remove_lock_free slave_remove rte_eth_dev_internal_reset if (dev->data->dev_started) { RTE_ETHDEV_LOG(ERR, "Port %u must be stopped to allow reset\n", dev->data->port_id); return; } > >>           } >>           if (rte_eth_bond_slave_remove(bond_port_id, port_id) != 0) { > > .