From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id B2CC523B for ; Wed, 28 Mar 2018 18:42:02 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Mar 2018 09:42:01 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,372,1517904000"; d="scan'208";a="27755357" Received: from tanjianf-mobl.ccr.corp.intel.com (HELO [10.255.29.250]) ([10.255.29.250]) by fmsmga007.fm.intel.com with ESMTP; 28 Mar 2018 09:41:58 -0700 To: Jeff Guo , stephen@networkplumber.org, bruce.richardson@intel.com, ferruh.yigit@intel.com, konstantin.ananyev@intel.com, gaetan.rivet@6wind.com, jingjing.wu@intel.com, thomas@monjalon.net, motih@mellanox.com, harry.van.haaren@intel.com References: <1521610066-12966-3-git-send-email-jia.guo@intel.com> <1522063256-3997-1-git-send-email-jia.guo@intel.com> <1522063256-3997-5-git-send-email-jia.guo@intel.com> Cc: jblunck@infradead.org, shreyansh.jain@nxp.com, dev@dpdk.org, helin.zhang@intel.com From: "Tan, Jianfeng" Message-ID: Date: Thu, 29 Mar 2018 00:41:58 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1522063256-3997-5-git-send-email-jia.guo@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH V16 4/4] app/testpmd: enable device hotplug monitoring X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Mar 2018 16:42:03 -0000 On 3/26/2018 7:20 PM, Jeff Guo wrote: > Use testpmd for example, to show an application how to use device event > mechanism to monitor the hotplug event, involve both hot removal event involve -> including > and the hot insertion event. > > The process is that, testpmd first enable hotplug monitoring and register > the user's callback, when device being hotplug insertion or hotplug > removal, the eal monitor the event and call user's callbacks, the > application according their hot plug policy to detach or attach the device > from the bus. You are not exactly describing what's done in this example. From what I see, you only implement hot-unplug. For hot-plug, we will need to register callback with dev_name of NULL. And without the other patch series, this only works without starting datapath. > > Signed-off-by: Jeff Guo > --- > 1.modify log and patch description. > --- > app/test-pmd/parameters.c | 5 +- > app/test-pmd/testpmd.c | 195 +++++++++++++++++++++++++++++++++++++++++++++- > app/test-pmd/testpmd.h | 11 +++ > 3 files changed, 209 insertions(+), 2 deletions(-) > > diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c > index 97d22b8..825d602 100644 > --- a/app/test-pmd/parameters.c > +++ b/app/test-pmd/parameters.c > @@ -186,6 +186,7 @@ usage(char* progname) > printf(" --flow-isolate-all: " > "requests flow API isolated mode on all ports at initialization time.\n"); > printf(" --tx-offloads=0xXXXXXXXX: hexadecimal bitmask of TX queue offloads\n"); > + printf(" --hot-plug: enalbe hot plug for device.\n"); > } > > #ifdef RTE_LIBRTE_CMDLINE > @@ -621,6 +622,7 @@ launch_args_parse(int argc, char** argv) > { "print-event", 1, 0, 0 }, > { "mask-event", 1, 0, 0 }, > { "tx-offloads", 1, 0, 0 }, > + { "hot-plug", 0, 0, 0 }, > { 0, 0, 0, 0 }, > }; > > @@ -1102,7 +1104,8 @@ launch_args_parse(int argc, char** argv) > rte_exit(EXIT_FAILURE, > "invalid mask-event argument\n"); > } > - > + if (!strcmp(lgopts[opt_idx].name, "hot-plug")) > + hot_plug = 1; > break; > case 'h': > usage(argv[0]); > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c > index 4c0e258..bb1ac8f 100644 > --- a/app/test-pmd/testpmd.c > +++ b/app/test-pmd/testpmd.c > @@ -12,6 +12,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -284,6 +285,9 @@ uint8_t lsc_interrupt = 1; /* enabled by default */ > */ > uint8_t rmv_interrupt = 1; /* enabled by default */ > > + > +uint8_t hot_plug = 0; /**< hotplug disabled by default. */ > + > /* > * Display or mask ether events > * Default to all events except VF_MBOX > @@ -384,6 +388,8 @@ uint8_t bitrate_enabled; > struct gro_status gro_ports[RTE_MAX_ETHPORTS]; > uint8_t gro_flush_cycles = GRO_DEFAULT_FLUSH_CYCLES; > > +static struct hotplug_request_list hp_list; > + > /* Forward function declarations */ > static void map_port_queue_stats_mapping_registers(portid_t pi, > struct rte_port *port); > @@ -391,6 +397,14 @@ static void check_all_ports_link_status(uint32_t port_mask); > static int eth_event_callback(portid_t port_id, > enum rte_eth_event_type type, > void *param, void *ret_param); > +static int eth_dev_event_callback(char *device_name, > + enum rte_dev_event_type type, > + void *param); > +static int eth_dev_event_callback_register(portid_t port_id); > +static bool in_hotplug_list(const char *dev_name); > + > +static int hotplug_list_add(struct rte_device *device, > + enum rte_kernel_driver device_kdrv); > > /* > * Check if all the ports are started. > @@ -1853,6 +1867,27 @@ reset_port(portid_t pid) > printf("Done\n"); > } > > +static int > +eth_dev_event_callback_register(portid_t port_id) > +{ > + int diag; > + char device_name[128]; > + > + snprintf(device_name, sizeof(device_name), > + "%s", rte_eth_devices[port_id].device->name); > + > + /* register the dev_event callback */ > + > + diag = rte_dev_callback_register(device_name, > + eth_dev_event_callback, (void *)(intptr_t)port_id); > + if (diag) { > + printf("Failed to setup dev_event callback\n"); > + return -1; > + } > + > + return 0; > +} > + > void > attach_port(char *identifier) > { > @@ -1869,6 +1904,8 @@ attach_port(char *identifier) > if (rte_eth_dev_attach(identifier, &pi)) > return; > > + eth_dev_event_callback_register(pi); What's the difference with below one? > + > socket_id = (unsigned)rte_eth_dev_socket_id(pi); > /* if socket_id is invalid, set to 0 */ > if (check_socket_id(socket_id) < 0) > @@ -1880,6 +1917,12 @@ attach_port(char *identifier) > > ports[pi].port_status = RTE_PORT_STOPPED; > > + if (hot_plug) { > + hotplug_list_add(rte_eth_devices[pi].device, > + rte_eth_devices[pi].data->kdrv); > + eth_dev_event_callback_register(pi); > + } > + > printf("Port %d is attached. Now total ports is %d\n", pi, nb_ports); > printf("Done\n"); > } > @@ -1906,6 +1949,12 @@ detach_port(portid_t port_id) > > nb_ports = rte_eth_dev_count(); > > + if (hot_plug) { > + hotplug_list_add(rte_eth_devices[port_id].device, > + rte_eth_devices[port_id].data->kdrv); > + eth_dev_event_callback_register(port_id); > + } > + > printf("Port '%s' is detached. Now total ports is %d\n", > name, nb_ports); > printf("Done\n"); > @@ -1929,6 +1978,9 @@ pmd_test_exit(void) > close_port(pt_id); > } > } > + > + rte_dev_event_monitor_stop(); > + > printf("\nBye...\n"); > } > > @@ -2013,6 +2065,49 @@ rmv_event_callback(void *arg) > dev->device->name); > } > > +static void > +rmv_dev_event_callback(void *arg) > +{ > + char name[RTE_ETH_NAME_MAX_LEN]; > + uint8_t port_id = (intptr_t)arg; > + > + rte_eal_alarm_cancel(rmv_dev_event_callback, arg); > + > + RTE_ETH_VALID_PORTID_OR_RET(port_id); > + printf("removing port id:%u\n", port_id); > + > + if (!in_hotplug_list(rte_eth_devices[port_id].device->name)) > + return; > + > + stop_packet_forwarding(); > + > + stop_port(port_id); > + close_port(port_id); > + if (rte_eth_dev_detach(port_id, name)) { > + RTE_LOG(ERR, USER1, "Failed to detach port '%s'\n", name); > + return; > + } > + > + nb_ports = rte_eth_dev_count(); > + > + printf("Port '%s' is detached. Now total ports is %d\n", > + name, nb_ports); > +} > + > +static void > +add_dev_event_callback(void *arg) > +{ > + char *dev_name = (char *)arg; > + > + rte_eal_alarm_cancel(add_dev_event_callback, arg); > + > + if (!in_hotplug_list(dev_name)) > + return; > + > + RTE_LOG(ERR, EAL, "add device: %s\n", dev_name); > + attach_port(dev_name); > +} > + > /* This function is used by the interrupt thread */ > static int > eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param, > @@ -2059,6 +2154,86 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param, > return 0; > } > > +static bool > +in_hotplug_list(const char *dev_name) > +{ > + struct hotplug_request *hp_request = NULL; > + > + TAILQ_FOREACH(hp_request, &hp_list, next) { > + if (!strcmp(hp_request->dev_name, dev_name)) > + break; > + } > + > + if (hp_request) > + return true; > + > + return false; > +} > + > +static int > +hotplug_list_add(struct rte_device *device, enum rte_kernel_driver device_kdrv) > +{ > + struct hotplug_request *hp_request; > + > + hp_request = rte_zmalloc("hoplug request", > + sizeof(*hp_request), 0); > + if (hp_request == NULL) { > + fprintf(stderr, "%s can not alloc memory\n", > + __func__); > + return -ENOMEM; > + } > + > + hp_request->dev_name = device->name; > + hp_request->dev_kdrv = device_kdrv; > + > + TAILQ_INSERT_TAIL(&hp_list, hp_request, next); > + > + return 0; > +} > + > +/* This function is used by the interrupt thread */ > +static int > +eth_dev_event_callback(char *device_name, enum rte_dev_event_type type, > + void *arg) > +{ > + static const char * const event_desc[] = { > + [RTE_DEV_EVENT_UNKNOWN] = "Unknown", > + [RTE_DEV_EVENT_ADD] = "add", > + [RTE_DEV_EVENT_REMOVE] = "remove", > + }; > + char *dev_name = malloc(strlen(device_name) + 1); > + > + strcpy(dev_name, device_name); strdup is easier? > + > + if (type >= RTE_DEV_EVENT_MAX) { > + fprintf(stderr, "%s called upon invalid event %d\n", > + __func__, type); > + fflush(stderr); > + } else if (event_print_mask & (UINT32_C(1) << type)) { > + printf("%s event\n", > + event_desc[type]); > + fflush(stdout); > + } > + > + switch (type) { > + case RTE_DEV_EVENT_REMOVE: > + if (rte_eal_alarm_set(100000, > + rmv_dev_event_callback, arg)) Why not rmv_dev_event_callback directly? Besides, the name of rmv_dev_event_callback is really confusing. It's not a dev event callback. > + fprintf(stderr, > + "Could not set up deferred device removal\n"); > + break; > + case RTE_DEV_EVENT_ADD: > + if (rte_eal_alarm_set(100000, > + add_dev_event_callback, dev_name)) > + fprintf(stderr, > + "Could not set up deferred device add\n"); Ditto. > + break; > + default: > + break; > + } > + return 0; > +} > + > static int > set_tx_queue_stats_mapping_registers(portid_t port_id, struct rte_port *port) > { > @@ -2474,8 +2649,9 @@ signal_handler(int signum) > int > main(int argc, char** argv) > { > - int diag; > + int diag; > portid_t port_id; > + int ret; > > signal(SIGINT, signal_handler); > signal(SIGTERM, signal_handler); > @@ -2543,6 +2719,23 @@ main(int argc, char** argv) > nb_rxq, nb_txq); > > init_config(); > + > + if (hot_plug) { > + /* enable hot plug monitoring */ > + ret = rte_dev_event_monitor_start(); > + if (ret) { > + rte_errno = EINVAL; > + return -1; > + } > + if (TAILQ_EMPTY(&hp_list)) > + TAILQ_INIT(&hp_list); > + RTE_ETH_FOREACH_DEV(port_id) { > + hotplug_list_add(rte_eth_devices[port_id].device, > + rte_eth_devices[port_id].data->kdrv); > + eth_dev_event_callback_register(port_id); > + } Why not monitoring all devices with dev_name of NULL? It makes things much easier. And we can also monitor the hot-plug event. > + } > + > if (start_port(RTE_PORT_ALL) != 0) > rte_exit(EXIT_FAILURE, "Start ports failed\n"); > > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h > index 153abea..c619e11 100644 > --- a/app/test-pmd/testpmd.h > +++ b/app/test-pmd/testpmd.h > @@ -63,6 +63,15 @@ typedef uint16_t streamid_t; > #define TM_MODE 0 > #endif > > +struct hotplug_request { > + TAILQ_ENTRY(hotplug_request) next; /**< Callbacks list */ > + const char *dev_name; /* request device name */ > + enum rte_kernel_driver dev_kdrv; /* kernel driver binded */ > +}; > + > +/** @internal Structure to keep track of registered callbacks */ > +TAILQ_HEAD(hotplug_request_list, hotplug_request); > + > enum { > PORT_TOPOLOGY_PAIRED, > PORT_TOPOLOGY_CHAINED, > @@ -319,6 +328,8 @@ extern volatile int test_done; /* stop packet forwarding when set to 1. */ > extern uint8_t lsc_interrupt; /**< disabled by "--no-lsc-interrupt" parameter */ > extern uint8_t rmv_interrupt; /**< disabled by "--no-rmv-interrupt" parameter */ > extern uint32_t event_print_mask; > +extern uint8_t hot_plug; /**< enable by "--hot-plug" parameter */ > + > /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */ > > #ifdef RTE_LIBRTE_IXGBE_BYPASS