From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-f172.google.com (mail-pd0-f172.google.com [209.85.192.172]) by dpdk.org (Postfix) with ESMTP id 2A437B3E8 for ; Thu, 5 Feb 2015 02:38:03 +0100 (CET) Received: by pdbnh10 with SMTP id nh10so1763210pdb.12 for ; Wed, 04 Feb 2015 17:38:02 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :subject:references:in-reply-to:content-type :content-transfer-encoding; bh=iSUSAhXuOv4IIqqSh0T6SRf9B8HUVeGP4U3GmyTEmP8=; b=Vv6tG+HHbsn879xqjvHFM3iKxKxsMVnhAOVKX81EbrxU/zA1Pgs6lakav10WS8Oo9+ sKPXMunNNnWc10SSEOzAIfECxG/9zzku52IauMc4gGO4L24lbT+y8k5S323MLPMHVRiR vKA+lNgvwkJtGMFIYAl59pt6Hy8Q/ENkuFSFCgoFkAqQoK3bE6d4iaP8hIV6V7ZJxxZz gfqDmr2GTUfWazh4ydw5TD5MggvX7FRFHnb9N827h600nHxuHVw50iuYK86/3OsbXbXv Wuy7WLpxADNGXKDOeYCMcn2W3P2v5b7B6WxR6655XeIIFlk9vxYVkNKNVQ38K9J1hSa4 Q5Lw== X-Gm-Message-State: ALoCoQmCpocq5/KX5kOnVJQdZX+WfSiOlCoDWw77sGOX4iKJ6dlPLhQJ0DzJAV4/o/zurG0UNK+A X-Received: by 10.66.121.197 with SMTP id lm5mr1852121pab.76.1423100282494; Wed, 04 Feb 2015 17:38:02 -0800 (PST) Received: from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132]) by mx.google.com with ESMTPSA id iv1sm3226866pbc.87.2015.02.04.17.38.00 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 04 Feb 2015 17:38:01 -0800 (PST) Message-ID: <54D2C976.6060604@igel.co.jp> Date: Thu, 05 Feb 2015 10:37:58 +0900 From: Tetsuya Mukawa User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: "Qiu, Michael" , "dev@dpdk.org" References: <1421664027-17971-9-git-send-email-mukawa@igel.co.jp> <1422763322-13742-1-git-send-email-mukawa@igel.co.jp> <1422763322-13742-16-git-send-email-mukawa@igel.co.jp> <533710CFB86FA344BFBF2D6802E60286CD3E56@SHSMSX101.ccr.corp.intel.com> <533710CFB86FA344BFBF2D6802E60286CD3F7D@SHSMSX101.ccr.corp.intel.com> <54D0A31C.2030903@igel.co.jp> <533710CFB86FA344BFBF2D6802E60286CD420D@SHSMSX101.ccr.corp.intel.com> In-Reply-To: <533710CFB86FA344BFBF2D6802E60286CD420D@SHSMSX101.ccr.corp.intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6] testpmd: Add port hotplug support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Feb 2015 01:38:03 -0000 On 2015/02/04 10:44, Qiu, Michael wrote: > On 2/3/2015 6:30 PM, Tetsuya Mukawa wrote: >> On 2015/02/03 18:14, Qiu, Michael wrote: >>> On 2/3/2015 2:16 PM, Qiu, Michael wrote: >>>> On 2/1/2015 12:02 PM, Tetsuya Mukawa wrote: >>>>> The patch introduces following commands. >>>>> - port attach [ident] >>>>> - port detach [port_id] >>>>> - attach: attaching a port >>>>> - detach: detaching a port >>>>> - ident: pci address of physical device. >>>>> Or device name and paramerters of virtual device. >>>>> (ex. 0000:02:00.0, eth_pcap0,iface=eth0) >>>>> - port_id: port identifier >>>>> >>>>> v5: >>>>> - Add testpmd documentation. >>>>> (Thanks to Iremonger, Bernard) >>>>> v4: >>>>> - Fix strings of command help. >>>>> >>>>> Signed-off-by: Tetsuya Mukawa >>> [...] >>> >>>>> +static int >>>>> +port_is_closed(portid_t port_id) >>>>> +{ >>>>> + if (port_id_is_invalid(port_id, ENABLED_WARN)) >>>>> + return 0; >>>>> + >>>>> + if (ports[port_id].port_status != RTE_PORT_CLOSED) >>>>> + return 0; >>>>> + >>>>> + return 1; >>>>> +} >>>>> + >>>>> +int >>>>> start_port(portid_t pid) >>>>> { >>>>> int diag, need_check_link_status = 0; >>>>> @@ -1296,8 +1347,8 @@ start_port(portid_t pid) >>>>> >>>>> if(dcb_config) >>>>> dcb_test = 1; >>>>> - for (pi = 0; pi < nb_ports; pi++) { >>>>> - if (pid < nb_ports && pid != pi) >>>>> + FOREACH_PORT(pi, ports) { >>>>> + if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) >>>> Here may it be: >>>> >>>> if (!port_id_is_invalid(pid, DISABLED_WARN) && (pid != pi || pid == RET_PORT_ALL)) >>> Sorry, should be: >>> >>> if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi && pid != (portid_t)RET_PORT_ALL) >>> >>> Otherwise, should check for "RET_PORT_ALL" in function port_id_is_invalid() >> Thanks for comment. I've found 2 issues. >> (I guess the original code has same issue.) > Original code may not have this issue, cause RET_PORT_ALL is always > greater than nb_ports, so "continue" will not exec. The logic may be > right, but it is a little hard to understand. > >> One is that "port_id_is_invalid" should receives "pi" instead of "pid". >> The other is if statement is wrong as you said. >> >> I guess following statement will be good. >> >> if (port_id_is_invalid(pi, DISABLED_WARN) || (pid != pi && pid != >> (portid_t)RTE_PORT_ALL)) > Actually, "port_id_is_invalid(pi, DISABLED_WARN)" could be removed as > "FOREACH_PORT" will find a valid port. Good point! > So it could be: > > if (pid != pi && pid != (portid_t)RTE_PORT_ALL) > > What do you think? I agree with you. I will change like above. Thanks, Tetsuya > Thanks, > Michael >> How about it? >> >> Thanks, >> Tetsuya >> >> >>> Thanks, >>> Michael >>> >>>> Otherwise no port will be start by default. >>>> >>>> >>>> Thanks, >>>> Michael >>>> >>>>> continue; >>>>> >>>>> port = &ports[pi]; >>>>> @@ -1421,7 +1472,7 @@ start_port(portid_t pid) >>>>> } >>>>> >>>>> if (need_check_link_status && !no_link_check) >>>>> - check_all_ports_link_status(nb_ports, RTE_PORT_ALL); >>>>> + check_all_ports_link_status(RTE_PORT_ALL); >>>>> else >>>>> printf("Please stop the ports first\n"); >>>>> >>>>> @@ -1446,8 +1497,8 @@ stop_port(portid_t pid) >>>>> } >>>>> printf("Stopping ports...\n"); >>>>> >>>>> - for (pi = 0; pi < nb_ports; pi++) { >>>>> - if (pid < nb_ports && pid != pi) >>>>> + FOREACH_PORT(pi, ports) { >>>>> + if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) >>>>> continue; >>>>> >>>>> port = &ports[pi]; >>>>> @@ -1463,7 +1514,7 @@ stop_port(portid_t pid) >>>>> need_check_link_status = 1; >>>>> } >>>>> if (need_check_link_status && !no_link_check) >>>>> - check_all_ports_link_status(nb_ports, RTE_PORT_ALL); >>>>> + check_all_ports_link_status(RTE_PORT_ALL); >>>>> >>>>> printf("Done\n"); >>>>> } >>>>> @@ -1481,8 +1532,8 @@ close_port(portid_t pid) >>>>> >>>>> printf("Closing ports...\n"); >>>>> >>>>> - for (pi = 0; pi < nb_ports; pi++) { >>>>> - if (pid < nb_ports && pid != pi) >>>>> + FOREACH_PORT(pi, ports) { >>>>> + if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) >>>>> continue; >>>>> >>>>> port = &ports[pi]; >>>>> @@ -1502,31 +1553,83 @@ close_port(portid_t pid) >>>>> printf("Done\n"); >>>>> } >>>>> >>>>> -int >>>>> -all_ports_stopped(void) >>>>> +void >>>>> +attach_port(char *identifier) >>>>> { >>>>> - portid_t pi; >>>>> - struct rte_port *port; >>>>> + portid_t i, j, pi = 0; >>>>> >>>>> - for (pi = 0; pi < nb_ports; pi++) { >>>>> - port = &ports[pi]; >>>>> - if (port->port_status != RTE_PORT_STOPPED) >>>>> - return 0; >>>>> + printf("Attaching a new port...\n"); >>>>> + >>>>> + if (identifier == NULL) { >>>>> + printf("Invalid parameters are speficied\n"); >>>>> + return; >>>>> } >>>>> >>>>> - return 1; >>>>> + if (test_done == 0) { >>>>> + printf("Please stop forwarding first\n"); >>>>> + return; >>>>> + } >>>>> + >>>>> + if (rte_eal_dev_attach(identifier, &pi)) >>>>> + return; >>>>> + >>>>> + ports[pi].enabled = 1; >>>>> + reconfig(pi, rte_eth_dev_socket_id(pi)); >>>>> + rte_eth_promiscuous_enable(pi); >>>>> + >>>>> + nb_ports = rte_eth_dev_count(); >>>>> + >>>>> + /* set_default_fwd_ports_config(); */ >>>>> + bzero(fwd_ports_ids, sizeof(fwd_ports_ids)); >>>>> + i = 0; >>>>> + FOREACH_PORT(j, ports) { >>>>> + fwd_ports_ids[i] = j; >>>>> + i++; >>>>> + } >>>>> + nb_cfg_ports = nb_ports; >>>>> + nb_fwd_ports++; >>>>> + >>>>> + ports[pi].port_status = RTE_PORT_STOPPED; >>>>> + >>>>> + printf("Port %d is attached. Now total ports is %d\n", pi, nb_ports); >>>>> + printf("Done\n"); >>>>> } >>>>> >>>>> -int >>>>> -port_is_started(portid_t port_id) >>>>> +void >>>>> +detach_port(uint8_t port_id) >>>>> { >>>>> - if (port_id_is_invalid(port_id)) >>>>> - return -1; >>>>> + portid_t i, pi = 0; >>>>> + char name[RTE_ETH_NAME_MAX_LEN]; >>>>> >>>>> - if (ports[port_id].port_status != RTE_PORT_STARTED) >>>>> - return 0; >>>>> + printf("Detaching a port...\n"); >>>>> >>>>> - return 1; >>>>> + if (!port_is_closed(port_id)) { >>>>> + printf("Please close port first\n"); >>>>> + return; >>>>> + } >>>>> + >>>>> + rte_eth_promiscuous_disable(port_id); >>>>> + >>>>> + if (rte_eal_dev_detach(port_id, name)) >>>>> + return; >>>>> + >>>>> + ports[port_id].enabled = 0; >>>>> + nb_ports = rte_eth_dev_count(); >>>>> + >>>>> + /* set_default_fwd_ports_config(); */ >>>>> + bzero(fwd_ports_ids, sizeof(fwd_ports_ids)); >>>>> + i = 0; >>>>> + FOREACH_PORT(pi, ports) { >>>>> + fwd_ports_ids[i] = pi; >>>>> + i++; >>>>> + } >>>>> + nb_cfg_ports = nb_ports; >>>>> + nb_fwd_ports--; >>>>> + >>>>> + printf("Port '%s' is detached. Now total ports is %d\n", >>>>> + name, nb_ports); >>>>> + printf("Done\n"); >>>>> + return; >>>>> } >>>>> >>>>> void >>>>> @@ -1534,7 +1637,7 @@ pmd_test_exit(void) >>>>> { >>>>> portid_t pt_id; >>>>> >>>>> - for (pt_id = 0; pt_id < nb_ports; pt_id++) { >>>>> + FOREACH_PORT(pt_id, ports) { >>>>> printf("Stopping port %d...", pt_id); >>>>> fflush(stdout); >>>>> rte_eth_dev_close(pt_id); >>>>> @@ -1553,7 +1656,7 @@ struct pmd_test_command { >>>>> >>>>> /* Check the link status of all ports in up to 9s, and print them finally */ >>>>> static void >>>>> -check_all_ports_link_status(uint8_t port_num, uint32_t port_mask) >>>>> +check_all_ports_link_status(uint32_t port_mask) >>>>> { >>>>> #define CHECK_INTERVAL 100 /* 100ms */ >>>>> #define MAX_CHECK_TIME 90 /* 9s (90 * 100ms) in total */ >>>>> @@ -1564,7 +1667,7 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask) >>>>> fflush(stdout); >>>>> for (count = 0; count <= MAX_CHECK_TIME; count++) { >>>>> all_ports_up = 1; >>>>> - for (portid = 0; portid < port_num; portid++) { >>>>> + FOREACH_PORT(portid, ports) { >>>>> if ((port_mask & (1 << portid)) == 0) >>>>> continue; >>>>> memset(&link, 0, sizeof(link)); >>>>> @@ -1688,7 +1791,7 @@ init_port_config(void) >>>>> portid_t pid; >>>>> struct rte_port *port; >>>>> >>>>> - for (pid = 0; pid < nb_ports; pid++) { >>>>> + FOREACH_PORT(pid, ports) { >>>>> port = &ports[pid]; >>>>> port->dev_conf.rxmode = rx_mode; >>>>> port->dev_conf.fdir_conf = fdir_conf; >>>>> @@ -1877,7 +1980,7 @@ main(int argc, char** argv) >>>>> >>>>> nb_ports = (portid_t) rte_eth_dev_count(); >>>>> if (nb_ports == 0) >>>>> - rte_exit(EXIT_FAILURE, "No probed ethernet device\n"); >>>>> + RTE_LOG(WARNING, EAL, "No probed ethernet devices\n"); >>>>> >>>>> set_def_fwd_config(); >>>>> if (nb_lcores == 0) >>>>> @@ -1899,7 +2002,7 @@ main(int argc, char** argv) >>>>> rte_exit(EXIT_FAILURE, "Start ports failed\n"); >>>>> >>>>> /* set all ports to promiscuous mode by default */ >>>>> - for (port_id = 0; port_id < nb_ports; port_id++) >>>>> + FOREACH_PORT(port_id, ports) >>>>> rte_eth_promiscuous_enable(port_id); >>>>> >>>>> #ifdef RTE_LIBRTE_CMDLINE >>>>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h >>>>> index 8f5e6c7..109c670 100644 >>>>> --- a/app/test-pmd/testpmd.h >>>>> +++ b/app/test-pmd/testpmd.h >>>>> @@ -134,6 +134,7 @@ struct fwd_stream { >>>>> * The data structure associated with each port. >>>>> */ >>>>> struct rte_port { >>>>> + uint8_t enabled; /**< Port enabled or not */ >>>>> struct rte_eth_dev_info dev_info; /**< PCI info + driver name */ >>>>> struct rte_eth_conf dev_conf; /**< Port configuration. */ >>>>> struct ether_addr eth_addr; /**< Port ethernet address */ >>>>> @@ -159,6 +160,14 @@ struct rte_port { >>>>> struct rte_eth_txconf tx_conf; /**< tx configuration */ >>>>> }; >>>>> >>>>> +extern portid_t __rte_unused >>>>> +find_next_port(portid_t p, struct rte_port *ports, int size); >>>>> + >>>>> +#define FOREACH_PORT(p, ports) \ >>>>> + for (p = find_next_port(0, ports, RTE_MAX_ETHPORTS); \ >>>>> + p < RTE_MAX_ETHPORTS; \ >>>>> + p = find_next_port(p + 1, ports, RTE_MAX_ETHPORTS)) >>>>> + >>>>> /** >>>>> * The data structure associated with each forwarding logical core. >>>>> * The logical cores are internally numbered by a core index from 0 to >>>>> @@ -515,6 +524,8 @@ int init_port_dcb_config(portid_t pid,struct dcb_config *dcb_conf); >>>>> int start_port(portid_t pid); >>>>> void stop_port(portid_t pid); >>>>> void close_port(portid_t pid); >>>>> +void attach_port(char *identifier); >>>>> +void detach_port(uint8_t port_id); >>>>> int all_ports_stopped(void); >>>>> int port_is_started(portid_t port_id); >>>>> void pmd_test_exit(void); >>>>> @@ -558,10 +569,15 @@ void get_ethertype_filter(uint8_t port_id, uint16_t index); >>>>> void get_2tuple_filter(uint8_t port_id, uint16_t index); >>>>> void get_5tuple_filter(uint8_t port_id, uint16_t index); >>>>> void get_flex_filter(uint8_t port_id, uint16_t index); >>>>> -int port_id_is_invalid(portid_t port_id); >>>>> int rx_queue_id_is_invalid(queueid_t rxq_id); >>>>> int tx_queue_id_is_invalid(queueid_t txq_id); >>>>> >>>>> +enum print_warning { >>>>> + ENABLED_WARN = 0, >>>>> + DISABLED_WARN >>>>> +}; >>>>> +int port_id_is_invalid(portid_t port_id, enum print_warning warning); >>>>> + >>>>> /* >>>>> * Work-around of a compilation error with ICC on invocations of the >>>>> * rte_be_to_cpu_16() function. >>>>> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>>>> index 218835a..1cacbcf 100644 >>>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst >>>>> @@ -808,6 +808,63 @@ The following sections show functions for configuring ports. >>>>> >>>>> Port configuration changes only become active when forwarding is started/restarted. >>>>> >>>>> +port attach >>>>> +~~~~~~~~~~~ >>>>> + >>>>> +Attach a port specified by pci address or virtual device args. >>>>> + >>>>> +To attach a new pci device, the device should be recognized by kernel first. >>>>> +Then it should be moved under DPDK management. >>>>> +Finally the port can be attached to testpmd. >>>>> +On the other hand, to attach a port created by virtual device, above steps are not needed. >>>>> + >>>>> +port attach (identifier) >>>>> + >>>>> +For example, to attach a port that pci address is 0000:02:00.0. >>>>> + >>>>> +.. code-block:: console >>>>> + >>>>> + testpmd> port attach 0000:02:00.0 >>>>> + Attaching a new port... >>>>> + ... snip ... >>>>> + Port 0 is attached. Now total ports is 1 >>>>> + Done >>>>> + >>>>> +For example, to attach a port created by pcap PMD. >>>>> + >>>>> +.. code-block:: console >>>>> + >>>>> + testpmd> port attach eth_pcap0,iface=eth0 >>>>> + Attaching a new port... >>>>> + ... snip ... >>>>> + Port 0 is attached. Now total ports is 1 >>>>> + Done >>>>> + >>>>> +In this case, identifier is "eth_pcap0,iface=eth0". >>>>> +This identifier format is the same as "--vdev" format of DPDK applications. >>>>> + >>>>> +port detach >>>>> +~~~~~~~~~~~ >>>>> + >>>>> +Detach a specific port. >>>>> + >>>>> +Before detaching a port, the port should be closed. >>>>> +Also to remove a pci device completely from the system, first detach the port from testpmd. >>>>> +Then the device should be moved under kernel management. >>>>> +Finally the device can be remove using kernel pci hotplug functionality. >>>>> +On the other hand, to remove a port created by virtual device, above steps are not needed. >>>>> + >>>>> +port detach (port_id) >>>>> + >>>>> +For example, to detach a port 0. >>>>> + >>>>> +.. code-block:: console >>>>> + >>>>> + testpmd> port detach 0 >>>>> + Detaching a port... >>>>> + ... snip ... >>>>> + Done >>>>> + >>>>> port start >>>>> ~~~~~~~~~~ >>>>> >>