From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 6133E1B55C for ; Fri, 29 Jun 2018 14:06:23 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Jun 2018 05:06:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,285,1526367600"; d="scan'208";a="62583726" Received: from irsmsx109.ger.corp.intel.com ([163.33.3.23]) by fmsmga002.fm.intel.com with ESMTP; 29 Jun 2018 05:06:19 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.126]) by IRSMSX109.ger.corp.intel.com ([169.254.13.225]) with mapi id 14.03.0319.002; Fri, 29 Jun 2018 13:06:18 +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" , "Zhang, Qi Z" , "He, Shaopeng" , "Iremonger, Bernard" CC: "jblunck@infradead.org" , "shreyansh.jain@nxp.com" , "dev@dpdk.org" , "Zhang, Helin" Thread-Topic: [PATCH V4 6/9] eal: add failure handle mechanism for hot plug Thread-Index: AQHUD5SkQ/ndgb6g5kGRsSeM4YxFrqR3DEZA///4qYCAAB4LEA== Date: Fri, 29 Jun 2018 12:06:18 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258C0C43C30@irsmsx105.ger.corp.intel.com> References: <1498711073-42917-1-git-send-email-jia.guo@intel.com> <1530268248-7328-1-git-send-email-jia.guo@intel.com> <1530268248-7328-7-git-send-email-jia.guo@intel.com> <2601191342CEEE43887BDE71AB977258C0C43A33@irsmsx105.ger.corp.intel.com> <556075ba-0a9e-30c8-2215-10a1e04a7111@intel.com> In-Reply-To: <556075ba-0a9e-30c8-2215-10a1e04a7111@intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMzJlYzYwMTgtODdiNC00MmY2LTkxZDctMTVhZjVkOGU4MmZmIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoickRSRHZ6bkFVM2Zkd1JmaU9sUko3T2QyWlZpXC9IWFlEZytZTmVEdnI2TWpHZDVENm5rMmw5UzlLT3FJcW1ZTUoifQ== 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 V4 6/9] 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: Fri, 29 Jun 2018 12:06:24 -0000 > -----Original Message----- > From: Guo, Jia > Sent: Friday, June 29, 2018 12:15 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 > ; Zhang, Qi Z ; He, Sha= openg ; Iremonger, Bernard > > Cc: jblunck@infradead.org; shreyansh.jain@nxp.com; dev@dpdk.org; Zhang, H= elin > Subject: Re: [PATCH V4 6/9] eal: add failure handle mechanism for hot plu= g >=20 > hi,konstantin >=20 >=20 > On 6/29/2018 6:49 PM, Ananyev, Konstantin wrote: > > Hi Jeff, > > > >> 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 hot unplug devices. > >> > >> Signed-off-by: Jeff Guo > >> --- > >> v4->v3: > >> split patches to be small and clear. > >> --- > >> lib/librte_eal/linuxapp/eal/eal_dev.c | 88 +++++++++++++++++++++++++= +++++++++- > >> 1 file changed, 87 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..c9dddab 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,24 @@ > >> #include > >> #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 void dev_uev_handler(__rte_unused void *param); > >> > >> /* identify the system layer which reports this event. */ > >> @@ -33,6 +44,34 @@ enum eal_dev_event_subsystem { > >> EAL_DEV_EVENT_SUBSYSTEM_MAX > >> }; > >> > >> +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 rte_bus_sigbus_handler(info->si_addr); > >> + rte_spinlock_unlock(&dev_failure_lock); > >> + if (!ret) > >> + RTE_LOG(INFO, EAL, > >> + "Success to handle SIGBUS error for hotplug!\n"); > >> + else > >> + rte_exit(EXIT_FAILURE, > >> + "A generic SIGBUS error, (rte_errno: %s)!", > >> + strerror(rte_errno)); > >> +} > > As I said in comments for previous versions: > > I think we need to distinguish why do we fail - > > 1) address doesn't belong to any device, > > 2) we failed to remap > > For 1) we probably need to call previous sigbus handler. >=20 > i know your point, but i think what ever 1) or 2), we should also need > to call previous sigbus handler to show exception of the memory error, > to cut down any other after try to run. I don't agree. If 1) - that error doesn't belong to us (DPDK), but user app might know how= to handle it. So we just invoke previously saved previous (if any) sigbus handler. for 2) - there is not much we can do but rte_exit().=20 > and for the previous sigbus handler, i still not find a explicit call to > use it, i think it the sigbus handler could be restore but only will use > when next error occur, right? > if so, do you think i just use a rte_exit to replace this origin handler > is make sense? >=20 > >> + > >> +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) > >> { > >> @@ -147,6 +186,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 +213,48 @@ 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", > >> + busname); > >> + return; > >> + } > >> + dev =3D bus->find_device(NULL, cmp_dev_name, > >> + uevent.devname); > >> + if (dev =3D=3D NULL) { > >> + RTE_LOG(ERR, EAL, "Cannot find device (%s) on " > >> + "bus (%s)\n", uevent.devname, busname); > >> + return; > >> + } > >> + rte_spinlock_lock(&dev_failure_lock); > >> + ret =3D bus->hotplug_handler(dev); > >> + rte_spinlock_unlock(&dev_failure_lock); > > Ok, but this function is executed from interrupt thread, correct? >=20 > yes. >=20 > > What would happen if user would do dev-detach() at the same time and de= v would not be valid anymore? > > Shouldn't we have a lock (per bus?) that we would grab before find_devi= ce() and release after hotplug_handler? > > Though in that case we probably need to revisit other bus ops too. >=20 > make sense, i think should be the case and need lock any bus ops here to > sync. >=20 > >> + 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 +274,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; > >> + sigaction(SIGBUS, &action, NULL); > >> + > > I still think we have to save (and restore at monitor_stop) previous si= gbus handler. >=20 > ok, i think i missing here, if monitor_stop, definitely should restore > the previous sigbus handler. >=20 > >> monitor_started =3D true; > >> > >> return 0; > >> @@ -220,5 +305,6 @@ rte_dev_event_monitor_stop(void) > >> close(intr_handle.fd); > >> intr_handle.fd =3D -1; > >> monitor_started =3D false; > >> + > >> return 0; > >> } > >> -- > >> 2.7.4