patches for DPDK stable branches
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@xilinx.com>
To: "Min Hu (Connor)" <humin29@huawei.com>,
	Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>,
	<dev@dpdk.org>
Cc: Huisong Li <lihuisong@huawei.com>, <stable@dpdk.org>,
	Aman Singh <aman.deep.singh@intel.com>,
	Xiaoyun Li <xiaoyun.li@intel.com>,
	Yuying Zhang <yuying.zhang@intel.com>,
	Pablo de Lara <pablo.de.lara.guarch@intel.com>,
	Bernard Iremonger <bernard.iremonger@intel.com>
Subject: Re: [PATCH v3 3/5] app/testpmd: fix port status of slave device
Date: Tue, 10 May 2022 17:34:14 +0100
Message-ID: <bbc13ccc-cb79-3325-8bb5-377e4320cd38@xilinx.com> (raw)
In-Reply-To: <ce00ffaa-715b-9734-14a0-1178a9041b8f@huawei.com>

On 5/6/2022 9:16 AM, Min Hu (Connor) wrote:
> Hi, Konstantin,
> 
> 在 2022/5/4 7:39, Konstantin Ananyev 写道:
>> 03/05/2022 11:02, Min Hu (Connor) пишет:
>>> From: Huisong Li <lihuisong@huawei.com>
>>>
>>> Starting or stopping a bonded port also starts or stops all active 
>>> slaves
>>> under the bonded port. If this port is a bonded device, we need to 
>>> modify
>>> the port status of all slaves.
>>>
>>> Fixes: 0e545d3047fe ("app/testpmd: check stopping port is not in 
>>> bonding")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> Acked-by: Aman Singh <aman.deep.singh@intel.com>
>>> ---
>>>   app/test-pmd/cmdline.c |  1 +
>>>   app/test-pmd/testpmd.c | 74 +++++++++++++++++++++++++++++++++++++++---
>>>   app/test-pmd/testpmd.h |  3 +-
>>>   3 files changed, 73 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>> index 6ffea8e21a..d9fc7a88bd 100644
>>> --- a/app/test-pmd/cmdline.c
>>> +++ b/app/test-pmd/cmdline.c
>>> @@ -6671,6 +6671,7 @@ static void 
>>> cmd_create_bonded_device_parsed(void *parsed_result,
>>>                   "Failed to enable promiscuous mode for port %u: %s 
>>> - ignore\n",
>>>                   port_id, rte_strerror(-ret));
>>> +        ports[port_id].bond_flag = 1;
>>>           ports[port_id].need_setup = 0;
>>>           ports[port_id].port_status = RTE_PORT_STOPPED;
>>>       }
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>>> index fe2ce19f99..dc90600787 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -66,6 +66,9 @@
>>>   #ifdef RTE_EXEC_ENV_WINDOWS
>>>   #include <process.h>
>>>   #endif
>>> +#ifdef RTE_NET_BOND
>>> +#include <rte_eth_bond.h>
>>> +#endif
>>>   #include "testpmd.h"
>>> @@ -597,11 +600,57 @@ eth_dev_configure_mp(uint16_t port_id, uint16_t 
>>> nb_rx_q, uint16_t nb_tx_q,
>>>       return 0;
>>>   }
>>> +#ifdef RTE_NET_BOND
>>> +static int
>>> +change_bonding_slave_port_status(portid_t bond_pid, bool is_stop)
>>> +{
>>> +    portid_t slave_pids[RTE_MAX_ETHPORTS];
>>> +    struct rte_port *port;
>>> +    int num_slaves;
>>> +    portid_t slave_pid;
>>> +    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 num_slaves;
>>> +    }
>>> +
>>> +    for (i = 0; i < num_slaves; i++) {
>>> +        slave_pid = slave_pids[i];
>>> +        port = &ports[slave_pid];
>>> +        port->port_status =
>>> +            is_stop ? RTE_PORT_STOPPED : RTE_PORT_STARTED;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +#endif
>>> +
>>>   static int
>>>   eth_dev_start_mp(uint16_t port_id)
>>>   {
>>> -    if (is_proc_primary())
>>> -        return rte_eth_dev_start(port_id);
>>> +    int ret;
>>> +
>>> +    if (is_proc_primary()) {
>>> +        ret = rte_eth_dev_start(port_id);
>>> +        if (ret != 0)
>>> +            return ret;
>>> +
>>> +#ifdef RTE_NET_BOND
>>> +        struct rte_port *port = &ports[port_id];
>>> +
>>> +        /*
>>> +         * Starting a bonded port also starts all slaves under the 
>>> bonded
>>> +         * device. So if this port is bond device, we need to modify 
>>> the
>>> +         * port status of these slaves.
>>> +         */
>>> +        if (port->bond_flag == 1)
>>> +            return change_bonding_slave_port_status(port_id, false);
>>> +#endif
>>> +    }
>>>       return 0;
>>>   }
>>> @@ -609,8 +658,25 @@ eth_dev_start_mp(uint16_t port_id)
>>>   static int
>>>   eth_dev_stop_mp(uint16_t port_id)
>>>   {
>>> -    if (is_proc_primary())
>>> -        return rte_eth_dev_stop(port_id);
>>> +    int ret;
>>> +
>>> +    if (is_proc_primary()) {
>>> +        ret = rte_eth_dev_stop(port_id);
>>> +        if (ret != 0)
>>> +            return ret;
>>> +
>>> +#ifdef RTE_NET_BOND
>>
>> Here and in other places - probably no need to pollute the code
>> with all these 'ifdef RTE_NET_BOND'.
>> I suppose this logic (for checking is bonding API present or not)
>> can be hidden inside change_bonding_slave_port_status() itself.
>>
> I think it does not pollute the code. anyone can tell according to
> the flag 'ifdef RTE_NET_BOND'.
> if hiddle inside 'change_bonding_slave_port_status', it will be weird.
> For example, if the port is not bonding port, It will also invoke 
> 'change_bonding_slave_port_status'. That is unreasonable.
> 

Hi Konstantin,

I also was not happy to have bonding (or any PMD) ifdef in the generic 
(start()/stop()) functions, but the ifdef blocks updates testpmd 
(application) level status. So that can't be handled in the PMD and need 
to be in the application level.
Which is enforcing to have same PMD specific code in the testpmd level,
if you have any suggestion to prevent this, I am for it.

I will proceed with first two patch of this set, which fixes bonding PMD 
issues, I will hold the testpmd ones for more comments.

Thanks,
ferruh

> 
>>
>>> +        struct rte_port *port = &ports[port_id];
>>> +
>>> +        /*
>>> +         * Stopping a bonded port also stops all slaves under the 
>>> bonded
>>> +         * device. So if this port is bond device, we need to modify 
>>> the
>>> +         * port status of these slaves.
>>> +         */
>>> +        if (port->bond_flag == 1)
>>> +            return change_bonding_slave_port_status(port_id, true);
>>> +#endif
>>> +    }
>>>       return 0;
>>>   }
>>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>>> index 31f766c965..67f253b30e 100644
>>> --- a/app/test-pmd/testpmd.h
>>> +++ b/app/test-pmd/testpmd.h
>>> @@ -266,7 +266,8 @@ struct rte_port {
>>>       uint32_t                mc_addr_nb; /**< nb. of addr. in 
>>> mc_addr_pool */
>>>       queueid_t               queue_nb; /**< nb. of queues for flow 
>>> rules */
>>>       uint32_t                queue_sz; /**< size of a queue for flow 
>>> rules */
>>> -    uint8_t                 slave_flag; /**< bonding slave port */
>>> +    uint8_t                 slave_flag : 1, /**< bonding slave port */
>>> +                bond_flag : 1; /**< port is bond device */
>>>       struct port_template    *pattern_templ_list; /**< Pattern 
>>> templates. */
>>>       struct port_template    *actions_templ_list; /**< Actions 
>>> templates. */
>>>       struct port_table       *table_list; /**< Flow tables. */
>>
>> .


  parent reply	other threads:[~2022-05-10 16:34 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20211025063922.34066-1-humin29@huawei.com>
     [not found] ` <20220324030036.4761-1-humin29@huawei.com>
2022-03-24  3:00   ` [PATCH V2 1/4] net/bonding: fix non-active slaves aren't stopped Min Hu (Connor)
2022-04-26 18:19     ` Ferruh Yigit
2022-04-29  6:45       ` Min Hu (Connor)
2022-04-29 13:31         ` Ferruh Yigit
2022-05-03  6:54           ` Min Hu (Connor)
2022-05-03 19:04             ` Ferruh Yigit
2022-05-05  1:16               ` Min Hu (Connor)
2022-03-24  3:00   ` [PATCH V2 2/4] net/bonding: fix non-terminable while loop Min Hu (Connor)
2022-04-26 18:19     ` Ferruh Yigit
2022-04-29  6:52       ` Min Hu (Connor)
2022-04-29 13:35         ` Ferruh Yigit
2022-03-24  3:00   ` [PATCH V2 3/4] app/testpmd: fix port status of slave device Min Hu (Connor)
2022-03-24  3:00   ` [PATCH V2 4/4] app/testpmd: fix slave device isn't released Min Hu (Connor)
2022-05-30  6:01     ` Min Hu (Connor)
2022-05-30 10:21       ` Singh, Aman Deep
     [not found]   ` <20220503100217.46203-1-humin29@huawei.com>
2022-05-03 10:02     ` [PATCH v3 1/5] net/bonding: fix non-active slaves aren't stopped Min Hu (Connor)
2022-05-03 10:02     ` [PATCH v3 2/5] net/bonding: fix non-terminable while loop Min Hu (Connor)
2022-05-03 10:02     ` [PATCH v3 3/5] app/testpmd: fix port status of slave device Min Hu (Connor)
2022-05-03 23:39       ` Konstantin Ananyev
2022-05-06  8:16         ` Min Hu (Connor)
2022-05-08 11:28           ` Konstantin Ananyev
2022-05-10 16:34           ` Ferruh Yigit [this message]
2022-05-10 21:48             ` Konstantin Ananyev
2022-05-11  2:16               ` Min Hu (Connor)
2022-05-11 10:05                 ` Ferruh Yigit
2022-05-11  2:14       ` [PATCH v4] " Min Hu (Connor)
2022-05-11 22:08         ` Konstantin Ananyev
2022-05-19  7:15           ` Andrew Rybchenko
2022-05-03 10:02     ` [PATCH v3 4/5] app/testpmd: fix slave device isn't released Min Hu (Connor)
2022-06-01 17:54       ` Ferruh Yigit
2022-06-07  8:15         ` Dongdong Liu
2022-06-07  8:10       ` [PATCH v4] " Dongdong Liu
2022-06-07 14:31         ` Ferruh Yigit
2022-06-09  7:50           ` Dongdong Liu
2022-06-09  8:50             ` Ferruh Yigit
2022-06-09 11:20               ` Dongdong Liu
2022-06-09 11:49       ` [PATCH v5] " Dongdong Liu
2022-06-10  8:10         ` Ferruh Yigit
2022-05-03 10:02     ` [PATCH v3 5/5] ethdev: fix dev state when stop Min Hu (Connor)
2022-05-25 17:44       ` Ferruh Yigit
2022-05-26 10:21         ` Thomas Monjalon
2022-05-30 12:04           ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bbc13ccc-cb79-3325-8bb5-377e4320cd38@xilinx.com \
    --to=ferruh.yigit@xilinx.com \
    --cc=aman.deep.singh@intel.com \
    --cc=bernard.iremonger@intel.com \
    --cc=dev@dpdk.org \
    --cc=humin29@huawei.com \
    --cc=konstantin.v.ananyev@yandex.ru \
    --cc=lihuisong@huawei.com \
    --cc=pablo.de.lara.guarch@intel.com \
    --cc=stable@dpdk.org \
    --cc=xiaoyun.li@intel.com \
    --cc=yuying.zhang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

patches for DPDK stable branches

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ http://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git