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 A3B9AC3064D for ; Tue, 2 Jul 2024 15:30:22 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 401E86B00A0; Tue, 2 Jul 2024 11:30:22 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3B1E46B00A2; Tue, 2 Jul 2024 11:30:22 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 27A376B00A3; Tue, 2 Jul 2024 11:30:22 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 073BA6B00A0 for ; Tue, 2 Jul 2024 11:30:22 -0400 (EDT) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 9BB98801BD for ; Tue, 2 Jul 2024 15:30:21 +0000 (UTC) X-FDA: 82295199042.30.7548536 Received: from mail-pl1-f173.google.com (mail-pl1-f173.google.com [209.85.214.173]) by imf25.hostedemail.com (Postfix) with ESMTP id 9DDFAA0025 for ; Tue, 2 Jul 2024 15:30:19 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=h27EFNAI; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf25.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.214.173 as permitted sender) smtp.mailfrom=alexander.duyck@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1719934194; 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:dkim-signature; bh=9WziBX23oZK9QDnqDHzmJlEPl4STCd/KkEbB1uUJ7YU=; b=8pq9aU9J9WVnmMGNUdZmemEQ2tgtxndBMWmrWnK+76ph5zTrOjXMTQ+KaJSKmDSUjyP73V WhduDm6pMocrHTOw9QrgRsQXgMbPsmAcoTFg+Ba2GQh9zCIfh+YhhGy2pLKNEcfWsQOFQv T1zDZrH9HMNnSneILzCqoYGUH1haF2M= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1719934194; a=rsa-sha256; cv=none; b=hU606WPDaeTeWFIYgpjtabOAnk0Fi6FyfZgL/D65cl6lKJKymcv8iofzgwekfwfjxSXwZQ HzAqaeHWmyuS+3Y0sMIeM90HtGke75VRnStxdv7SPDDTVpJa6bVAkEqHl3pqOn3HrbbRix oWWnT7vljEWPEYrtt7IowpMckPdZjR8= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=h27EFNAI; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf25.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.214.173 as permitted sender) smtp.mailfrom=alexander.duyck@gmail.com Received: by mail-pl1-f173.google.com with SMTP id d9443c01a7336-1f64ecb1766so24081405ad.1 for ; Tue, 02 Jul 2024 08:30:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1719934218; x=1720539018; darn=kvack.org; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:from:to:cc:subject :date:message-id:reply-to; bh=9WziBX23oZK9QDnqDHzmJlEPl4STCd/KkEbB1uUJ7YU=; b=h27EFNAIQEws/iuzBxC/ZVvmHPBIjcCDpT4mSJ8au2vcXDklhY5j6ONAt7ZNDxm0dE i4vLZW7FJjWG51yYn+HbmP6LLOXt47rrfw1aUywNntUdOEBeVuyiyVTwaP6zQCOSuRGD qrhY3Hk5wV0kSUrcjB0b+T9ijFIKbGr0BD3MtsXoly1yjhgzIOfLt05HTN/k9SuiwBCk UMrov7oLFVpE34xVowYRtR85nHZ017XSCCjFsGYv+nWNsxBPQPhNqD6X0JV0Y0Y2R9Jd /YM93Mc2mbYSqzS7VzqbBZXbMv4itAhCj7V5WJ/hKOMh2WbDMNxLIoRa7VHB1afNlayN 2aXg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719934218; x=1720539018; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=9WziBX23oZK9QDnqDHzmJlEPl4STCd/KkEbB1uUJ7YU=; b=Q70LyG2FDnWdEX3GKIUqcnuU3QItwBdiYVAizcDTFMYdkrVw4YanLkhMJLDab6G+ix ABgCELP7vDUITvJoYDRiCbmwFgm6cEYxhx+Cz2L28Y1f1jS77zo8hpUZm9Zd7dynZlpg bJaAiVo5dMEAx5DcbGOxRuSE98KoaiuAfZ4qKa/TrYjK12BrrpILLPqyU0DR8eu3lkPr FwZdX1NXRSUeEk5LFfwWifPR9mc5rvEPFkKfHIs1WJ2xLOFtJQYqFXiWxZWZh69Sa8Ap meq5sctJocSy1qbrduFdAcqHGgrtJLP+QZfnj8m/l47bLfOUCR+2++whOmqwQ6MDsxyN lO1g== X-Forwarded-Encrypted: i=1; AJvYcCW0j/+w4uqPcACus6AXJiJGFgABk6IYBwUhL8jQ5TLa3xErh4N3QAnCtjNtv8ctKWRD/mG1UWquWTXaAcdKpAsIZsk= X-Gm-Message-State: AOJu0YyXI9lgm9F0v0N21/zGslMxbjnQYqmcg5BZma1Wwr7ES+Xz8L0s HWW5pJYcGS1IBpqf5AVWsDzASNjydA2bjBj20hzvQvuKczxMee4r X-Google-Smtp-Source: AGHT+IEo6xTMHEJQfZVZ4CqyWceq1noeoaurNROb6sO/XV/kvW5mqIJE7/2OugE/CjbTXef4Lhkx0g== X-Received: by 2002:a17:903:32cb:b0:1f4:620b:6a27 with SMTP id d9443c01a7336-1fadbc5b950mr59205545ad.13.1719934217867; Tue, 02 Jul 2024 08:30:17 -0700 (PDT) Received: from ?IPv6:2605:59c8:829:4c00:82ee:73ff:fe41:9a02? ([2605:59c8:829:4c00:82ee:73ff:fe41:9a02]) by smtp.googlemail.com with ESMTPSA id d9443c01a7336-1fac10e22c1sm85831715ad.72.2024.07.02.08.30.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 Jul 2024 08:30:17 -0700 (PDT) Message-ID: Subject: Re: [PATCH net-next v9 07/13] mm: page_frag: some minor refactoring before adding new API From: Alexander H Duyck To: Yunsheng Lin , davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Andrew Morton , linux-mm@kvack.org Date: Tue, 02 Jul 2024 08:30:16 -0700 In-Reply-To: <20240625135216.47007-8-linyunsheng@huawei.com> References: <20240625135216.47007-1-linyunsheng@huawei.com> <20240625135216.47007-8-linyunsheng@huawei.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) MIME-Version: 1.0 X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 9DDFAA0025 X-Stat-Signature: 9fdgg1pxuzeusaqs6nte9143esjrjsp6 X-Rspam-User: X-HE-Tag: 1719934219-807573 X-HE-Meta: U2FsdGVkX1+FXvnr5owY5lSJp8GHCXFLAyXAcyxYUAewbj0w/CinB31ydeYiJNnuhPJ29AVEzVZ8jsbcM6wFXkcDxMgilwDtzR5TKR/Sr2m8aT1mX+BRdDRUlhAbfTUQ5KM9H8LgdtkeFzjy//xrDoOSHCSORtkyDBzlis3gPPtR+6/oqRJLa1VvcUaf+VqTqHQ7A+PHH02Vw9t/g2IMKaUC8wj1PNo9w+g1Zv7/WcFwWfxviodCWrl+SzN0wmgY/Pa0Sy2kQKfLcqNuV7hWodh55prwoHdPhYkUuyWf7PtvmAoAmD7uckGHIaxRJbwDRstCp8W144LZhLPa4pgeFluQhsCeFIHIozteQ4OB3mj7HaTQd8ynriurVGa+mfdLyiq/sHRwmxUtVHLaa9SoL1DNyLg+ZbkagIncNx/sOcOkErqOfa6SoRtxQFv4P+fI5A4qFl2wDxAWefvtmbTogzin8Dzt9TRJVnIAU7RSrRiFDcTZlEQeV5hHoisM1LPSlfoU+aZgOqIC8a9oaDBW3C2hoPR4toGfrxKJ7vADFiPxSfqUxUTnrVN5MkY4PUHP7LEBpwy08rXFRY02+/7XdMp6k20xFkB5JoqUqYpF6KD1HJe5EfL7wXZGs1DzPeRrlx1VRxaDhguROy/WXkJqF8d+ttW6C+Es07ghZHDU/Q3ajLoHr9GuwoGnc1d2dJk3wj9+R0e1XzCY82Xssuj3OU6qBFJ1V2oqmPbMlkULauUWWl22iC40S6E07wWVzom9pbAxG1STj3evvFnwC467Fhy56Eo8M28aAVF8E7AtWeh7TZIqgDWPQtQT1XV/i/bkzpnwLN6mqI1ojORV8z9q9RmKNlbq2cTDu9y5bmovq8pikkaIif1fzLK+By7HBp96ABTex/VumeY3q3uwYhFEGp4u5s9j3KkOpKGbI5EJkV5zND699GPcxjwG8S9aTfrBVf/bjb1o9ozknYUPhI6 RnEZSrQL Km5sKhxa+EzhrihRQqpkpmkI0Lfqf9OM1IMa6Aqp7Se1fN8lYMkguRUUWCXrfYtB6UKxixp3psQ9v+KFQ194dKa+ix8AbP2U2CN/IApPQuFzvAiGUW91ggrbruULHM9BbRFrV7kln9XrSwBUdCYbykv90EvuGT8pWmpGi+yjaidmlYP5g0cwVQ3Z5Hy7RQWpeSyNiK56dVIYo2d0zbrY8YZ3ZCXgzemdZPdxgduGZnBAw2PTgWejnQNIVTs55cJonNxwOZYm3axI5FOglPxxaClsckceDiTnIIMKiboiBbhywlmqW8RVhOMp/lzD80D73aKZYde6KrEbQGoL7o4yKRbXYbizBAtqggH98heG77d9poOUxXEl+2qy6q7r1NOazdz7vCWRJQUl0eoIes7Q8RWz8PR6RKapyPk6JLIdGgpgDmxjxWylhA4w8Zoto28hqJGgf 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 Tue, 2024-06-25 at 21:52 +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. >=20 > CC: Alexander Duyck > Signed-off-by: Yunsheng Lin I am generally not a fan of the concept behind this patch. I really think we should keep the page_frag_cache_refill function to just allocating the page, or in this case the encoded_va and populating only that portion of the struct. > --- > mm/page_frag_cache.c | 61 ++++++++++++++++++++++---------------------- > 1 file changed, 31 insertions(+), 30 deletions(-) >=20 > diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c > index a3316dd50eff..4fd421d4f22c 100644 > --- a/mm/page_frag_cache.c > +++ b/mm/page_frag_cache.c > @@ -29,10 +29,36 @@ static void *page_frag_cache_current_va(struct page_f= rag_cache *nc) > static struct page *__page_frag_cache_refill(struct page_frag_cache *nc, > gfp_t gfp_mask) > { > - struct page *page =3D NULL; > + struct encoded_va *encoded_va =3D nc->encoded_va; > gfp_t gfp =3D gfp_mask; > unsigned int order; > + struct page *page; > + > + if (unlikely(!encoded_va)) > + goto alloc; > + > + page =3D virt_to_page(encoded_va); > + if (!page_ref_sub_and_test(page, nc->pagecnt_bias)) > + goto alloc; > + > + if (unlikely(encoded_page_pfmemalloc(encoded_va))) { > + VM_BUG_ON(compound_order(page) !=3D > + encoded_page_order(encoded_va)); > + free_unref_page(page, encoded_page_order(encoded_va)); > + goto alloc; > + } > + > + /* OK, page count is 0, we can safely set it */ > + set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1); Why not just make this block of code a function onto itself? You put an if statement at the top that essentially is just merging two functions into one. Perhaps this logic could be __page_frag_cache_recharge which would return an error if the page is busy or the wrong type. Then acting on that you could switch to the refill attempt. Also thinking about it more the set_page_count in this function and page_ref_add in the other can probably be merged into the recharge and refill functions since they are acting directly on the encoded page and not interacting with the other parts of the page_frag_cache. > + > + /* reset page count bias and remaining of new frag */ > + nc->pagecnt_bias =3D PAGE_FRAG_CACHE_MAX_SIZE + 1; > + nc->remaining =3D page_frag_cache_page_size(encoded_va); These two parts are more or less agnostic to the setup and could be applied to refill or recharge. Also one thought occurs to me. You were encoding "order" into the encoded VA. Why use that when your choices are either PAGE_FRAG_CACHE_MAX_SIZE or PAGE_SIZE. It should be a single bit and doesn't need to be a fully byte to store that. That would allow you to reduce this down to just 2 bits, one for pfmemalloc and one for max order vs order 0. > + > + return page; > =20 > +alloc: > + page =3D NULL; > #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE) > gfp_mask =3D (gfp_mask & ~__GFP_DIRECT_RECLAIM) | __GFP_COMP | > __GFP_NOWARN | __GFP_NORETRY | __GFP_NOMEMALLOC; > @@ -89,40 +115,15 @@ void *__page_frag_alloc_va_align(struct page_frag_ca= che *nc, > unsigned int fragsz, gfp_t gfp_mask, > unsigned int align_mask) > { > - struct encoded_va *encoded_va =3D nc->encoded_va; > - struct page *page; > - int remaining; > + int remaining =3D nc->remaining & align_mask; > void *va; > =20 > - if (unlikely(!encoded_va)) { > -refill: > - if (unlikely(!__page_frag_cache_refill(nc, gfp_mask))) > - return NULL; > - > - encoded_va =3D nc->encoded_va; > - } > - > - remaining =3D nc->remaining & align_mask; > remaining -=3D fragsz; > if (unlikely(remaining < 0)) { I see, so this is why you were using the memset calls everywhere. > - page =3D virt_to_page(encoded_va); > - if (!page_ref_sub_and_test(page, nc->pagecnt_bias)) > - goto refill; > - > - if (unlikely(encoded_page_pfmemalloc(encoded_va))) { > - VM_BUG_ON(compound_order(page) !=3D > - encoded_page_order(encoded_va)); > - free_unref_page(page, encoded_page_order(encoded_va)); > - goto refill; > - } > - > - /* OK, page count is 0, we can safely set it */ > - set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1); > + if (unlikely(!__page_frag_cache_refill(nc, gfp_mask))) > + return NULL; > =20 > - /* reset page count bias and remaining of new frag */ > - nc->pagecnt_bias =3D PAGE_FRAG_CACHE_MAX_SIZE + 1; > - nc->remaining =3D remaining =3D page_frag_cache_page_size(encoded_va); > - remaining -=3D fragsz; > + remaining =3D nc->remaining - fragsz; > if (unlikely(remaining < 0)) { > /* > * The caller is trying to allocate a fragment