From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet Subject: Re: [PATCH v7 5/7] bus: add helper to handle sigbus Date: Tue, 10 Jul 2018 10:40:46 +0200 Message-ID: <20180710084046.mhejmfodrebapgzt@bidouze.vm.6wind.com> References: <1498711073-42917-1-git-send-email-jia.guo@intel.com> <1531137666-10351-1-git-send-email-jia.guo@intel.com> <1531137666-10351-6-git-send-email-jia.guo@intel.com> <7d719385-51e5-a45b-341a-d07f7cb92445@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: Andrew Rybchenko , stephen@networkplumber.org, bruce.richardson@intel.com, ferruh.yigit@intel.com, konstantin.ananyev@intel.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, wenzhuo.lu@intel.com, jblunck@infradead.org, shreyansh.jain@nxp.com, dev@dpdk.org, helin.zhang@intel.com To: Jeff Guo Return-path: Received: from mail-wm0-f67.google.com (mail-wm0-f67.google.com [74.125.82.67]) by dpdk.org (Postfix) with ESMTP id 8B9C25F34 for ; Tue, 10 Jul 2018 10:41:29 +0200 (CEST) Received: by mail-wm0-f67.google.com with SMTP id v3-v6so19807486wmh.0 for ; Tue, 10 Jul 2018 01:41:29 -0700 (PDT) Content-Disposition: inline In-Reply-To: <7d719385-51e5-a45b-341a-d07f7cb92445@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Jul 10, 2018 at 04:22:23PM +0800, Jeff Guo wrote: > > > On 7/9/2018 9:48 PM, Andrew Rybchenko wrote: > > On 09.07.2018 15:01, Jeff Guo wrote: > > > This patch aim to add a helper to iterate all buses to find the > > > corresponding bus to handle the sigbus error. > > > > > > Signed-off-by: Jeff Guo > > > Acked-by: Shaopeng He > > > --- > > > v7->v6: > > > no change > > > --- > > > lib/librte_eal/common/eal_common_bus.c | 42 > > > ++++++++++++++++++++++++++++++++++ > > > lib/librte_eal/common/eal_private.h | 12 ++++++++++ > > > 2 files changed, 54 insertions(+) > > > > > > diff --git a/lib/librte_eal/common/eal_common_bus.c > > > b/lib/librte_eal/common/eal_common_bus.c > > > index 0943851..8856adc 100644 > > > --- a/lib/librte_eal/common/eal_common_bus.c > > > +++ b/lib/librte_eal/common/eal_common_bus.c > > > @@ -37,6 +37,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > #include "eal_private.h" > > > @@ -242,3 +243,44 @@ rte_bus_get_iommu_class(void) > > > } > > > return mode; > > > } > > > + > > > +static int > > > +bus_handle_sigbus(const struct rte_bus *bus, > > > + const void *failure_addr) > > > +{ > > > + int ret; > > > + > > > + if (!bus->sigbus_handler) { > > > + RTE_LOG(ERR, EAL, "Function sigbus_handler not supported by " > > > + "bus (%s)\n", bus->name); > > > > It is not an error. It is OK that some buses cannot handle SIGBUS. > > > > yes, it is. > > > > + return -1; > > > + } > > > + > > > + ret = bus->sigbus_handler(failure_addr); > > > + rte_errno = ret; > > > + > > > + return !(bus->sigbus_handler && ret <= 0); > > > > There is no point to check bus->sigbus_handler here. It is already > > checked above. > > So, it should be just: > > return ret > 0; > > I.e. we should continue search if the address is not handled by any > > device > > on the bus (we should stop if it is handled (ret==0) or failed to to > > handle > > (ret < 0)). > > > > i will modify it, thanks. > Why is rte_errno set here? rte_errno is meant by the bus dev to be set on error. You do not have to modify it. ret would already be <0 on error. At most, you could do something like: if (ret < 0 && rte_errno == 0) rte_errno = ENOTSUP; Or something akin, with a non-descriptive error hinting that the developper didn't seem to care about setting errno to something meaningful (so only partially respecting the API). > > > +} > > > + > > > +int > > > +rte_bus_sigbus_handler(const void *failure_addr) > > > +{ > > > + struct rte_bus *bus; > > > + > > > + int ret = 0; > > > + int old_errno = rte_errno; > > > + > > > + rte_errno = 0; > > > + > > > + bus = rte_bus_find(NULL, bus_handle_sigbus, failure_addr); > > > + /* failed to handle the sigbus, pass the new errno. */ > > > + if (!bus) > > > + ret = 1; > > > + else if (rte_errno == -1) > > > > I'm still thinking it is bad to keep negative value in rte_errno here. > > > > i think the rte_errno just no used for the caller if return -1. Since if > find bus but process failed, will use rte_exit to process whatever the > rte_errno value. Only return 1 means use the origin sigbus handler that will > care about the errno. > With the changes above, the check should be something like: if (bus == NULL) return 1; else if (rte_errno != 0) return -rte_errno; rte_errno = old_errno; return 0; Which would avoid resetting rte_errno on top of whichever value a dev would have used, and having it set to a negative non-errno value. (Please do not just use this as-is, if you think this is not a good idea just tell us why or how you would prefer to do it. I'm only proposing a way that I think would work.) Regards, -- Gaëtan Rivet 6WIND