From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-he1eur01on0050.outbound.protection.outlook.com [104.47.0.50]) by dpdk.org (Postfix) with ESMTP id 6D7AE1B7EF for ; Thu, 12 Apr 2018 07:31:06 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=bvt0sQY/DXfWNgwvsPg8ipd9pt17vdNd1CZIBJF5k+s=; b=nfFI75V7NtePtP0r/JSdGl1fyVmZ5IiaV0f/A55oYe6XqKKKPtl+8OoqhBJaSzsQUuk6UQsMplPbw4K3AcRPJXuVe8aI3IuXtBSXMEH749eVZRgVtEXQuPun4yZR4yGVNtz2N4QelSy0MlLnlf3HlHYsTkfp8Ms+v7GTDvg68M0= Received: from AM4PR0501MB2657.eurprd05.prod.outlook.com (10.172.215.19) by AM4PR0501MB2610.eurprd05.prod.outlook.com (10.172.215.8) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.675.9; Thu, 12 Apr 2018 05:31:03 +0000 Received: from AM4PR0501MB2657.eurprd05.prod.outlook.com ([fe80::6885:c169:afcb:37e6]) by AM4PR0501MB2657.eurprd05.prod.outlook.com ([fe80::6885:c169:afcb:37e6%9]) with mapi id 15.20.0675.011; Thu, 12 Apr 2018 05:31:03 +0000 From: Matan Azrad 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 , Mordechay Haimovsky , "harry.van.haaren@intel.com" , "jianfeng.tan@intel.com" CC: "jblunck@infradead.org" , "shreyansh.jain@nxp.com" , "dev@dpdk.org" , "helin.zhang@intel.com" Thread-Topic: [dpdk-dev] [PATCH V19 4/4] app/testpmd: use auto handle for hotplug Thread-Index: AQHTzZY/MqP6PEfnnkiDxaxrDsq6x6P6Hbdw Date: Thu, 12 Apr 2018 05:31:03 +0000 Message-ID: References: <1522779443-1932-6-git-send-email-jia.guo@intel.com> <1523012217-31146-1-git-send-email-jia.guo@intel.com> <1523012217-31146-5-git-send-email-jia.guo@intel.com> In-Reply-To: <1523012217-31146-5-git-send-email-jia.guo@intel.com> Accept-Language: en-US, he-IL Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=matan@mellanox.com; x-originating-ip: [193.47.165.251] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; AM4PR0501MB2610; 7:8Huw3Im4zm89bKJv4vvADmeXsTa8hKM+z/GCdDxJ91x6rngdTOWW+l6IEtonGDIyfupmha075bFPfauo8B0vUWcR2VxIlw0HDC7eBnZeCV4o1EI3JrqSPqqj4mswa//pM6H9RCAjOeJ1qq+fR4FtuZbjawD4QGFNgTIj7q7kTFqurCDYHD5CrsPl4M/KVq7b76226NlCuTxbI0FmbBUR/bM4/V1P1nkPd1aA91gVYYVlXMNEXGByIVD4346NaB+z x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(4534165)(4627221)(201703031133081)(201702281549075)(48565401081)(5600026)(2017052603328)(7153060)(7193020); SRVR:AM4PR0501MB2610; x-ms-traffictypediagnostic: AM4PR0501MB2610: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(228905959029699); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(8121501046)(5005006)(10201501046)(3002001)(93006095)(93001095)(3231221)(944501327)(52105095)(6055026)(6041310)(20161123558120)(20161123560045)(20161123564045)(20161123562045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(6072148)(201708071742011); SRVR:AM4PR0501MB2610; BCL:0; PCL:0; RULEID:; SRVR:AM4PR0501MB2610; x-forefront-prvs: 06400060E1 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(346002)(396003)(39860400002)(39380400002)(376002)(366004)(53754006)(199004)(189003)(51444003)(106356001)(5890100001)(105586002)(55016002)(5250100002)(6436002)(3280700002)(3660700001)(11346002)(478600001)(33656002)(14454004)(486006)(25786009)(2906002)(59450400001)(2900100001)(446003)(476003)(2501003)(6116002)(97736004)(3846002)(8936002)(4326008)(81156014)(81166006)(316002)(99286004)(6246003)(7416002)(7696005)(5660300001)(66066001)(305945005)(110136005)(7736002)(8676002)(74316002)(54906003)(6506007)(2201001)(26005)(68736007)(86362001)(8656006)(76176011)(229853002)(9686003)(102836004)(186003)(53936002)(921003)(1121003); DIR:OUT; SFP:1101; SCL:1; SRVR:AM4PR0501MB2610; H:AM4PR0501MB2657.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: aSjliPRIEzfqHksaByyt3M4HMmj7Zhj6nym2qgBLnO7sXoQKtiLZki37J6BTh8jm8SMTpKuq90DWAWvkAhclJYzxqIKTGQ/0yyEsreRuduk0ZO/4GjSvttE0v9EEzBcOwAV/mDbjoeRYtRQCGdzCbu5l6oNO6Tw9LczMIerRvyZ1517s5RcM7CYtBdq5ifXA spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Office365-Filtering-Correlation-Id: 01a8aceb-0070-4756-cf3f-08d5a036955d X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 01a8aceb-0070-4756-cf3f-08d5a036955d X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Apr 2018 05:31:03.5202 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR0501MB2610 Subject: Re: [dpdk-dev] [PATCH V19 4/4] app/testpmd: use auto handle for hotplug 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: Thu, 12 Apr 2018 05:31:06 -0000 Hi All From: Jeff Guo, Friday, April 6, 2018 1:57 PM > Use testpmd for example, to show how an application smoothly handle > failure when device be hot removal, and show how to auto bind kernal driv= er > to preparing attach device when device being hot insertion. I saw the kmod of old devices were saved by name and the app hope that new = device name will be as previous saved name. How can the application know which driver should be bind for any new device= ? Moreover, I think that the application should not deal with driver binding\= remap and all this should be managed by the relevant PMDs -=20 Looks like the bind mechanism should be managed directly by the PMDs and no= t annoying application to deal with it. Am I missing something? > Signed-off-by: Jeff Guo > --- > v19->v18: > clean code > --- > app/test-pmd/testpmd.c | 199 > +++++++++++++++++++++++++++++++++++++++++++------ > app/test-pmd/testpmd.h | 9 +++ > 2 files changed, 184 insertions(+), 24 deletions(-) >=20 > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index > d2c122a..d7fa913 100644 > --- a/app/test-pmd/testpmd.c > +++ b/app/test-pmd/testpmd.c > @@ -285,6 +285,8 @@ uint8_t lsc_interrupt =3D 1; /* enabled by default */ > */ > uint8_t rmv_interrupt =3D 1; /* enabled by default */ >=20 > +#define HOT_PLUG_FOR_ALL_DEVICE -1 > +#define ALL_CALLBACK -1 > uint8_t hot_plug =3D 0; /**< hotplug disabled by default. */ >=20 > /* > @@ -387,6 +389,8 @@ uint8_t bitrate_enabled; struct gro_status > gro_ports[RTE_MAX_ETHPORTS]; uint8_t gro_flush_cycles =3D > GRO_DEFAULT_FLUSH_CYCLES; >=20 > +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); > @@ -397,9 +401,13 @@ static int eth_event_callback(portid_t port_id, sta= tic > void eth_dev_event_callback(char *device_name, > enum rte_dev_event_type type, > void *param); > -static int eth_dev_event_callback_register(void); > -static int eth_dev_event_callback_unregister(void); > +static int eth_dev_event_callback_register(portid_t port_id); static > +int eth_dev_event_callback_unregister(portid_t port_id); > + > +static bool in_hotplug_list(const char *dev_name); >=20 > +static int hotplug_list_add(struct rte_device *device, > + enum rte_kernel_driver device_kdrv); >=20 > /* > * Check if all the ports are started. > @@ -1120,11 +1128,17 @@ run_pkt_fwd_on_lcore(struct fwd_lcore *fc, > packet_fwd_t pkt_fwd) > uint64_t tics_datum; > uint64_t tics_current; > uint8_t idx_port, cnt_ports; > + int ret; >=20 > cnt_ports =3D rte_eth_dev_count(); > tics_datum =3D rte_rdtsc(); > tics_per_1sec =3D rte_get_timer_hz(); > #endif > + if (hot_plug) { > + ret =3D rte_dev_handle_hot_unplug(); > + if (ret) > + printf("The device is being hot unplug!\n"); > + } > fsm =3D &fwd_streams[fc->stream_idx]; > nb_fs =3D fc->stream_nb; > do { > @@ -1863,15 +1877,24 @@ reset_port(portid_t pid) } >=20 > static int > -eth_dev_event_callback_register(void) > +eth_dev_event_callback_register(portid_t port_id) > { > int ret; > + char *device_name; >=20 > + /* if port id equal -1, unregister event callbacks for all device. */ unregister =3D> register And I think that HOT_PLUG_FOR_ALL_DEVICE is descriptive enough. > + if (port_id =3D=3D (portid_t)HOT_PLUG_FOR_ALL_DEVICE) { > + device_name =3D NULL; > + } else { > + device_name =3D strdup(rte_eth_devices[port_id].device- > >name); > + if (!device_name) > + return -1; > + } > /* register the device event callback */ > - ret =3D rte_dev_event_callback_register(NULL, > - eth_dev_event_callback, NULL); > + ret =3D rte_dev_event_callback_register(device_name, > + eth_dev_event_callback, (void *)(intptr_t)port_id); > if (ret) { > - printf("Failed to register device event callback\n"); > + printf("Failed to register device event callback.\n"); > return -1; > } You missed device_name freeing. > @@ -1880,15 +1903,25 @@ eth_dev_event_callback_register(void) >=20 >=20 > static int > -eth_dev_event_callback_unregister(void) > +eth_dev_event_callback_unregister(portid_t port_id) > { > int ret; > + char *device_name; > + > + /* if port id equal -1, unregister all device event callbacks */ > + if (port_id =3D=3D (portid_t)HOT_PLUG_FOR_ALL_DEVICE) { > + device_name =3D NULL; > + } else { > + device_name =3D strdup(rte_eth_devices[port_id].device- > >name); > + if (!device_name) > + return -1; > + } >=20 > /* unregister the device event callback */ > - ret =3D rte_dev_event_callback_unregister(NULL, > - eth_dev_event_callback, NULL); > - if (ret < 0) { > - printf("Failed to unregister device event callback\n"); > + ret =3D rte_dev_event_callback_unregister(device_name, > + eth_dev_event_callback, (void *)(intptr_t)port_id); Also here. > + if (ret) { > + printf("Failed to unregister device event callback.\n"); > return -1; > } >=20 > @@ -1911,6 +1944,8 @@ attach_port(char *identifier) > if (rte_eth_dev_attach(identifier, &pi)) > return; >=20 > + eth_dev_event_callback_register(pi); > + > socket_id =3D (unsigned)rte_eth_dev_socket_id(pi); > /* if socket_id is invalid, set to 0 */ > if (check_socket_id(socket_id) < 0) > @@ -1922,6 +1957,12 @@ attach_port(char *identifier) >=20 > ports[pi].port_status =3D RTE_PORT_STOPPED; >=20 > + 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"); > } > @@ -1933,6 +1974,12 @@ detach_port(portid_t port_id) >=20 > printf("Detaching a port...\n"); >=20 > + 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); > + } > + > if (!port_is_closed(port_id)) { > printf("Please close port first\n"); > return; > @@ -1979,7 +2026,7 @@ pmd_test_exit(void) > RTE_LOG(ERR, EAL, > "fail to stop device event monitor."); >=20 > - ret =3D eth_dev_event_callback_unregister(); > + ret =3D eth_dev_event_callback_unregister(ALL_CALLBACK); > if (ret) > RTE_LOG(ERR, EAL, > "fail to unregister all event callbacks."); @@ - > 2068,6 +2115,40 @@ rmv_event_callback(void *arg) > dev->device->name); > } >=20 > +static void > +rmv_dev_event_callback(void *arg) > +{ > + uint8_t port_id =3D (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); > + detach_port(port_id); Did you check the synchronization with the RMV event callback of ethdev?=20 > +} > + > +static void > +add_dev_event_callback(void *arg) > +{ > + char *dev_name =3D (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, @@ -2114,31 +2195,96 @@ eth_event_callback(portid_t port_id, > enum rte_eth_event_type type, void *param, > return 0; > } >=20 > +static bool > +in_hotplug_list(const char *dev_name) > +{ > + struct hotplug_request *hp_request =3D 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 enum rte_kernel_driver > +get_hotplug_driver(const char *dev_name) { > + struct hotplug_request *hp_request =3D NULL; > + > + TAILQ_FOREACH(hp_request, &hp_list, next) { > + if (!strcmp(hp_request->dev_name, dev_name)) > + return hp_request->dev_kdrv; > + } > + return -1; > +} > + > +static int > +hotplug_list_add(struct rte_device *device, enum rte_kernel_driver > +device_kdrv) { > + struct hotplug_request *hp_request; > + > + hp_request =3D rte_zmalloc("hoplug request", > + sizeof(*hp_request), 0); > + if (hp_request =3D=3D NULL) { > + fprintf(stderr, "%s can not alloc memory\n", > + __func__); > + return -ENOMEM; > + } > + > + hp_request->dev_name =3D device->name; > + hp_request->dev_kdrv =3D device_kdrv; > + > + TAILQ_INSERT_TAIL(&hp_list, hp_request, next); > + > + return 0; > +} > + > /* This function is used by the interrupt thread */ static void > eth_dev_event_callback(char *device_name, enum rte_dev_event_type > type, > - __rte_unused void *arg) > + void *arg) > { > + static const char * const event_desc[] =3D { > + [RTE_DEV_EVENT_ADD] =3D "add", > + [RTE_DEV_EVENT_REMOVE] =3D "remove", > + }; > + char *dev_name =3D malloc(strlen(device_name) + 1); > + > + strcpy(dev_name, device_name); > + > if (type >=3D 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); > } > You need to use port_id_is_invalid() to check the port validity. =20 > switch (type) { > case RTE_DEV_EVENT_REMOVE: > - RTE_LOG(ERR, EAL, "The device: %s has been removed!\n", > - device_name); > - /* TODO: After finish failure handle, begin to stop > - * packet forward, stop port, close port, detach port. > - */ > + if (rte_eal_alarm_set(100000, > + rmv_dev_event_callback, arg)) > + fprintf(stderr, "Could not set up deferred " > + "device removal\n"); > break; > case RTE_DEV_EVENT_ADD: > - RTE_LOG(ERR, EAL, "The device: %s has been added!\n", > - device_name); > - /* TODO: After finish kernel driver binding, > - * begin to attach port. > + /** > + * bind the driver to the device > + * before process of hot plug adding device > */ > + rte_dev_bind_kernel_driver(dev_name, > + get_hotplug_driver(dev_name)); > + if (rte_eal_alarm_set(100000, > + add_dev_event_callback, dev_name)) > + fprintf(stderr, "Could not set up deferred " > + "device add\n"); > break; > default: > break; > @@ -2638,8 +2784,13 @@ main(int argc, char** argv) > rte_errno =3D EINVAL; > return -1; > } > - eth_dev_event_callback_register(); > - > + 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); > + } > } >=20 > if (start_port(RTE_PORT_ALL) !=3D 0) > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index > 8fde68d..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 >=20 > +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, > -- > 2.7.4