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 12AE9C3DA7E for ; Tue, 30 Jul 2024 13:20:56 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 96E946B007B; Tue, 30 Jul 2024 09:20:55 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 91E4F6B0082; Tue, 30 Jul 2024 09:20:55 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7E60A6B0085; Tue, 30 Jul 2024 09:20:55 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 5B3466B007B for ; Tue, 30 Jul 2024 09:20:55 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id A1D831402B3 for ; Tue, 30 Jul 2024 13:20:54 +0000 (UTC) X-FDA: 82396479228.26.961123E Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by imf10.hostedemail.com (Postfix) with ESMTP id 14AE5C0024 for ; Tue, 30 Jul 2024 13:20:50 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf10.hostedemail.com: domain of linyunsheng@huawei.com designates 45.249.212.188 as permitted sender) smtp.mailfrom=linyunsheng@huawei.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1722345611; a=rsa-sha256; cv=none; b=aKYvXSdgbK7522GVUEesflAZykFzP9nuo9FHZ9EWYOYi+hMoGku+8wBGTUmnsKxgA1xf1I fXaBFaws9Er+487dWg7hVAEw1iWvWw65tCyhOLhX4XIY+zSwkXWq1yw0ehAu8exq6+bJQF 1km68DSi2WBpOnXJcMHJ83+aP+yX3ik= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf10.hostedemail.com: domain of linyunsheng@huawei.com designates 45.249.212.188 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=1722345611; 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=Fe92aK4HSvlFB0f0JNrzu957q/dE2s6I5u3CI8vG6EU=; b=qJ2huE//1q2WDDEDxZGss2vILyFc5NSHKN6yiH29R0dUzvqP6AKktSgQosEaSehyMDyfoW Zum7oEIJQ+ymhs4syZZmtFNSqsHOmeRFHnChN77pRGzKchBqMeLZRmSU1PAnjgDqhvMqND f2YXfhYqXrqQ8Kgzil1fWViLYsXaWAE= Received: from mail.maildlp.com (unknown [172.19.88.194]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4WYG6Y1HPNzncv0; Tue, 30 Jul 2024 21:19:49 +0800 (CST) Received: from dggpemf200006.china.huawei.com (unknown [7.185.36.61]) by mail.maildlp.com (Postfix) with ESMTPS id 472C7140FAA; Tue, 30 Jul 2024 21:20:47 +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; Tue, 30 Jul 2024 21:20:46 +0800 Message-ID: Date: Tue, 30 Jul 2024 21:20:46 +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 From: Yunsheng Lin To: Alexander Duyck CC: , , , , , Andrew Morton , References: <20240719093338.55117-1-linyunsheng@huawei.com> <20240719093338.55117-9-linyunsheng@huawei.com> <5a0e12c1-0e98-426a-ab4d-50de2b09f36f@huawei.com> Content-Language: en-US In-Reply-To: <5a0e12c1-0e98-426a-ab4d-50de2b09f36f@huawei.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.120.129] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To dggpemf200006.china.huawei.com (7.185.36.61) X-Rspamd-Queue-Id: 14AE5C0024 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: xkfaaa5fmf33qxzs6idjs6w8bkire18z X-HE-Tag: 1722345650-202023 X-HE-Meta: U2FsdGVkX1/NZse0y+3TrEn8zwx8JKqp7fo3uoMdT03/YLArbn+mm4IHTAsQkx9fSSDsqj7aFCPl6aUZFE5RL2kcq9FZwAFymovELpdLK7recBxD1DOuoFVLYxfO7iu5k/j5GQxi5rGO56nYOrk3DQKZMbxxrK8yyAT+Rxd2hgJKyBbJhCV9dCYzlYHfk5sPQUTuQPLj1MAqR6s9Icos9ATjkdha9JWJ/V20eHeVCj8YgRRI0dCqB98tQ7n/IsQ2xg4TIFd4+NdochnrZewvL4j54abbmlPimFIPKb2RaCP7Msiwnu17yr8O6w0UTR9QR0YznwtghYw07EBlOkOGLpyZfKJcThF5rU/CxwwyfqFl2xTi40Dsw+g0ts4wxguGEUpxn+Mta8NGcotgG5TCBzt6Ru9mW72qWIded9RRUJGtjRv9YcUg6cUQaI1lPNNB3nCkaaqeTZcPOms7cvXs3RXhQthSM/9qZYUxW+3HhFTn2g/sNzdQsWIPKBajEvIpTco0nTQetbABnZ5Yq3uahYKQ1ZQhVH6U0FItipxLgIYwVSJ+Bq21CVR/Lpjo8AovBsyYnMJYXV+AUSI2J/93b59blDnVP9dm8WVWJf4U0UKrXz7kR7wZN3lopZ7r6sqb5bTHB0NrP8SCUkrbdCJKCjz4wTVbIZyA5l6v69ovbWa7BSeh3vd3ZkrV1jWyIiIGqW/lBrFMwCcpOAGlONFvr52EuaqvAmDJJvgKC/y6fgxC+UYEEHaiYqZG11aZ40HXOSDvV+U7MD0aZCFq2EBCMsduTv+nQLriz1HtTwHvtc8ssf6Pu3eEaKQOzd8wtqCed2zypPxcYBy8lKblfPYiMZOt5TYlFO9OjoH7X99Nju6Bz9P8v9IJzmjPgccsY+MGAHkXloq+Y10OLYn6jZVSt7RrZInCH9HKHdXZHXe/kHG8f+v8LrVNvNfnz2PoI+KcbGyB584IG8SwX5TTQLj 0YKZUjvo kpYBzhFTCUVOCTC2nm7aOIsp1NmDPLIpY4imizklNFpJUnJJJKtl6wWVfD05g6uipyjpHApp4n5fKCvRA0+35F0c8eWlP5LvQMeZjqu+3ApTW0fmpa9tY5xppRYe3PNqrg/WptDbpDZYfsvp0aRPcdgpF8JcBDlfWXfrURy+NzoTYe5QS6LA1/mz975wPBJF1G+upZ4w1eU6y8j8SxefTx0uI94CjJkbJNAQQdcZ5hcUEv2VjX7CMXQRTh9yQvVLxMpaXxd2bp5nWnhPwFDivOuaD6furRuL0+vnz 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/23 21:19, Yunsheng Lin wrote: ... >>>>> 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; >>> >> >> The only piece that is really reused here is the pagecnt_bias >> assignment. What is obfuscated away is that the order is gotten >> through one of two paths. Really order isn't order here it is size. >> Which should have been fetched already. What you end up doing with >> this change is duplicating a bunch of code throughout the function. >> You end up having to fetch size multiple times multiple ways. here you >> are generating it with order. Then you have to turn around and get it >> again at the start of the function, and again after calling this >> function in order to pull it back out. > > I am assuming you would like to reserve old behavior as below? > > if(!encoded_va) { > refill: > __page_frag_cache_refill() > } > > > if(remaining < fragsz) { > if(!__page_frag_cache_recharge()) > goto refill; > } > > As we are adding new APIs, are we expecting new APIs also duplicate > the above pattern? > >> How about something like below? __page_frag_cache_refill() and __page_frag_cache_reuse() does what their function name suggests as much as possible, __page_frag_cache_reload() is added to avoid new APIs duplicating similar pattern as much as possible, also avoid fetching size multiple times multiple ways as much as possible. static struct page *__page_frag_cache_reuse(unsigned long encoded_va, unsigned int pagecnt_bias) { struct page *page; page = virt_to_page((void *)encoded_va); if (!page_ref_sub_and_test(page, 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) { unsigned long order = PAGE_FRAG_CACHE_MAX_ORDER; struct page *page = NULL; gfp_t gfp = gfp_mask; #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE) gfp_mask = (gfp_mask & ~__GFP_DIRECT_RECLAIM) | __GFP_COMP | __GFP_NOWARN | __GFP_NORETRY | __GFP_NOMEMALLOC; page = alloc_pages_node(NUMA_NO_NODE, gfp_mask, PAGE_FRAG_CACHE_MAX_ORDER); #endif if (unlikely(!page)) { page = alloc_pages_node(NUMA_NO_NODE, gfp, 0); if (unlikely(!page)) { memset(nc, 0, sizeof(*nc)); return NULL; } order = 0; } 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); return page; } static struct page *__page_frag_cache_reload(struct page_frag_cache *nc, gfp_t gfp_mask) { struct page *page; if (likely(nc->encoded_va)) { page = __page_frag_cache_reuse(nc->encoded_va, nc->pagecnt_bias); if (page) goto out; } page = __page_frag_cache_refill(nc, gfp_mask); if (unlikely(!page)) return NULL; out: nc->remaining = page_frag_cache_page_size(nc->encoded_va); nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1; return page; } void *__page_frag_alloc_va_align(struct page_frag_cache *nc, unsigned int fragsz, gfp_t gfp_mask, unsigned int align_mask) { unsigned long encoded_va = nc->encoded_va; unsigned int remaining; remaining = nc->remaining & align_mask; if (unlikely(remaining < fragsz)) { if (unlikely(fragsz > PAGE_SIZE)) { /* * The caller is trying to allocate a fragment * with fragsz > PAGE_SIZE but the cache isn't big * enough to satisfy the request, this may * happen in low memory conditions. * We don't release the cache page because * it could make memory pressure worse * so we simply return NULL here. */ return NULL; } if (unlikely(!__page_frag_cache_reload(nc, gfp_mask))) return NULL; nc->pagecnt_bias--; nc->remaining -= fragsz; return encoded_page_address(nc->encoded_va); } nc->pagecnt_bias--; nc->remaining = remaining - fragsz; return encoded_page_address(encoded_va) + (page_frag_cache_page_size(encoded_va) - remaining); }