From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 5FE315F1F for ; Mon, 9 Jul 2018 08:47:26 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1-us4.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id 17441280057; Mon, 9 Jul 2018 06:47:24 +0000 (UTC) Received: from [192.168.1.16] (85.187.13.33) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Mon, 9 Jul 2018 07:47:11 +0100 To: Jeff Guo , , , , , , , , , , , , , , CC: , , , , Shreyansh Jain References: <1531116091-18030-1-git-send-email-jia.guo@intel.com> <1531116091-18030-7-git-send-email-jia.guo@intel.com> From: Andrew Rybchenko Message-ID: <6a0473c8-2c5c-8c33-e6e1-46c439a94f9f@solarflare.com> Date: Mon, 9 Jul 2018 09:47:05 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <1531116091-18030-7-git-send-email-jia.guo@intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Originating-IP: [85.187.13.33] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-11.0.0.1191-8.100.1062-23956.003 X-TM-AS-Result: No--4.460600-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-MDID: 1531118845-Q-H9LQKdd-yf Subject: Re: [dpdk-dev] [PATCH v6 6/7] eal: add failure handle mechanism 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: Mon, 09 Jul 2018 06:47:26 -0000 CC Shreyansh On 09.07.2018 09:01, Jeff Guo wrote: > This patch introduces a failure handle mechanism to handle device > hotplug removal event. > > First it can register sigbus handler when enable device event monitor. Once > sigbus error be captured, it will check the failure address and accordingly > remap the invalid memory for the corresponding device. Besed on this > mechanism, it could guaranty the application not crash when the device be > hotplug out. > > Signed-off-by: Jeff Guo > Acked-by: Shaopeng He > --- > v6->v5: > refine some doc and coding style > --- > lib/librte_eal/linuxapp/eal/eal_dev.c | 114 +++++++++++++++++++++++++++++++++- > 1 file changed, 113 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c b/lib/librte_eal/linuxapp/eal/eal_dev.c > index 1cf6aeb..cb30729 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,31 @@ > #include > #include > #include > +#include > +#include > +#include > +#include > > #include "eal_private.h" > > static struct rte_intr_handle intr_handle = {.fd = -1 }; > static bool monitor_started; > > +extern struct rte_bus_list rte_bus_list; > + Shreyansh, shouldn't it be done in rte_bus.h? > #define EAL_UEV_MSG_LEN 4096 > #define EAL_UEV_MSG_ELEM_LEN 128 > > +/* > + * spinlock for device failure process, protect the bus and the device > + * to avoid race condition. > + */ > +static rte_spinlock_t dev_failure_lock = RTE_SPINLOCK_INITIALIZER; Unfortunately it is still not very helpful. I don't understand circumstance when the race condition could happen. > + > +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. */ > @@ -33,6 +51,49 @@ enum eal_dev_event_subsystem { > EAL_DEV_EVENT_SUBSYSTEM_MAX > }; > > +static void > +sigbus_action_recover(void) > +{ > + if (sigbus_need_recover) { > + sigaction(SIGBUS, &sigbus_action_old, NULL); > + sigbus_need_recover = 0; > + } > +} > + > +static void sigbus_handler(int signum, siginfo_t *info, > + void *ctx __rte_unused) > +{ > + int ret; > + > + RTE_LOG(INFO, EAL, "Thread[%d] catch SIGBUS, fault address:%p\n", > + (int)pthread_self(), info->si_addr); > + > + rte_spinlock_lock(&dev_failure_lock); > + ret = rte_bus_sigbus_handler(info->si_addr); > + rte_spinlock_unlock(&dev_failure_lock); > + if (ret == -1) { > + rte_exit(EXIT_FAILURE, > + "Failed to handle SIGBUS for hotplug, " > + "(rte_errno: %s)!", strerror(rte_errno)); > + } else if (ret == 1) { > + if (sigbus_action_old.sa_handler) > + (*(sigbus_action_old.sa_handler))(signum); > + else > + rte_exit(EXIT_FAILURE, > + "Failed to handle generic SIGBUS!"); > + } > + > + RTE_LOG(INFO, EAL, "Success to handle SIGBUS for hotplug!\n"); > +} > + > +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) > { > @@ -147,6 +208,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 +235,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 = "pci"; > + break; > + default: > + break; > + } > + > + if (uevent.devname) { > + if (uevent.type == RTE_DEV_EVENT_REMOVE) { > + rte_spinlock_lock(&dev_failure_lock); > + bus = rte_bus_find_by_name(busname); I see no point find bus by empty name. I would suggest to initialize busname to NULL and simply would not even try to find bus with such name. Just check above and generate appropriate log. > + if (bus == NULL) { > + RTE_LOG(ERR, EAL, "Cannot find bus (%s)\n", > + busname); > + return; > + } > + > + dev = bus->find_device(NULL, cmp_dev_name, > + uevent.devname); > + if (dev == NULL) { > + RTE_LOG(ERR, EAL, "Cannot find device (%s) on " > + "bus (%s)\n", uevent.devname, busname); > + return; > + } > + > + ret = bus->hotplug_failure_handler(dev); > + rte_spinlock_unlock(&dev_failure_lock); > + if (ret) { > + RTE_LOG(ERR, EAL, "Can not handle hotplug for " > + "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 +298,14 @@ rte_dev_event_monitor_start(void) > return -1; > } > > + /* register sigbus handler */ > + sigemptyset(&mask); > + sigaddset(&mask, SIGBUS); > + action.sa_flags = SA_SIGINFO; > + action.sa_mask = mask; > + action.sa_sigaction = sigbus_handler; > + sigbus_need_recover = !sigaction(SIGBUS, &action, &sigbus_action_old); > + > monitor_started = true; > > return 0; > @@ -217,8 +326,11 @@ rte_dev_event_monitor_stop(void) > return ret; > } > > + sigbus_action_recover(); > + > close(intr_handle.fd); > intr_handle.fd = -1; > monitor_started = false; > + > return 0; > }