From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 8C5821B7D4 for ; Wed, 11 Apr 2018 13:34:35 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Apr 2018 04:34:33 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,435,1517904000"; d="scan'208";a="46036512" Received: from jguo15x-mobl3.ccr.corp.intel.com (HELO [10.254.215.165]) ([10.254.215.165]) by fmsmga001.fm.intel.com with ESMTP; 11 Apr 2018 04:34:31 -0700 To: "Ananyev, Konstantin" , "stephen@networkplumber.org" , "Richardson, Bruce" , "Yigit, Ferruh" , "gaetan.rivet@6wind.com" , "Wu, Jingjing" , "thomas@monjalon.net" , "motih@mellanox.com" , "Van Haaren, Harry" , "Tan, Jianfeng" References: <1522779443-1932-6-git-send-email-jia.guo@intel.com> <1523012217-31146-1-git-send-email-jia.guo@intel.com> <1523012217-31146-3-git-send-email-jia.guo@intel.com> <2601191342CEEE43887BDE71AB977258AE91267A@IRSMSX102.ger.corp.intel.com> Cc: "jblunck@infradead.org" , "shreyansh.jain@nxp.com" , "dev@dpdk.org" , "Zhang, Helin" From: "Guo, Jia" Message-ID: <89dddf28-125a-1810-f155-1a476367c0ef@intel.com> Date: Wed, 11 Apr 2018 19:34:30 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <2601191342CEEE43887BDE71AB977258AE91267A@IRSMSX102.ger.corp.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH V19 2/4] eal: add failure handler 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: Wed, 11 Apr 2018 11:34:36 -0000 thanks your review , Konstantin, comment below. On 4/10/2018 1:42 AM, Ananyev, Konstantin wrote: > Hi Jeff, > >> This patch introduces an API (rte_dev_handle_hot_unplug) 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. The api let user register the hot unplug handler, when >> hot plug failure occur, the working thread will be block until the uevent >> mechanism successful recovery the memory and guaranty the application keep >> running smoothly. >> >> Signed-off-by: Jeff Guo >> --- >> v19->18: >> add note for limitation of multiple hotplug >> --- >> doc/guides/rel_notes/release_18_05.rst | 6 ++ >> kernel/linux/igb_uio/igb_uio.c | 4 + >> lib/librte_eal/common/include/rte_dev.h | 19 +++++ >> lib/librte_eal/linuxapp/eal/eal_dev.c | 140 +++++++++++++++++++++++++++++++- >> lib/librte_eal/rte_eal_version.map | 1 + >> 5 files changed, 169 insertions(+), 1 deletion(-) >> >> diff --git a/doc/guides/rel_notes/release_18_05.rst b/doc/guides/rel_notes/release_18_05.rst >> index cb9e050..2707e73 100644 >> --- a/doc/guides/rel_notes/release_18_05.rst >> +++ b/doc/guides/rel_notes/release_18_05.rst >> @@ -70,6 +70,12 @@ New Features >> >> Linux uevent is supported as backend of this device event notification framework. >> >> +* **Added hot plug failure handler.** >> + >> + Added a failure handler machenism to handle hot unplug device. >> + >> + * ``rte_dev_handle_hot_unplug`` for handle hot unplug device failure. >> + >> API Changes >> ----------- >> >> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c >> index 4cae4dd..293c310 100644 >> --- a/kernel/linux/igb_uio/igb_uio.c >> +++ b/kernel/linux/igb_uio/igb_uio.c >> @@ -344,6 +344,10 @@ igbuio_pci_release(struct uio_info *info, struct inode *inode) >> struct rte_uio_pci_dev *udev = info->priv; >> struct pci_dev *dev = udev->pdev; >> >> + /* check if device has been remove before release */ >> + if ((&dev->dev.kobj)->state_remove_uevent_sent == 1) >> + return -1; >> + >> mutex_lock(&udev->lock); >> if (--udev->refcnt > 0) { >> mutex_unlock(&udev->lock); >> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h >> index a5203e7..17c446d 100644 >> --- a/lib/librte_eal/common/include/rte_dev.h >> +++ b/lib/librte_eal/common/include/rte_dev.h >> @@ -361,4 +361,23 @@ rte_dev_event_monitor_start(void); >> */ >> int __rte_experimental >> rte_dev_event_monitor_stop(void); >> + >> +/** >> + * @warning >> + * @b EXPERIMENTAL: this API may change without prior notice >> + * >> + * It can be used to register the device signal bus handler, and save the >> + * current environment for each thread, when signal bus error invoke, the >> + * handler would restore the environment by long jmp to each working >> + * thread previous locate, then block the thread to waiting until the memory >> + * recovery and remapping be finished, that would guaranty the system not >> + * crash when the device be hot unplug. >> + * >> + * @param none >> + * @return >> + * - From a successful direct invocation, zero. >> + * - From a call of siglongjmp(), non_zero. >> + */ >> +int __rte_experimental >> +rte_dev_handle_hot_unplug(void); >> #endif /* _RTE_DEV_H_ */ >> diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c b/lib/librte_eal/linuxapp/eal/eal_dev.c >> index 9478a39..84b7efc 100644 >> --- a/lib/librte_eal/linuxapp/eal/eal_dev.c >> +++ b/lib/librte_eal/linuxapp/eal/eal_dev.c >> @@ -4,6 +4,9 @@ >> >> #include >> #include >> +#include >> +#include >> +#include >> #include >> #include >> >> @@ -13,12 +16,17 @@ >> #include >> #include >> #include >> +#include >> +#include >> >> #include "eal_private.h" >> >> static struct rte_intr_handle intr_handle = {.fd = -1 }; >> static bool monitor_started; >> >> +pthread_mutex_t failure_recovery_lock; >> +pthread_cond_t failure_recovery_cond; >> + >> #define EAL_UEV_MSG_LEN 4096 >> #define EAL_UEV_MSG_ELEM_LEN 128 >> >> @@ -32,6 +40,22 @@ enum eal_dev_event_subsystem { >> EAL_DEV_EVENT_SUBSYSTEM_MAX >> }; >> >> +static RTE_DEFINE_PER_LCORE(sigjmp_buf, unplug_longjmp_env); >> + >> +static void sigbus_handler(int signum __rte_unused) >> +{ >> + RTE_LOG(DEBUG, EAL, "receive SIGBUS error!\n"); >> + siglongjmp(RTE_PER_LCORE(unplug_longjmp_env), 1); >> +} >> + >> +static int cmp_dev_name(const struct rte_device *dev, >> + const void *_name) >> +{ >> + const char *name = _name; >> + >> + return strcmp(dev->name, name); >> +} >> + >> static int >> dev_uev_socket_fd_create(void) >> { >> @@ -132,6 +156,31 @@ dev_uev_parse(const char *buf, struct rte_dev_event *event, int length) >> return 0; >> } >> >> +static int >> +dev_uev_remove_handler(struct rte_device *dev) >> +{ >> + struct rte_bus *bus = rte_bus_find_by_device_name(dev->name); >> + int ret; >> + >> + if (!dev) >> + return -1; >> + >> + if (bus->handle_hot_unplug) { >> + /** >> + * call bus ops to handle hot unplug. >> + */ >> + ret = bus->handle_hot_unplug(dev); >> + if (ret) { >> + RTE_LOG(ERR, EAL, >> + "It cannot handle hot unplug for device (%s) " >> + "on the bus.\n ", >> + dev->name); >> + return ret; >> + } >> + } >> + return 0; >> +} >> + >> static void >> dev_delayed_unregister(void *param) >> { >> @@ -146,6 +195,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); >> @@ -170,11 +222,87 @@ 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 = "pci"; >> + break; >> + default: >> + break; >> + } >> + >> + if (uevent.devname) { >> + if (uevent.type == RTE_DEV_EVENT_REMOVE) { >> + bus = rte_bus_find_by_name(busname); >> + if (bus == NULL) { >> + RTE_LOG(ERR, EAL, "Cannot find bus (%s)\n", >> + uevent.devname); >> + return; >> + } >> + dev = bus->find_device(NULL, cmp_dev_name, >> + uevent.devname); >> + if (dev == NULL) { >> + RTE_LOG(ERR, EAL, >> + "Cannot find unplugged device (%s)\n", >> + uevent.devname); >> + return; >> + } >> + ret = dev_uev_remove_handler(dev); >> + if (ret) { >> + RTE_LOG(ERR, EAL, "Driver cannot remap the " >> + "device (%s)\n", >> + dev->name); >> + return; >> + } >> + /* wake up all the threads */ >> + pthread_cond_broadcast(&failure_recovery_cond); >> + } >> dev_callback_process(uevent.devname, uevent.type); >> + } >> } >> >> int __rte_experimental >> +rte_dev_handle_hot_unplug(void) >> +{ >> + struct sigaction act; >> + sigset_t mask; >> + int ret = 0; >> + >> + /* set signal handlers */ >> + memset(&act, 0x00, sizeof(struct sigaction)); >> + act.sa_handler = sigbus_handler; >> + sigemptyset(&act.sa_mask); >> + act.sa_flags = SA_RESTART; >> + sigaction(SIGBUS, &act, NULL); >> + sigemptyset(&mask); >> + sigaddset(&mask, SIGBUS); >> + pthread_sigmask(SIG_UNBLOCK, &mask, NULL); > Ok, but it is really hard to predict which thread will get a sigbus, > even a control thread could be affected just by reading HW stats. > Can we make it a unified one, per process? > yes, the signal would random sent to each thread, i will let the handler and the mask to be unified. >> + >> + ret = sigsetjmp(RTE_PER_LCORE(unplug_longjmp_env), 1); > Hmm... and why we have to do that? > Why we can't call recover procedure (as I understand all it will do - > mmap our unplugged dev VA to some dummy memory space) from sighandler itself, > and then just make thread to resume? > you do invoke a good question, seem make thread resume but not jump could be avoid some clean work and make thing be easily. >> + if (ret) { >> + /* >> + * Waitting for condition variable before failure recovery >> + * finish. Now the limitation is only handle one device >> + * hot plug, for multiple devices hotplug, need check if >> + * the device belong to this working thread, then directly >> + * call memory remaping, unrelated thread just keep going >> + * their work by no interrupt from hotplug. >> + * TODO: multiple device hotplug >> + */ >> + pthread_mutex_lock(&failure_recovery_lock); >> + RTE_LOG(DEBUG, EAL, "begin waiting for the failure handler.\n"); >> + pthread_cond_wait(&failure_recovery_cond, >> + &failure_recovery_lock); >> + RTE_LOG(DEBUG, EAL, >> + "come back from waiting for failure handler.\n"); >> + pthread_mutex_unlock(&failure_recovery_lock); > That's a strange one - how do you know that you received SIGBUS because of HW unplug? > Might be some buggy code is just user tried to access some mmaped file beyond it's boundaries, > or so? > In that case your signal handler would be hanged forever. > Second thing, how do you know that dev_uev_handler() has fixed that particular device unplugging? > I think you have first figure out by fault address - is that one of our device addresses, > and if yes - identify which particular. > Then do actual remap(). > BTW, once again why we need to wipe current prog state with longjmp()? > > Konstantin your are right , that would not handler the other cause well, and agree your second suggestion, we could use address of signal info to identify the device which is being unplug, that would be handler the multiple device hot unplug case, i will think about to implement it and remove the TODO comment. >> + } >> + >> + return ret; >> +} >> + >> + >> +int __rte_experimental >> rte_dev_event_monitor_start(void) >> { >> int ret; >> @@ -196,6 +324,12 @@ rte_dev_event_monitor_start(void) >> return -1; >> } >> >> + /* initialize mutex and condition variable >> + * to control failure recovery. >> + */ >> + pthread_mutex_init(&failure_recovery_lock, NULL); >> + pthread_cond_init(&failure_recovery_cond, NULL); >> + >> monitor_started = true; >> >> return 0; >> @@ -219,5 +353,9 @@ rte_dev_event_monitor_stop(void) >> close(intr_handle.fd); >> intr_handle.fd = -1; >> monitor_started = false; >> + >> + pthread_cond_destroy(&failure_recovery_cond); >> + pthread_mutex_destroy(&failure_recovery_lock); >> + >> return 0; >> } >> diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map >> index fc5c62a..873ef38 100644 >> --- a/lib/librte_eal/rte_eal_version.map >> +++ b/lib/librte_eal/rte_eal_version.map >> @@ -262,5 +262,6 @@ EXPERIMENTAL { >> rte_dev_event_monitor_stop; >> rte_dev_event_callback_register; >> rte_dev_event_callback_unregister; >> + rte_dev_handle_hot_unplug; >> >> } DPDK_18.02; >> -- >> 2.7.4