From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id A67AD5F5B for ; Wed, 14 Mar 2018 17:53:37 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Mar 2018 09:53:36 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,306,1517904000"; d="scan'208";a="25244288" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.112]) ([10.237.220.112]) by orsmga008.jf.intel.com with ESMTP; 14 Mar 2018 09:53:35 -0700 To: Andrew Rybchenko , dev@dpdk.org Cc: Olivier MATZ References: <1516713372-10572-1-git-send-email-arybchenko@solarflare.com> <1520696382-16400-1-git-send-email-arybchenko@solarflare.com> <1520696382-16400-4-git-send-email-arybchenko@solarflare.com> <72792bc9-8e64-9a18-ce6c-ffd6777f1748@solarflare.com> From: "Burakov, Anatoly" Message-ID: <216ac4e5-2815-de23-4185-fd28210c0477@intel.com> Date: Wed, 14 Mar 2018 16:53:34 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <72792bc9-8e64-9a18-ce6c-ffd6777f1748@solarflare.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v1 3/9] mempool: remove callback to get capabilities 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: Wed, 14 Mar 2018 16:53:40 -0000 On 14-Mar-18 4:12 PM, Andrew Rybchenko wrote: > On 03/14/2018 05:40 PM, Burakov, Anatoly wrote: >> On 10-Mar-18 3:39 PM, Andrew Rybchenko wrote: >>> The callback was introduced to let generic code to know octeontx >>> mempool driver requirements to use single physically contiguous >>> memory chunk to store all objects and align object address to >>> total object size. Now these requirements are met using a new >>> callbacks to calculate required memory chunk size and to populate >>> objects using provided memory chunk. >>> >>> These capability flags are not used anywhere else. >>> >>> Restricting capabilities to flags is not generic and likely to >>> be insufficient to describe mempool driver features. If required >>> in the future, API which returns structured information may be >>> added. >>> >>> Signed-off-by: Andrew Rybchenko >>> --- >> >> Just a general comment - it is not enough to describe minimum memchunk >> requirements. With memory hotplug patchset that's hopefully getting >> merged in 18.05, memzones will no longer be guaranteed to be >> IOVA-contiguous. So, if a driver requires its mempool to not only be >> populated from a single memzone, but a single *physically contiguous* >> memzone, going by only callbacks will not do, because whether or not >> something should be a single memzone says nothing about whether this >> memzone has to also be IOVA-contiguous. >> >> So i believe this needs to stay in one form or another. >> >> (also it would be nice to have a flag that a user could pass to >> mempool_create that would force memzone reservation be >> IOVA-contiguous, but that's a topic for another conversation. prime >> user for this would be KNI.) > > I think that min_chunk_size should be treated as IOVA-contiguous. Why? It's perfectly reasonable to e.g. implement a software mempool driver that would perform some optimizations due to all objects being in the same VA-contiguous memzone, yet not be dependent on underlying physical memory layout. These are two separate concerns IMO. > So, we > have 4 levels: >  - MEMPOOL_F_NO_PHYS_CONTIG (min_chunk_size == 0) -- IOVA-congtiguous > is not required at all >  - no MEMPOOL_F_NO_PHYS_CONTIG (min_chunk_size == total_obj_size) -- > object should be IOVA-contiguous >  - min_chunk_size > total_obj_size  -- group of objects should be > IOVA-contiguous >  - min_chunk_size == -- all objects should be > IOVA-contiguous I don't think this "automagic" decision on what should be IOVA-contiguous or not is the way to go. It needlessly complicates things, when all it takes is another flag passed to mempool allocator somewhere. I'm not sure what is the best solution here. Perhaps another option would be to let mempool drivers allocate their memory as well? I.e. leave current behavior as default, as it's likely that it would be suitable for nearly all use cases, but provide another option to override memory allocation completely, so that e.g. octeontx could just do a memzone_reserve_contig() without regard for default allocation settings. I think this could be the cleanest solution. > > If so, how allocation should be implemented? >  1. if (min_chunk_size > min_page_size) >     a. try all contiguous >     b. if cannot, do by mem_chunk_size contiguous >  2. else allocate non-contiguous > > -- > Andrew. -- Thanks, Anatoly