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 99AC2A04A7; Tue, 8 Feb 2022 02:12:54 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2F2434014E; Tue, 8 Feb 2022 02:12:54 +0100 (CET) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 8E84240141 for ; Tue, 8 Feb 2022 02:12:52 +0100 (CET) Received: from kwepemi100017.china.huawei.com (unknown [172.30.72.57]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4Jt4jK3SjTzbk3B; Tue, 8 Feb 2022 09:11:49 +0800 (CST) Received: from kwepemm600004.china.huawei.com (7.193.23.242) by kwepemi100017.china.huawei.com (7.221.188.163) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.21; Tue, 8 Feb 2022 09:12:49 +0800 Received: from [10.67.103.231] (10.67.103.231) by kwepemm600004.china.huawei.com (7.193.23.242) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.21; Tue, 8 Feb 2022 09:12:49 +0800 Message-ID: Date: Tue, 8 Feb 2022 09:12:48 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [dpdk-dev] [PATCH 2/3] app/testpmd: fix slave device isn't released To: Ferruh Yigit , "Min Hu (Connor)" , CC: , References: <20211025063922.34066-1-humin29@huawei.com> <20211025063922.34066-3-humin29@huawei.com> From: "lihuisong (C)" In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.231] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To kwepemm600004.china.huawei.com (7.193.23.242) 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/4 20:14, Ferruh Yigit 写道: > On 10/25/2021 7:39 AM, Min Hu (Connor) wrote: >> From: Huisong Li >> >> Currently, some eth devices are added to bond device, these devices >> are not >> released when the quit command is executed in testpmd. This patch >> adds the >> release operation for all active slaves under a bond device. >> > > 'close_port()' function traverses all ports, when bonding is close, if it > removes the slaves, won't 'close_port()' close slave devices? Yes > > If so this prevents, 'cl_quit' global variable. The variable is used to ensure that the slave ports are not closed when the bonding port is closed, and are closed when testpmd quit or is killed. > >> Fixes: 0e545d3047fe ("app/testpmd: check stopping port is not in >> bonding") >> Cc: stable@dpdk.org >> >> Signed-off-by: Huisong Li >> Signed-off-by: Min Hu (Connor) >> --- >>   app/test-pmd/cmdline.c |  1 + >>   app/test-pmd/testpmd.c | 67 ++++++++++++++++++++++++++++++++++++------ >>   app/test-pmd/testpmd.h |  1 + >>   3 files changed, 60 insertions(+), 9 deletions(-) >> >> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c >> index 5bfb4b509b..98236a8be3 100644 >> --- a/app/test-pmd/cmdline.c >> +++ b/app/test-pmd/cmdline.c >> @@ -8743,6 +8743,7 @@ static void cmd_quit_parsed(__rte_unused void >> *parsed_result, >>                   __rte_unused void *data) >>   { >>       cmdline_quit(cl); >> +    cl_quit = 1; >>   } >>     cmdline_parse_token_string_t cmd_quit_quit = >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c >> index d6b9ebc4dd..fea9744bd6 100644 >> --- a/app/test-pmd/testpmd.c >> +++ b/app/test-pmd/testpmd.c >> @@ -221,6 +221,7 @@ unsigned int xstats_display_num; /**< Size of >> extended statistics to show */ >>    * option. Set flag to exit stats period loop after received >> SIGINT/SIGTERM. >>    */ >>   uint8_t f_quit; >> +uint8_t cl_quit; /* Quit testpmd from cmdline. */ >>     /* >>    * Max Rx frame size, set by '--max-pkt-len' parameter. >> @@ -613,15 +614,6 @@ eth_dev_start_mp(uint16_t port_id) >>       return 0; >>   } >>   -static int >> -eth_dev_stop_mp(uint16_t port_id) >> -{ >> -    if (is_proc_primary()) >> -        return rte_eth_dev_stop(port_id); >> - >> -    return 0; >> -} >> - >>   static void >>   mempool_free_mp(struct rte_mempool *mp) >>   { >> @@ -3123,6 +3115,55 @@ remove_invalid_ports(void) >>       nb_cfg_ports = nb_fwd_ports; >>   } >>   +#ifdef RTE_NET_BOND >> +static void >> +handle_bonding_slave_device(portid_t bond_pid) > > 'handle' in the function is not very specific, it is not clear > what this function does. ok > >> +{ >> +    portid_t slave_pids[RTE_MAX_ETHPORTS]; >> +    struct rte_port *port; >> +    portid_t slave_pid; >> +    int num_slaves; >> +    int i; >> + >> +    num_slaves = rte_eth_bond_slaves_get(bond_pid, slave_pids, >> +                         RTE_MAX_ETHPORTS); >> +    if (num_slaves < 0) { >> +        fprintf(stderr, "Failed to get slave list for port = %u\n", >> +            bond_pid); >> +        return; >> +    } >> + >> +    for (i = 0; i < num_slaves; i++) { >> +        slave_pid = slave_pids[i]; >> +        /* Before removing a slave device, stop the slave device. */ >> +        if (port_is_started(slave_pid) == 1) { >> +            if (rte_eth_dev_stop(slave_pid) != 0) >> +                fprintf(stderr, "rte_eth_dev_stop failed for port >> %u\n", >> +                    slave_pid); >> + >> +            port = &ports[slave_pid]; >> +            if (rte_atomic16_cmpset(&(port->port_status), >> +                RTE_PORT_STARTED, RTE_PORT_STOPPED) == 0) >> +                fprintf(stderr, "Port %u can not be set into >> stopped\n", >> +                    slave_pid); >> +        } >> + >> +        /* >> +         * Remove the slave from a bonded device in case of failing to >> +         * close bond device. >> +         */ >> +        if (rte_eth_bond_slave_remove(bond_pid, slave_pid) != 0) > > Similar to Aman's comment in previous patch, if a bonding device is > removed > shouldn't the bonding PMD stop the slaves and removed them, instead of > application? I agree. I plan to remove them, and move the operations of clearing the slave flag and closing slave port to after the bonding port is closed. > >> +            fprintf(stderr, "Failed to remove slave %u from master >> port = %u\n", >> +                slave_pid, bond_pid); >> +        clear_port_slave_flag(slave_pid); >> + >> +        /* Close slave device when testpmd quit or is killed. */ >> +        if (cl_quit == 1 || f_quit == 1) >> +            rte_eth_dev_close(slave_pid); >> +    } >> +} >> +#endif >> + >>   void >>   close_port(portid_t pid) >>   { >> @@ -3161,6 +3202,14 @@ close_port(portid_t pid) >>             if (is_proc_primary()) { >>               port_flow_flush(pi); >> +#ifdef RTE_NET_BOND >> +            /* >> +             * If this port is bond device, all slaves under the >> +             * device need to be removed or closed. >> +             */ >> +            if (port->bond_flag == 1) >> +                handle_bonding_slave_device(pi); >> +#endif >>               port_flex_item_flush(pi); >>               rte_eth_dev_close(pi); >>           } >> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h >> index ad3b4f875c..216fc41432 100644 >> --- a/app/test-pmd/testpmd.h >> +++ b/app/test-pmd/testpmd.h >> @@ -27,6 +27,7 @@ >>   #define RTE_PORT_STARTED        (uint16_t)1 >>   #define RTE_PORT_CLOSED         (uint16_t)2 >>   #define RTE_PORT_HANDLING       (uint16_t)3 >> +extern uint8_t cl_quit; >>     /* >>    * It is used to allocate the memory for hash key. > > .