From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 3A26F2C38 for ; Tue, 3 Jul 2018 13:24:50 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Jul 2018 04:24:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,303,1526367600"; d="scan'208";a="71893695" Received: from jguo15x-mobl3.ccr.corp.intel.com (HELO [10.67.68.86]) ([10.67.68.86]) by orsmga002.jf.intel.com with ESMTP; 03 Jul 2018 04:24:45 -0700 To: =?UTF-8?Q?Ga=c3=abtan_Rivet?= , "Ananyev, Konstantin" References: <1498711073-42917-1-git-send-email-jia.guo@intel.com> <1530268248-7328-1-git-send-email-jia.guo@intel.com> <1530268248-7328-6-git-send-email-jia.guo@intel.com> <2601191342CEEE43887BDE71AB977258C0C43A49@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258C0C43C71@irsmsx105.ger.corp.intel.com> <20180629125200.uefvrokdg5nqthrl@bidouze.vm.6wind.com> Cc: "stephen@networkplumber.org" , "Richardson, Bruce" , "Yigit, Ferruh" , "Wu, Jingjing" , "thomas@monjalon.net" , "motih@mellanox.com" , "matan@mellanox.com" , "Van Haaren, Harry" , "Zhang, Qi Z" , "He, Shaopeng" , "Iremonger, Bernard" , "jblunck@infradead.org" , "shreyansh.jain@nxp.com" , "dev@dpdk.org" , "Zhang, Helin" From: "Guo, Jia" Message-ID: <2b9c1603-129e-7fa4-e9b5-770ad83e684c@intel.com> Date: Tue, 3 Jul 2018 19:24:42 +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: <20180629125200.uefvrokdg5nqthrl@bidouze.vm.6wind.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH V4 5/9] 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, 03 Jul 2018 11:24:52 -0000 hi, gaetan and konstantin answer both of your questions here as below. On 6/29/2018 8:52 PM, Gaëtan Rivet wrote: > On Fri, Jun 29, 2018 at 12:21:39PM +0000, Ananyev, Konstantin wrote: >> >>> -----Original Message----- >>> From: Guo, Jia >>> Sent: Friday, June 29, 2018 12:23 PM >>> To: Ananyev, Konstantin ; 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 >>> Subject: Re: [PATCH V4 5/9] bus: add helper to handle sigbus >>> >>> hi, konstantin >>> >>> >>> On 6/29/2018 6:51 PM, Ananyev, Konstantin wrote: >>>>> +int >>>>> +rte_bus_sigbus_handler(const void *failure_addr) >>>>> +{ >>>>> + struct rte_bus *bus; >>>>> + int old_errno = rte_errno; >>>>> + int ret = 0; >>>>> + >>>>> + rte_errno = 0; >>>>> + >>>>> + bus = rte_bus_find(NULL, bus_handle_sigbus, failure_addr); >>>>> + if (bus == NULL) { >>>>> + RTE_LOG(ERR, EAL, "No bus can handle the sigbus error!"); >>>>> + ret = -1; >>>>> + } else if (rte_errno != 0) { >>>>> + RTE_LOG(ERR, EAL, "Failed to handle the sigbus error!"); >>>>> + ret = -1; >>>>> + } >>>>> + >>>>> + /* if sigbus not be handled, return back old errno. */ >>>>> + if (ret) >>>>> + rte_errno = old_errno; >>>> Hmm, not sure why we need to set/restore rte_errno here? >>> restore old_errno just use to let caller know that the generic sigbus >>> still not handler by bus hotplug handler, that involve find a bus >>> handle but failed and can not find a hander, and can corresponding use >>> the previous sigbus handler to process it. >>> that is also unwser your question in other patch. do you think that make >>> sense? >> Sorry, still don't understand the intention. >> Suppose rte_bus_find() will return NULL, in that case you'll setup rte_errno >> to what it was before calling that function. >> If the returned bus is not NULL, but bus_find() set's an rte_errno, >> you still would restore rte_ernno? >> What is the prupose? >> Why do you need to touch rte_errno at all in that function? >> Konstantin >> > The way it is written here does not work, but the intention is > to make sure that a previous error is still catched. Something like > that: > > int old_errno = rte_errno; > > rte_errno = 0; > rte_eal_call(); > > if (rte_errno) > return -1; > else { > rte_errno = old_errno; > return 0; > } > > If someone calls the function while rte_errno is already set, then an > earlier error would be hidden by setting rte_errno to 0 within the > function. > > I'm not sure this is useful, but sometimes when using errno within a > library call I'm bothered that I am masking previous issues. > > Should it be avoided? i agree with konstantin about distinguish to process the handle failed or no handle, and agree with gaetan about restore the errno if it is not belong to the sigbus handler. Could you check if it is fulfill that as bellow, -1 means find bus but handle failed, use rte_exit. 1 means can no find bus, use older handler to handle. 0 means find bus and success handle. the handler is the new handler. static int bus_handle_sigbus(const struct rte_bus *bus, const void *failure_addr) { int ret; ret = bus->sigbus_handler(failure_addr); rte_errno = ret; return !(bus->sigbus_handler && ret <= 0); } 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 && rte_errno == -1) return -1; else if (!bus) ret =1; /* otherwise restore the old errno. */ rte_errno = old_errno; return ret; } static void sigbus_handler(int signum, siginfo_t *info, void *ctx __rte_unused) { int ret; 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!"); } }