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 48AB7C3DA5D for ; Mon, 22 Jul 2024 12:55:38 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8B3256B007B; Mon, 22 Jul 2024 08:55:37 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 863B36B0083; Mon, 22 Jul 2024 08:55:37 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 72AB06B0085; Mon, 22 Jul 2024 08:55:37 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 55AAD6B007B for ; Mon, 22 Jul 2024 08:55:37 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 9997581687 for ; Mon, 22 Jul 2024 12:55:36 +0000 (UTC) X-FDA: 82367385072.26.2F6923C Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by imf01.hostedemail.com (Postfix) with ESMTP id 332C040010 for ; Mon, 22 Jul 2024 12:55:32 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf01.hostedemail.com: domain of linyunsheng@huawei.com designates 45.249.212.255 as permitted sender) smtp.mailfrom=linyunsheng@huawei.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1721652897; a=rsa-sha256; cv=none; b=b0mMRYR5ygCnJc/pdA0J5SbpZLI6/xFShao1ZY3SVY1sdsGPpEZuu9gklWdaKh+/u9HUb0 FLkhRoHL/nk2y7cA7NLmTaukT9osx9G+vHSiWbAIwA4ttvxaSy+2zN7XymIKQtbk1K8WiI TUSBJfdBLmA/mxOKV8MEhB3T18aXtk0= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf01.hostedemail.com: domain of linyunsheng@huawei.com designates 45.249.212.255 as permitted sender) smtp.mailfrom=linyunsheng@huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1721652897; 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=k6kxY3zAklbrLpDNokaM2C8k0VFYW9vgMp0kyyi7QVc=; b=MQQGgomUN48HukFrwuuF/1TXoZRbe2p6lpJskxpyOqR1NO+OnP49IiXaqdcOVqo3/JXV9I WoeR1FkuEDD5WiNrZTHe83rYAkK/Rnt1FELd5q9CGzLqczJXqqoLEeCwUFzMe52ogGnQe4 Hn8TnLTkDV9lIPilHg58modVwCu11sc= Received: from mail.maildlp.com (unknown [172.19.163.48]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4WSKrP3GLmz1JDSt; Mon, 22 Jul 2024 20:50:29 +0800 (CST) Received: from dggpemf200006.china.huawei.com (unknown [7.185.36.61]) by mail.maildlp.com (Postfix) with ESMTPS id E79E0180064; Mon, 22 Jul 2024 20:55:27 +0800 (CST) Received: from [10.67.120.129] (10.67.120.129) by dggpemf200006.china.huawei.com (7.185.36.61) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.11; Mon, 22 Jul 2024 20:55:27 +0800 Message-ID: Date: Mon, 22 Jul 2024 20:55:27 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC v11 08/14] mm: page_frag: some minor refactoring before adding new API To: Alexander H Duyck , , , CC: , , Andrew Morton , References: <20240719093338.55117-1-linyunsheng@huawei.com> <20240719093338.55117-9-linyunsheng@huawei.com> Content-Language: en-US From: Yunsheng Lin In-Reply-To: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.120.129] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To dggpemf200006.china.huawei.com (7.185.36.61) X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 332C040010 X-Stat-Signature: j49nna7dsfmug1m3my8j676wt64rfgtc X-Rspam-User: X-HE-Tag: 1721652932-551097 X-HE-Meta: U2FsdGVkX18Eu2QCc+1EGFoZMCF9uiA4jB2UPYh3O2jVA+YZRLICUusCV4RIYWHUmEu6xcepYDS72JZZlQAO61QrdRoPFkfAwpCOKOUPPaCneY2FeP1A+1qqHgo2x8bGKHDTzjsWoaLg2C79EPOux5/3nQBQOhu/zIzlmliK5cv+ZUtECKy8FqFOMujewWEcTq5NtTeOQgtKweLc16Tf0q5mp630cSoCL868vwDbhuM2vcaMhcg/TY2LCcVIapi//e6EPM8w9mksIACpP9LSn8jLsf40OWGySH8EJtIXcJmncsXncos/1BBmAJGvi4ptJTP8/3AsJgT/erBc4+FAP8tenE+dq4MG2UceRLcTiIrub3YNW4y8ET7/l8eItdPTk+QW+8Y3+XkXRUMdcYas5HAOoYeSReDejlJzq0SAUJXTuwkth8hOQbCQCcTN9cuyAQsXt8uhBHertJtKGUuZtKWY9l4kYCm0R5JqdMfoXwcLF5XOFUm/bfeHxT0XCDSu2q41ihSyPinRDm8J0FUB1zgRgSzKWTrsRAW+c6MfnExoYq+lQ3ADFzvt+SL/226KbLkzhYPF6Yz8wHeQidPtc8ZzoekOJ3Elyw/cyBydm4fOcgC/urPRdihw57wVOQBjdJ9ZcDVUG9rFvcyNtcJ/MwYfDE1OxTh12li3N0y7o3d2ctZ5VuRGj+bbeLmIeipYqU7O3dbXruSDBl1SnrW0wfNMOhd0qRggRA2PTIIu6GUDfvxbQQ7PceTyONG+Fq6O6OOZ+EJ2ipAQ0f1VGLUrUIgORsPFi6HkwilNnByopi8vvW/Av8deKHggboUkTCgHrIfsZ3ToANAzxaSbwqxrLXaV+tu5UBBGKSSfeYrti30UTuUZugOhdMs/Yzeh9pCyMuNXytfXtn6C9PkTGvWQOTJdm4NqvecbnwKQB2+tuVKY/o9NALTF5ez2yJtc/gyKDfrdHFK7aXTffxlKXPb 0P2Exgg2 WafjhdHPw1AUE9XgkmSu3UfLocqc/AzIl4g/miI37EleqneqsYQJsFPJ+7tgfUhFq3rqlO4jWmgyEU68TKgjyyT8S6p6HS6sDydFKEU/izp6LjfYH188fwUN3ZpAPyElJNNMsPXucxKlec9ik6Iuvo0mBKmZh8s7bfQV3frCxS0LBgxdEfcnLqSDKFFN5lUZm6/GujMl4div91wYhek+xqgCX0vLVCMPkA0BtgXiOxHftoIFnYBWVYWWPKMZaIYUK+isMlUQ4rGSB8677RkMfi+6DI4pN45CAneml0sLUHpS5xXF3A6hLGer7mAPoha829WoQMJGT2jAeTMY= 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 2024/7/22 7:40, Alexander H Duyck wrote: > On Fri, 2024-07-19 at 17:33 +0800, Yunsheng Lin wrote: >> Refactor common codes from __page_frag_alloc_va_align() >> to __page_frag_cache_refill(), so that the new API can >> make use of them. >> >> CC: Alexander Duyck >> Signed-off-by: Yunsheng Lin >> --- >> include/linux/page_frag_cache.h | 2 +- >> mm/page_frag_cache.c | 93 +++++++++++++++++---------------- >> 2 files changed, 49 insertions(+), 46 deletions(-) >> >> diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h >> index 12a16f8e8ad0..5aa45de7a9a5 100644 >> --- a/include/linux/page_frag_cache.h >> +++ b/include/linux/page_frag_cache.h >> @@ -50,7 +50,7 @@ static inline void *encoded_page_address(unsigned long encoded_va) >> >> static inline void page_frag_cache_init(struct page_frag_cache *nc) >> { >> - nc->encoded_va = 0; >> + memset(nc, 0, sizeof(*nc)); >> } >> > > I do not like requiring the entire structure to be reset as a part of > init. If encoded_va is 0 then we have reset the page and the flags. > There shouldn't be anything else we need to reset as remaining and bias > will be reset when we reallocate. The argument is about aoviding one checking for fast path by doing the memset in the slow path, which you might already know accroding to your comment in previous version. It is just sometimes hard to understand your preference for maintainability over performance here as sometimes your comment seems to perfer performance over maintainability, like the LEA trick you mentioned and offset count-down before this patchset. It would be good to be more consistent about this, otherwise it is sometimes confusing when doing the refactoring. > >> static inline bool page_frag_cache_is_pfmemalloc(struct page_frag_cache *nc) >> diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c >> index 7928e5d50711..d9c9cad17af7 100644 >> --- a/mm/page_frag_cache.c >> +++ b/mm/page_frag_cache.c >> @@ -19,6 +19,28 @@ >> #include >> #include "internal.h" >> >> +static struct page *__page_frag_cache_recharge(struct page_frag_cache *nc) >> +{ >> + unsigned long encoded_va = nc->encoded_va; >> + struct page *page; >> + >> + page = virt_to_page((void *)encoded_va); >> + if (!page_ref_sub_and_test(page, nc->pagecnt_bias)) >> + return NULL; >> + >> + if (unlikely(encoded_page_pfmemalloc(encoded_va))) { >> + VM_BUG_ON(compound_order(page) != >> + encoded_page_order(encoded_va)); >> + free_unref_page(page, encoded_page_order(encoded_va)); >> + return NULL; >> + } >> + >> + /* OK, page count is 0, we can safely set it */ >> + set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1); >> + >> + return page; >> +} >> + >> static struct page *__page_frag_cache_refill(struct page_frag_cache *nc, >> gfp_t gfp_mask) >> { >> @@ -26,6 +48,14 @@ static struct page *__page_frag_cache_refill(struct page_frag_cache *nc, >> struct page *page = NULL; >> gfp_t gfp = gfp_mask; >> >> + if (likely(nc->encoded_va)) { >> + page = __page_frag_cache_recharge(nc); >> + if (page) { >> + order = encoded_page_order(nc->encoded_va); >> + goto out; >> + } >> + } >> + > > This code has no business here. This is refill, you just dropped > recharge in here which will make a complete mess of the ordering and be > confusing to say the least. > > The expectation was that if we are calling this function it is going to > overwrite the virtual address to NULL on failure so we discard the old > page if there is one present. This changes that behaviour. What you > effectively did is made __page_frag_cache_refill into the recharge > function. The idea is to reuse the below for both __page_frag_cache_refill() and __page_frag_cache_recharge(), which seems to be about maintainability to not having duplicated code. If there is a better idea to avoid that duplicated code while keeping the old behaviour, I am happy to change it. /* reset page count bias and remaining to start of new frag */ nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1; nc->remaining = PAGE_SIZE << order; > >> #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE) >> gfp_mask = (gfp_mask & ~__GFP_DIRECT_RECLAIM) | __GFP_COMP | >> __GFP_NOWARN | __GFP_NORETRY | __GFP_NOMEMALLOC; >> @@ -35,7 +65,7 @@ static struct page *__page_frag_cache_refill(struct page_frag_cache *nc, >> if (unlikely(!page)) { >> page = alloc_pages_node(NUMA_NO_NODE, gfp, 0); >> if (unlikely(!page)) { >> - nc->encoded_va = 0; >> + memset(nc, 0, sizeof(*nc)); >> return NULL; >> } >> > > The memset will take a few more instructions than the existing code > did. I would prefer to keep this as is if at all possible. It will not take more instructions for arm64 as it has 'stp' instruction for __HAVE_ARCH_MEMSET is set. There is something similar for x64? > >> @@ -45,6 +75,16 @@ static struct page *__page_frag_cache_refill(struct page_frag_cache *nc, >> nc->encoded_va = encode_aligned_va(page_address(page), order, >> page_is_pfmemalloc(page)); >> >> + /* Even if we own the page, we do not use atomic_set(). >> + * This would break get_page_unless_zero() users. >> + */ >> + page_ref_add(page, PAGE_FRAG_CACHE_MAX_SIZE); >> + >> +out: >> + /* reset page count bias and remaining to start of new frag */ >> + nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1; >> + nc->remaining = PAGE_SIZE << order; >> + >> return page; >> } >> > > Why bother returning a page at all? It doesn't seem like you don't use > it anymore. It looks like the use cases you have for it in patch 11/12 > all appear to be broken from what I can tell as you are adding page as > a variable when we don't need to be passing internal details to the > callers of the function when just a simple error return code would do. It would be good to be more specific about the 'broken' part here.