From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 5EA16271 for ; Mon, 9 Jul 2018 07:40:22 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Jul 2018 22:40:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,329,1526367600"; d="scan'208";a="55382734" Received: from jguo15x-mobl3.ccr.corp.intel.com (HELO [10.67.68.84]) ([10.67.68.84]) by orsmga008.jf.intel.com with ESMTP; 08 Jul 2018 22:40:11 -0700 To: Andrew Rybchenko , 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, matan@mellanox.com, harry.van.haaren@intel.com, qi.z.zhang@intel.com, shaopeng.he@intel.com, bernard.iremonger@intel.com References: <1498711073-42917-1-git-send-email-jia.guo@intel.com> <1530776333-30318-1-git-send-email-jia.guo@intel.com> <1530776333-30318-7-git-send-email-jia.guo@intel.com> <6ac41f24-9a1b-5ca8-d7d7-bf983bf67f08@solarflare.com> Cc: jblunck@infradead.org, shreyansh.jain@nxp.com, dev@dpdk.org, helin.zhang@intel.com From: Jeff Guo Message-ID: <92e415fe-ce1b-5635-d460-fb9e8355e09c@intel.com> Date: Mon, 9 Jul 2018 13:40:11 +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: <6ac41f24-9a1b-5ca8-d7d7-bf983bf67f08@solarflare.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH V5 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 05:40:23 -0000 On 7/8/2018 9:46 PM, Andrew Rybchenko wrote: > On 05.07.2018 10:38, Jeff Guo wrote: >> This patch introduces a failure handler mechanism to handle device >> hot plug removal event. >> >> First register sigbus handler, once sigbus error be captured, will >> check the failure address and accordingly remap the invalid memory >> for the corresponding device. Bese on this mechanism, it could >> guaranty the application not to be crash when hotplug out device. >> >> Signed-off-by: Jeff Guo >> --- >> v5->v4: >> add sigbus old handler recover. >> --- >> lib/librte_eal/linuxapp/eal/eal_dev.c | 111 >> +++++++++++++++++++++++++++++++++- >> 1 file changed, 110 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..a22cb9a 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,28 @@ >> #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; >> + > > Shouldn't rte_bus.h provide it? > I think rte_bus.h provide the rte_bus_list structure, and then announcement a variable in eal_common_bus.c, then i use it by extern in eal_dev.c. >> #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 = RTE_SPINLOCK_INITIALIZER; > > It would be useful to explain why the lock is needed and when > it should be obtained/released. Which resources are protected > by the lock? > make sense, this locker should be use both bus and device access protection. Will explicit to let it to be more readable. >> + >> +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 +48,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 +205,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 +232,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); > > It looks like busname could be uninitialized here. > you are correct i think. >> + 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 +295,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 +323,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; >> } >