From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id EF5F2C47258 for ; Wed, 31 Jan 2024 13:20:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7DEB16B0075; Wed, 31 Jan 2024 08:20:00 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 78F706B0083; Wed, 31 Jan 2024 08:20:00 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 656EC6B0085; Wed, 31 Jan 2024 08:20:00 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 506266B0075 for ; Wed, 31 Jan 2024 08:20:00 -0500 (EST) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 0FE13A0D41 for ; Wed, 31 Jan 2024 13:20:00 +0000 (UTC) X-FDA: 81739664160.17.CD3D6FD Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf21.hostedemail.com (Postfix) with ESMTP id 3FEC21C000B for ; Wed, 31 Jan 2024 13:19:58 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=none; spf=pass (imf21.hostedemail.com: domain of anshuman.khandual@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=anshuman.khandual@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1706707198; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=dhJXdcir/pT02PooyQlI9vyS0pICzIj4ApFRakYCTYc=; b=pj1uCfp2b0I6Yw805JjKUo37pBxw7IleAdOckm+v1B6HN553vOGj2v9xPEhNbghSiMZ6X8 +/QHbK1akHS+yVHqQchfKIW4fM1Ctq5FrHz+dkPLRABdcm+WHCBOxWRflGu9JHY3Mc5qtv GTS8tVHrbAQsG1uJ+OSqQAtT5tDojy0= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1706707198; a=rsa-sha256; cv=none; b=Ti4ItiIBZv5p5ieubY9KeOS8WEEKn0Jqkp5FDeR5vwxixGTMIg+3sP4VPsG/kQoFLBrnUz HrXDxyDFRD56JEA4Ji39DWLOT311UPngJEgDaWGt+F+ObCgqtw5BBmnAuGdHVDgZK3hJsz AgTJt+GpBQNLRtG7E4ZPvHwvl0QcYSs= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=none; spf=pass (imf21.hostedemail.com: domain of anshuman.khandual@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=anshuman.khandual@arm.com; dmarc=pass (policy=none) header.from=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C7B5BDA7; Wed, 31 Jan 2024 05:20:40 -0800 (PST) Received: from [10.163.41.195] (unknown [10.163.41.195]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 556463F738; Wed, 31 Jan 2024 05:19:35 -0800 (PST) Message-ID: Date: Wed, 31 Jan 2024 18:49:34 +0530 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH RFC v3 09/35] mm: cma: Introduce cma_remove_mem() Content-Language: en-US To: Alexandru Elisei Cc: catalin.marinas@arm.com, will@kernel.org, oliver.upton@linux.dev, maz@kernel.org, james.morse@arm.com, suzuki.poulose@arm.com, yuzenghui@huawei.com, arnd@arndb.de, akpm@linux-foundation.org, mingo@redhat.com, peterz@infradead.org, juri.lelli@redhat.com, vincent.guittot@linaro.org, dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com, mhiramat@kernel.org, rppt@kernel.org, hughd@google.com, pcc@google.com, steven.price@arm.com, vincenzo.frascino@arm.com, david@redhat.com, eugenis@google.com, kcc@google.com, hyesoo.yu@samsung.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kvmarm@lists.linux.dev, linux-fsdevel@vger.kernel.org, linux-arch@vger.kernel.org, linux-mm@kvack.org, linux-trace-kernel@vger.kernel.org References: <20240125164256.4147-1-alexandru.elisei@arm.com> <20240125164256.4147-10-alexandru.elisei@arm.com> <830691cf-cb96-443e-b6eb-2adfe2edd587@arm.com> From: Anshuman Khandual In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 3FEC21C000B X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: sc4xnobryg3qsmmdj93h1p5i38z943zz X-HE-Tag: 1706707198-390472 X-HE-Meta: U2FsdGVkX18fABFv6tJu2UJdTEO3/fPMr5XFRRRda16GlkGxfk4c6ZTyD8HV1dPiaLrLuvdoq0aAgQBSGKx2D+xfYIP52K6POiNHNSpByPXVKFfhr1WnbNSpqhbvbIg5IXwARwRYtZQSMjySDAUfYCbj03Mg6a65gW3wVEhWWwtFz7hZxi62qPXSo8IGrj8G9Usc98Ts44l+xtbaOieLNDb0Q2RmzXz5fYQ+TAccC4x8jfXpcEw5M+ZrjCxJjNSxaNktpHAUlmv58vQOlPHkc+qiEUzy1bW7b0hHqa+yhDiazgHIl32vXg6PJp/TRTcS0NcvQQjevNgwWnpClw1d4kNSn0vKlfGNB9/A4akQ0p+bZ6N1N07vysRYro5AZ3DFl8vLQS7SeAv3ZYXL8IvGvIm48tXoessrxKs1rQK4M03irPyZvrOnyQdg5QLr9uLkN4s/rNorcQU0/b8b/bBP/x/xLsPgLzTCvxgEQyAOnw8j6A6oaPtF8nUb2ERXgpgqEk/eqc1PSOCm+8OCvWQkk29k8nx4ifQL6BYswImw3u93a2gwRFMSwyi+H0IdsjS/196rVQZFog5GIDSHTE0Je/2aRbhPeriwiH1UkOAN5UGE+uUDpLcU8fkR6nqCdgkoEYjgRKO9ZXlIHHfMmuLAvJwas4lyTbCwGagJ2S5UcVm+SRemUu0fqvm2ilGYgwG8/iIt28Z/UHuJYxxCoTveC40k6kf+9hesfUXlncxmgllxGKg5Hs7wfmjWFpftvA+jNhrN1kU2FDa+ONfJHZoOBU9ic3/zNc8Vms8LGgmylqrSy3a3qjCejsyFBAe8OygVFoTRRHpJhHEPss76Va5Uq2l6GiZqslYFRtpAIuUq226X2LOvBNM3BoRQ7hdNP60Y+Gn5ULEppdRnEVr9GdR2uKcNxJwLa4pYNfQIyPz+xHEdOZv/KzcFLR87/dfPGKLyO01i4ysLYgEQa6pCORi rqdHkOTc iCeUnaOuGwVsNY0E4FkbvZVOzVaIQrccj2KGkd96uEZESqI3daBjW+EZd5Qd/vn0nFERuWz1Xa3oehLenrqIhaQfTXvVYW6nflzXKpdzR6u/493qU+M75BlayAhF7f/b721nFINuMYIATm2AqmgUeugLSYoQ3Mbr5XPbdgukcilwBYOV5NFks4FTLOnhivA0/QGZhWEHDdCKFE6C6xB99AFCsphYTTToGc/SwO6tbeVClajXof7nhhJUhAoGSHzCTx7q5 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 1/30/24 17:03, Alexandru Elisei wrote: > Hi, > > I really appreciate the feedback you have given me so far. I believe the > commit message isn't clear enough and there has been a confusion. > > A CMA user adds a CMA area to the cma_areas array with > cma_declare_contiguous_nid() or cma_init_reserved_mem(). > init_cma_reserved_pageblock() then iterates over the array and activates > all cma areas. Agreed. > > The function cma_remove_mem() is intended to be used to remove a cma area > from the cma_areas array **before** the area has been activated. Understood. > > Usecase: a driver (in this case, the arm64 dynamic tag storage code) > manages several cma areas. The driver successfully adds the first area to > the cma_areas array. When the driver tries to adds the second area, the > function fails. Without cma_remove_mem(), the driver has no way to prevent > the first area from being freed to the page allocator. cma_remove_mem() is > about providing a means to do cleanup in case of error. > > Does that make more sense now? How to ensure that cma_remove_mem() should get called by the driver before core_initcall()---> cma_init_reserved_areas()---> cma_activate_area() chain happens. Else cma_remove_mem() will miss out to clear cma->count and given area will proceed to get activated like always. > > Ok Tue, Jan 30, 2024 at 11:20:56AM +0530, Anshuman Khandual wrote: >> >> >> On 1/25/24 22:12, Alexandru Elisei wrote: >>> Memory is added to CMA with cma_declare_contiguous_nid() and >>> cma_init_reserved_mem(). This memory is then put on the MIGRATE_CMA list in >>> cma_init_reserved_areas(), where the page allocator can make use of it. >> >> cma_declare_contiguous_nid() reserves memory in memblock and marks the > > You forgot about about cma_init_reserved_mem() which does the same thing, > but yes, you are right. Agreed, missed that. There are some direct cma_init_reserved_mem() calls as well. > >> for subsequent CMA usage, where as cma_init_reserved_areas() activates >> these memory areas through init_cma_reserved_pageblock(). Standard page >> allocator only receives these memory via free_reserved_page() - only if > > I don't think that's correct. init_cma_reserved_pageblock() clears the > PG_reserved page flag, sets the migratetype to MIGRATE_CMA and then frees > the page. After that, the page is available to the standard page allocator > to use for allocation. Otherwise, what would be the point of the > MIGRATE_CMA migratetype? Understood and agreed. > >> the page block activation fails. > > For the sake of having a complete picture, I'll add that that only happens > if cma->reserve_pages_on_error is false. If the CMA user sets the field to > 'true' (with cma_reserve_pages_on_error()), then the pages in the CMA > region are kept PG_reserved if activation fails. Why cannot you use cma_reserve_pages_on_error() ? > >> >>> >>> If a device manages multiple CMA areas, and there's an error when one of >>> the areas is added to CMA, there is no mechanism for the device to prevent >> >> What kind of error ? init_cma_reserved_pageblock() fails ? But that will >> not happen until cma_init_reserved_areas(). > > I think I haven't been clear enough. When I say that "an area is added > to CMA", I mean that the memory region is added to cma_areas array, via > cma_declare_contiguous_nid() or cma_init_reserved_mem(). There are several > ways in which either function can fail. Okay. > >> >>> the rest of the areas, which were added before the error occured, from >>> being later added to the MIGRATE_CMA list. >> >> Why is this mechanism required ? cma_init_reserved_areas() scans over all >> CMA areas and try and activate each of them sequentially. Why is not this >> sufficient ? > > This patch is about removing a struct cma from the cma_areas array after it > has been added to the array, with cma_declare_contiguous_nid() or > cma_init_reserved_mem(), to prevent the area from being activated in > cma_init_reserved_areas(). Sorry for the confusion. > > I'll add a check in cma_remove_mem() to fail if the cma area has been > activated, and a comment to the function to explain its usage. That will be a good check. > >> >>> >>> Add cma_remove_mem() which allows a previously reserved CMA area to be >>> removed and thus it cannot be used by the page allocator. >> >> Successfully activated CMA areas do not get used by the buddy allocator. > > I don't believe that is correct, see above. Apologies, it's my bad. > >> >>> >>> Signed-off-by: Alexandru Elisei >>> --- >>> >>> Changes since rfc v2: >>> >>> * New patch. >>> >>> include/linux/cma.h | 1 + >>> mm/cma.c | 30 +++++++++++++++++++++++++++++- >>> 2 files changed, 30 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/linux/cma.h b/include/linux/cma.h >>> index e32559da6942..787cbec1702e 100644 >>> --- a/include/linux/cma.h >>> +++ b/include/linux/cma.h >>> @@ -48,6 +48,7 @@ extern int cma_init_reserved_mem(phys_addr_t base, phys_addr_t size, >>> unsigned int order_per_bit, >>> const char *name, >>> struct cma **res_cma); >>> +extern void cma_remove_mem(struct cma **res_cma); >>> extern struct page *cma_alloc(struct cma *cma, unsigned long count, unsigned int align, >>> bool no_warn); >>> extern int cma_alloc_range(struct cma *cma, unsigned long start, unsigned long count, >>> diff --git a/mm/cma.c b/mm/cma.c >>> index 4a0f68b9443b..2881bab12b01 100644 >>> --- a/mm/cma.c >>> +++ b/mm/cma.c >>> @@ -147,8 +147,12 @@ static int __init cma_init_reserved_areas(void) >>> { >>> int i; >>> >>> - for (i = 0; i < cma_area_count; i++) >>> + for (i = 0; i < cma_area_count; i++) { >>> + /* Region was removed. */ >>> + if (!cma_areas[i].count) >>> + continue; >> >> Skip previously added CMA area (now zeroed out) ? > > Yes, that's what I meant with the comment "Region was removed". Do you > think I should reword the comment? > >> >>> cma_activate_area(&cma_areas[i]); >>> + } >>> >>> return 0; >>> } >> >> cma_init_reserved_areas() gets called via core_initcall(). Some how >> platform/device needs to call cma_remove_mem() before core_initcall() >> gets called ? This might be time sensitive. > > I don't understand your point. > >> >>> @@ -216,6 +220,30 @@ int __init cma_init_reserved_mem(phys_addr_t base, phys_addr_t size, >>> return 0; >>> } >>> >>> +/** >>> + * cma_remove_mem() - remove cma area >>> + * @res_cma: Pointer to the cma region. >>> + * >>> + * This function removes a cma region created with cma_init_reserved_mem(). The >>> + * ->count is set to 0. >>> + */ >>> +void __init cma_remove_mem(struct cma **res_cma) >>> +{ >>> + struct cma *cma; >>> + >>> + if (WARN_ON_ONCE(!res_cma || !(*res_cma))) >>> + return; >>> + >>> + cma = *res_cma; >>> + if (WARN_ON_ONCE(!cma->count)) >>> + return; >>> + >>> + totalcma_pages -= cma->count; >>> + cma->count = 0; >>> + >>> + *res_cma = NULL; >>> +} >>> + >>> /** >>> * cma_declare_contiguous_nid() - reserve custom contiguous area >>> * @base: Base address of the reserved area optional, use 0 for any >> >> But first please do explain what are the errors device or platform might > > cma_declare_contiguous_nid() and cma_init_reserved_mem() can fail in a > number of ways, the code should be self documenting. But when they do fail - would not cma->count be left uninitialized as 0 ? Hence the proposed check (!cma->count) in cma_init_reserved_areas() should just do the trick without requiring an explicit cma_remove_mem() call. > >> see on a previously marked CMA area so that removing them on way becomes >> necessary preventing their activation via cma_init_reserved_areas(). > > I've described how the function is supposed to be used at the top of my > reply. > > Thanks, > Alex