From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 91A3F6CBB for ; Tue, 8 May 2018 17:19:27 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 May 2018 08:19:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,378,1520924400"; d="scan'208";a="40198193" Received: from irsmsx152.ger.corp.intel.com ([163.33.192.66]) by orsmga006.jf.intel.com with ESMTP; 08 May 2018 08:19:23 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.214]) by IRSMSX152.ger.corp.intel.com ([169.254.6.199]) with mapi id 14.03.0319.002; Tue, 8 May 2018 16:19:23 +0100 From: "Ananyev, Konstantin" To: "Guo, Jia" , "stephen@networkplumber.org" , "Richardson, Bruce" , "Yigit, Ferruh" , "gaetan.rivet@6wind.com" , "Wu, Jingjing" , "thomas@monjalon.net" , "motih@mellanox.com" , "matan@mellanox.com" , "Van Haaren, Harry" , "Tan, Jianfeng" CC: "jblunck@infradead.org" , "shreyansh.jain@nxp.com" , "dev@dpdk.org" , "Zhang, Helin" Thread-Topic: [PATCH V21 2/4] eal: add failure handle mechanism for hot plug Thread-Index: AQHT4syJCEBEclxmNk6aelBVPgf1/aQfukwAgAYpEoCAABZBsA== Date: Tue, 8 May 2018 15:19:22 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258AEDC3CE6@irsmsx105.ger.corp.intel.com> References: <1498711073-42917-1-git-send-email-jia.guo@intel.com> <1525344524-26946-1-git-send-email-jia.guo@intel.com> <1525344524-26946-3-git-send-email-jia.guo@intel.com> <2601191342CEEE43887BDE71AB977258AEDC28C9@irsmsx105.ger.corp.intel.com> <6419dd13-e038-11bd-0bdc-beef4467bde9@intel.com> In-Reply-To: <6419dd13-e038-11bd-0bdc-beef4467bde9@intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMDUxYjA0NzUtMmM2Yi00N2RkLThjMzUtMjUxODRiOWEyZGM0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IjNucHo0UjdLZVwvRGJqNjdxRjU3Tnlmb0V3aFQzWWZOZHJYdFQ0MnBkeWgwPSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH V21 2/4] eal: add failure handle mechanism for hot plug 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: Tue, 08 May 2018 15:19:28 -0000 > -----Original Message----- > From: Guo, Jia > Sent: Tuesday, May 8, 2018 3:57 PM > To: Ananyev, Konstantin ; stephen@networkpl= umber.org; Richardson, Bruce > ; Yigit, Ferruh ; gae= tan.rivet@6wind.com; Wu, Jingjing > ; thomas@monjalon.net; motih@mellanox.com; matan@m= ellanox.com; Van Haaren, Harry > ; Tan, Jianfeng > Cc: jblunck@infradead.org; shreyansh.jain@nxp.com; dev@dpdk.org; Zhang, H= elin > Subject: Re: [PATCH V21 2/4] eal: add failure handle mechanism for hot pl= ug >=20 >=20 >=20 > On 5/4/2018 11:56 PM, Ananyev, Konstantin wrote: > > Hi Jeff, > > > >> This patch introduces a failure handler mechanism to handle device > >> hot unplug event. When device be hot plug out, the device resource > >> become invalid, if this resource is still be unexpected read/write, > >> system will crash. This patch let eal help application to handle > >> this fault, when sigbus error occur, check the failure address and > >> accordingly remap the invalid memory for the corresponding device, > >> that could guaranty the application not to be shut down when hot plug. > >> > >> Signed-off-by: Jeff Guo > >> --- > >> v21->v20: > >> sync failure hanlde to fix multiple process issue > >> --- > >> lib/librte_eal/linuxapp/eal/eal_dev.c | 154 ++++++++++++++++++++++++= +++++++++- > >> 1 file changed, 153 insertions(+), 1 deletion(-) > >> > >> diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c b/lib/librte_eal/li= nuxapp/eal/eal_dev.c > >> index 1cf6aeb..3067f39 100644 > >> --- a/lib/librte_eal/linuxapp/eal/eal_dev.c > >> +++ b/lib/librte_eal/linuxapp/eal/eal_dev.c > >> @@ -4,6 +4,8 @@ > >> > >> #include > >> #include > >> +#include > >> +#include > >> #include > >> #include > >> > >> @@ -14,15 +16,27 @@ > >> #include > >> #include > >> #include > >> +#include > >> +#include > >> +#include > >> > >> #include "eal_private.h" > >> > >> static struct rte_intr_handle intr_handle =3D {.fd =3D -1 }; > >> static bool monitor_started; > >> > >> +extern struct rte_bus_list rte_bus_list; > >> + > >> #define EAL_UEV_MSG_LEN 4096 > >> #define EAL_UEV_MSG_ELEM_LEN 128 > >> > >> +/* spinlock for device failure process */ > >> +static rte_spinlock_t dev_failure_lock =3D RTE_SPINLOCK_INITIALIZER; > >> + > >> +static struct sigaction sigbus_action_old; > >> + > >> +static int sigbus_need_recover; > >> + > >> static void dev_uev_handler(__rte_unused void *param); > >> > >> /* identify the system layer which reports this event. */ > >> @@ -34,6 +48,93 @@ enum eal_dev_event_subsystem { > >> }; > >> > >> static int > >> +dev_uev_failure_process(struct rte_device *dev, void *dev_addr) > >> +{ > >> + struct rte_bus *bus; > >> + int ret =3D 0; > >> + > >> + if (!dev && !dev_addr) { > >> + return -EINVAL; > >> + } else if (dev) { > >> + bus =3D rte_bus_find_by_device_name(dev->name); > >> + if (bus->handle_hot_unplug) { > >> + /** > >> + * call bus ops to handle hot unplug. > >> + */ > >> + ret =3D bus->handle_hot_unplug(dev, dev_addr); > >> + if (ret) { > >> + RTE_LOG(ERR, EAL, > >> + "Cannot handle hot unplug " > >> + "for device %s " > >> + "on the bus %s.\n ", > >> + dev->name, bus->name); > >> + } > >> + } else { > >> + RTE_LOG(ERR, EAL, > >> + "Not support handle hot unplug for bus %s!\n", > >> + bus->name); > >> + ret =3D -ENOTSUP; > >> + } > >> + } else { > >> + TAILQ_FOREACH(bus, &rte_bus_list, next) { > >> + if (bus->handle_hot_unplug) { > >> + /** > >> + * call bus ops to handle hot unplug. > >> + */ > >> + ret =3D bus->handle_hot_unplug(dev, dev_addr); > >> + if (ret) > >> + RTE_LOG(ERR, EAL, > >> + "Cannot handle hot unplug " > >> + "for the device " > >> + "on the bus %s!\n", bus->name); > >> + else > >> + break; > >> + } else { > >> + RTE_LOG(ERR, EAL, > >> + "Not support handle hot unplug " > >> + "for bus %s!\n", bus->name); > >> + ret =3D -ENOTSUP; > >> + } > >> + } > >> + } > >> + return ret; > >> +} > >> + > >> +static void > >> +sigbus_action_recover(void) > >> +{ > >> + if (sigbus_need_recover) { > >> + sigaction(SIGBUS, &sigbus_action_old, NULL); > >> + sigbus_need_recover =3D 0; > >> + } > >> +} > >> + > >> +static void sigbus_handler(int signum __rte_unused, siginfo_t *info, > >> + void *ctx __rte_unused) > >> +{ > >> + int ret; > >> + > >> + RTE_LOG(DEBUG, EAL, "Thread[%d] catch SIGBUS, fault address:%p\n", > >> + (int)pthread_self(), info->si_addr); > >> + rte_spinlock_lock(&dev_failure_lock); > >> + ret =3D dev_uev_failure_process(NULL, info->si_addr); > >> + rte_spinlock_unlock(&dev_failure_lock); > >> + if (!ret) > >> + RTE_LOG(DEBUG, EAL, > >> + "Success to handle SIGBUS error for hot unplug!\n"); > >> + else > >> + rte_exit(EXIT_FAILURE, "exit for SIGBUS error!"); > > I still think we have to distinguish here 2 cases: > > 1) failure addr is not belong to any dpdk devices > > 2) failure addr does belong to dpdk device, but we fail to remap it. > > > > For 1) we probably need to call previous sigbus handler. > > For 2) we probably can only do exit(). >=20 > i think the previous sigbus handler is just a exception of sigbus error > and exit out of the process, so i think should use one way to handler > 1)+2) should be fine, do you agree with that? or you could find any > chance to > call any other sigbus handler at this positoin? I think application can have its own sigbus handler installed (same as we d= o). > >> +} > >> + > >> +static int cmp_dev_name(const struct rte_device *dev, > >> + const void *_name) > >> +{ > >> + const char *name =3D _name; > >> + > >> + return strcmp(dev->name, name); > >> +} > >> + > >> +static int > >> dev_uev_socket_fd_create(void) > >> { > >> struct sockaddr_nl addr; > >> @@ -147,6 +248,9 @@ dev_uev_handler(__rte_unused void *param) > >> struct rte_dev_event uevent; > >> int ret; > >> char buf[EAL_UEV_MSG_LEN]; > >> + struct rte_bus *bus; > >> + struct rte_device *dev; > >> + const char *busname; > >> > >> memset(&uevent, 0, sizeof(struct rte_dev_event)); > >> memset(buf, 0, EAL_UEV_MSG_LEN); > >> @@ -171,13 +275,50 @@ dev_uev_handler(__rte_unused void *param) > >> RTE_LOG(DEBUG, EAL, "receive uevent(name:%s, type:%d, subsystem:%d)= \n", > >> uevent.devname, uevent.type, uevent.subsystem); > >> > >> - if (uevent.devname) > >> + switch (uevent.subsystem) { > >> + case EAL_DEV_EVENT_SUBSYSTEM_PCI: > >> + case EAL_DEV_EVENT_SUBSYSTEM_UIO: > >> + busname =3D "pci"; > >> + break; > >> + default: > >> + break; > >> + } > >> + > >> + if (uevent.devname) { > >> + if (uevent.type =3D=3D RTE_DEV_EVENT_REMOVE) { > >> + bus =3D rte_bus_find_by_name(busname); > >> + if (bus =3D=3D NULL) { > >> + RTE_LOG(ERR, EAL, "Cannot find bus (%s)\n", > >> + uevent.devname); > >> + return; > >> + } > >> + dev =3D bus->find_device(NULL, cmp_dev_name, > >> + uevent.devname); > >> + if (dev =3D=3D NULL) { > >> + RTE_LOG(ERR, EAL, > >> + "Cannot find unplugged device (%s)\n", > >> + uevent.devname); > >> + return; > >> + } > >> + rte_spinlock_lock(&dev_failure_lock); > >> + ret =3D dev_uev_failure_process(dev, NULL); > >> + rte_spinlock_unlock(&dev_failure_lock); > > That's interrupt thread, right? > > I wonder could it happen that user will call device_detach() at the sam= e moment? > > Konstantin >=20 > it is in interrupt thread, and user will call device_detach after failure= process, you concern about twice or more device detach? i > don't think is there any problem here. Ok, but user can call device_detach() on his own, without waiting for failu= re to happen, right? >=20 > >> + if (ret) { > >> + RTE_LOG(ERR, EAL, "Driver cannot remap the " > >> + "device (%s)\n", > >> + dev->name); > >> + return; > >> + } > >> + } > >> dev_callback_process(uevent.devname, uevent.type); > >> + } > >> } > >> > >> int __rte_experimental > >> rte_dev_event_monitor_start(void) > >> { > >> + sigset_t mask; > >> + struct sigaction action; > >> int ret; > >> > >> if (monitor_started) > >> @@ -197,6 +338,14 @@ rte_dev_event_monitor_start(void) > >> return -1; > >> } > >> > >> + /* register sigbus handler */ > >> + sigemptyset(&mask); > >> + sigaddset(&mask, SIGBUS); > >> + action.sa_flags =3D SA_SIGINFO; > >> + action.sa_mask =3D mask; > >> + action.sa_sigaction =3D sigbus_handler; > >> + sigbus_need_recover =3D !sigaction(SIGBUS, &action, &sigbus_action_o= ld); > >> + > >> monitor_started =3D true; > >> > >> return 0; > >> @@ -217,8 +366,11 @@ rte_dev_event_monitor_stop(void) > >> return ret; > >> } > >> > >> + sigbus_action_recover(); > >> + > >> close(intr_handle.fd); > >> intr_handle.fd =3D -1; > >> monitor_started =3D false; > >> + > >> return 0; > >> } > >> -- > >> 2.7.4