From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 698A61B43C for ; Tue, 10 Jul 2018 12:07:38 +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 fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Jul 2018 03:07:37 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,334,1526367600"; d="scan'208";a="55740205" Received: from jguo15x-mobl3.ccr.corp.intel.com (HELO [10.67.68.57]) ([10.67.68.57]) by orsmga008.jf.intel.com with ESMTP; 10 Jul 2018 03:07:30 -0700 To: =?UTF-8?Q?Ga=c3=abtan_Rivet?= 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> <20180710084046.mhejmfodrebapgzt@bidouze.vm.6wind.com> 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 From: Jeff Guo Message-ID: <21ff311a-eb51-23a5-d330-71ab26c6fd89@intel.com> Date: Tue, 10 Jul 2018 18:07:27 +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: <20180710084046.mhejmfodrebapgzt@bidouze.vm.6wind.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v7 5/7] bus: add helper to handle sigbus 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: Tue, 10 Jul 2018 10:07:39 -0000 On 7/10/2018 4:40 PM, Gaëtan Rivet wrote: > 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). the purpose to set rte_errno here is because of the status of the handle need to pass though to the function caller "rte_bus_sigbus_handler", it could give a chance to check the searching status. >>>> +} >>>> + >>>> +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, i think that is the problem to find a better way, i agree to maximum to keep the rte_errno should be make sense.