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 D6FDAA04B8; Tue, 5 May 2020 16:43:48 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 62D681D5F8; Tue, 5 May 2020 16:43:48 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 035561D5F0 for ; Tue, 5 May 2020 16:43:46 +0200 (CEST) IronPort-SDR: 7mrQa8Lo+m/m585EsMGyMKK4J0mIfNyZYCwGFrUC0j76KS6FXeKIeXF//AtWS9imqY7M/vGo98 HDi3zdDJZtMw== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 May 2020 07:43:45 -0700 IronPort-SDR: pWi2ceGyP1RHyOoZ2ovCVetTIV35cF6AK76F37NXPCqE9cCJAsUrdWpq3iB8aYsxsiLkualQE1 25bkqZ7spXfw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,355,1583222400"; d="scan'208";a="304494743" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.213.197.31]) ([10.213.197.31]) by FMSMGA003.fm.intel.com with ESMTP; 05 May 2020 07:43:43 -0700 To: Dmitry Kozlyuk Cc: dev@dpdk.org, "Dmitry Malloy (MESHCHANINOV)" , Narcisa Ana Maria Vasile , Fady Bader , Tal Shnaiderman , Bruce Richardson References: <20200410164342.1194634-1-dmitry.kozliuk@gmail.com> <20200428235015.2820677-1-dmitry.kozliuk@gmail.com> <20200428235015.2820677-4-dmitry.kozliuk@gmail.com> <87a53d30-0cf4-2e0d-1336-3420f0e0b6d3@intel.com> <20200501220043.12a08e31@Sovereign> From: "Burakov, Anatoly" Message-ID: <2ac571cf-c872-55c9-98b0-c22975fa5dbe@intel.com> Date: Tue, 5 May 2020 15:43:42 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200501220043.12a08e31@Sovereign> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 3/8] eal: introduce memory management wrappers 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 01-May-20 8:00 PM, Dmitry Kozlyuk wrote: > Thanks for pointing out the errors, see some comments inline. > > On 2020-04-29 18:13 GMT+0100 Burakov, Anatoly wrote: >> On 29-Apr-20 12:50 AM, Dmitry Kozlyuk wrote: > >>> + * Reservation size. Must be a multiple of system page size. >>> + * @param flags >>> + * Reservation options, a combination of eal_mem_reserve_flags. >>> + * @returns >>> + * Starting address of the reserved area on success, NULL on failure. >>> + * Callers must not access this memory until remapping it. >>> + */ >>> +void *eal_mem_reserve(void *requested_addr, size_t size, int flags); >> >> Should we also require requested_addr to be page-aligned? > > Yes. > >> Also, here and in other added API's, nitpick but our coding style guide >> (and the code style in this file) suggests that return value should be >> on a separate line, e.g. >> >> void * >> eal_mem_reserve(...); > > Will follow your advice in v5 to keep the style within this file consistent. > However, DPDK Coding Style explicitly says: > > Unlike function definitions, the function prototypes do not need to > place the function return type on a separate line. > > [snip] >>> + >>> +int >>> +rte_mem_lock(const void *virt, size_t size) >>> +{ >>> + return mlock(virt, size); >> >> This call can fail. It should pass errno as rte_errno as well, just like >> all other calls from this family. >> >> Also, if the implementation "may require" page alignment, how about >> requiring it unconditionally? > > IMO even better to document this function as locking all pages crossed by the > address region. This would save address checking/alignment at call site and > all implementations work this way. Locking memory implies paging system. > I don't think any other external API we provide does automagic pointer alignment, so i'm not sure if it indeed would be better to have it align automatically. It's also better from the standpoint of not silently allowing seemingly invalid arguments. So, i would lean on the side of requiring alignment, but not doing it ourselves. -- Thanks, Anatoly