From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4C855A00C5; Thu, 11 Jun 2020 10:59:35 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 24E281B94F; Thu, 11 Jun 2020 10:59:35 +0200 (CEST) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 425D65F69 for ; Thu, 11 Jun 2020 10:59:34 +0200 (CEST) IronPort-SDR: 3v7UHguHxJFmXfdwbm6kH8z8gTF6XKYzvzn3CHDVTA+GB2PK3+i6yQDGXD52zLkfXDrrD+snnD R+95Z4R6We1A== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2020 01:59:33 -0700 IronPort-SDR: wFUqKn9mqZpkoTHSpDHZdcrK9Jj8hfL5mapNIn31ZJHP1WkEFr8jA1NMExOPCpldC2sVjSQzhK ltFEG+D3w8bw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,499,1583222400"; d="scan'208";a="306867160" Received: from strampit-mobl.ger.corp.intel.com (HELO [10.252.45.191]) ([10.252.45.191]) by orsmga008.jf.intel.com with ESMTP; 11 Jun 2020 01:59:31 -0700 To: Dmitry Kozlyuk Cc: dev@dpdk.org, Dmitry Malloy , Narcisa Ana Maria Vasile , Fady Bader , Tal Shnaiderman , Bruce Richardson References: <20200525003720.6410-1-dmitry.kozliuk@gmail.com> <20200602230329.17838-1-dmitry.kozliuk@gmail.com> <20200602230329.17838-5-dmitry.kozliuk@gmail.com> <255a3887-b187-e3b2-cf06-a0a67942e788@intel.com> <20200609171733.7e3d9601@sovereign> <20200610173151.27f1a1aa@sovereign> <201ca89a-ae94-0bc1-184f-ac8f41b38b57@intel.com> <20200610193946.2c1d704a@sovereign> From: "Burakov, Anatoly" Message-ID: <698e0cac-3fe0-d54b-8a43-d0dbf99c56d2@intel.com> Date: Thu, 11 Jun 2020 09:59:30 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.8.1 MIME-Version: 1.0 In-Reply-To: <20200610193946.2c1d704a@sovereign> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 04/11] eal/mem: extract common code for memseg list initialization 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 10-Jun-20 5:39 PM, Dmitry Kozlyuk wrote: > On Wed, 10 Jun 2020 16:48:58 +0100 > "Burakov, Anatoly" wrote: > >> On 10-Jun-20 3:31 PM, Dmitry Kozlyuk wrote: >>> On Wed, 10 Jun 2020 11:26:22 +0100 >>> "Burakov, Anatoly" wrote: >>> >>> [snip] >>>>>>> + addr = eal_get_virtual_area( >>>>>>> + msl->base_va, &mem_sz, page_sz, 0, reserve_flags); >>>>>>> + if (addr == NULL) { >>>>>>> +#ifndef RTE_EXEC_ENV_WINDOWS >>>>>>> + /* The hint would be misleading on Windows, but this function >>>>>>> + * is called from many places, including common code, >>>>>>> + * so don't duplicate the message. >>>>>>> + */ >>>>>>> + if (rte_errno == EADDRNOTAVAIL) >>>>>>> + RTE_LOG(ERR, EAL, "Cannot reserve %llu bytes at [%p] - " >>>>>>> + "please use '--" OPT_BASE_VIRTADDR "' option\n", >>>>>>> + (unsigned long long)mem_sz, msl->base_va); >>>>>>> + else >>>>>>> + RTE_LOG(ERR, EAL, "Cannot reserve memory\n"); >>>>>>> +#endif >>>>>> >>>>>> You're left without any error messages on Windows. How about: >>>>>> >>>>>> const char *err_str = "Cannot reserve memory\n"; >>>>>> #ifndef RTE_EXEC_ENV_WINDOWS >>>>>> if (rte_errno == EADDRNOTAVAIL) >>>>>> err_str = ... >>>>>> #endif >>>>>> RTE_LOG(ERR, EAL, err_str); >>>>>> >>>>>> or something like that? >>>>>> >>>>> >>>>> How about removing generic error message here completely and printing more >>>>> specific messages at call sites? In fact, almost all of them already do this. >>>>> It would be more helpful in tracking down errors. >>>>> >>>> >>>> Agreed, let's do that :) We do pass up the rte_errno, correct? So, we >>>> should be able to do that. >>> >>> Actually, callers don't need rte_errno, because we only have to distinguish >>> EADDRNOTAVAIL here, and eal_get_virtual_area() already prints precise >>> diagnostics at WARNING and ERR level. rte_errno is preserved, however. >>> >> >> Not sure i agree, we still need the "--base-virtaddr" hint, and we can >> only do that from the caller (without #ifdef-ery here), so we do need >> rte_errno for that. > > Maybe we're talking about different things. The "--base-virtaddr" hint is > printed from eal_memseg_list_alloc() on Unices for EADDRNOTAVAIL. > This is handy to avoid duplicating the hint and to provide context, so let's > keep it despite #ifndef. > > Otherwise, a generic error is printed from the same function (mistakenly not > on Windows in v6). This generic error adds nothing to eal_get_virtual_area() > logs and also doesn't help to known which exact eal_memseg_list_alloc() > failed. If instead callers printed their own messages, it would be clear > which call failed and in which context. Generic error can than be removed, > eal_memseg_list_alloc() code simplified. Callers can inspect rte_errno if they > ever need it, but really they don't, because hint is printed by > eal_memseg_list_alloc() and eal_get_virtual_area() prints even more precise > logs. This is what I did in v8. > Right, OK :) -- Thanks, Anatoly