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 0CCCFC3DA70 for ; Tue, 30 Jul 2024 15:12:49 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 99D0D6B00A7; Tue, 30 Jul 2024 11:12:48 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 94CC76B00A8; Tue, 30 Jul 2024 11:12:48 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 83BB36B00A9; Tue, 30 Jul 2024 11:12:48 -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 648AB6B00A7 for ; Tue, 30 Jul 2024 11:12:48 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id DC777402B0 for ; Tue, 30 Jul 2024 15:12:47 +0000 (UTC) X-FDA: 82396761174.27.E2B2873 Received: from mail-pf1-f182.google.com (mail-pf1-f182.google.com [209.85.210.182]) by imf15.hostedemail.com (Postfix) with ESMTP id C9D5FA0011 for ; Tue, 30 Jul 2024 15:12:45 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=PvUMM5It; spf=pass (imf15.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.210.182 as permitted sender) smtp.mailfrom=alexander.duyck@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1722352338; a=rsa-sha256; cv=none; b=KrujqY9d/l/2hoOgFHz6piDSBl6zWSuadtI6P74zNyUePBfZDw/2d1KHnEYkP6ScoIsSWz /XZURG+q1NMl0WWAU7b4FHiq/iXWxhNVLG6JNmiSqbDefmVW/CJ9k2vdBuBHILcD5twd+1 3ylvfixI+fA6dGlu419OPoeQ92Gg3mY= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=PvUMM5It; spf=pass (imf15.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.210.182 as permitted sender) smtp.mailfrom=alexander.duyck@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1722352338; 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=GVugQl+eanMaRvw5CudOTZB+XSH5eb6DxiOKQnVK1ZE=; b=dbV6QkXqIdTU/MuWPKztEDQBI5E4+O0YqMzIk0FMwz1rHzxQ3JDF0/erBYJve7ZaDCRq5w 48CV1U2WkloyUT73bZPVHFBNZk/xzNW2ojiuVF5P0sGoUNIF30VQ0GIStqBEAqjtmXztgp MMXloesmjl4YMZ5B+wGYHolMhorteks= Received: by mail-pf1-f182.google.com with SMTP id d2e1a72fcca58-70d1a74a43bso3171943b3a.1 for ; Tue, 30 Jul 2024 08:12:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1722352364; x=1722957164; 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=GVugQl+eanMaRvw5CudOTZB+XSH5eb6DxiOKQnVK1ZE=; b=PvUMM5ItgmXKiE5o2n42O5xJ0GqPir9+kGvmOOwKn63nwCewrMTeFTtJJIV+fXwTls mw7yj2smDzqOCDQTUuBs9nLd3RCpTdBNTudyux0VOHkyq5/wQtpgPthKnyRnbiUw2Pbi SaXTOr4af5azYLT5DFCcfngnJQjkHjy7U3ALwlHm5UqQE24kL6TVRanwOMVmwm0StPOu NlSZ7+fgECgTrQWjGx7dqxDa2/zdg8MsbFDqwCArua8lpXoflhj42sSDD84lpn7Xppzq 8z+o3UIgTMc+lnJpr2jILoCDiulWQDOVN7Y1SVb7cnJdPkioic/eKyVyZ3mhs5PnVyce 5L0w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722352364; x=1722957164; 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=GVugQl+eanMaRvw5CudOTZB+XSH5eb6DxiOKQnVK1ZE=; b=mu5zqvSQ0SwX1moUwASrlQ5HPnjVtYaYGM2HBUWCLALY/ik9QFCWyy/lpgOz3TIAVq PSi9Soc/W0Ke+IvNsr/nicsl7zMaVqhZH6+2GOXzUDGM8lN6wonNFqGv9MJN+kIHykH9 TScLib4pV8zroa9HhqvI77smIsAhXuUEou015pMZO9tvCuot8CJLEBQlCfzcv6GNcseG NBRG15qjDHNvQOpbBl319L90AbL0vn3QxE1DzIq1utId/SVdMWrgqnWh+5qvEIUsIP8/ qScUflRHz5Y6jz/ghsBlb891INkz54Jkw4sBt0zC8Y7sQ4iVdtef0I+LpObKQ/HY4LLO 2rHg== X-Forwarded-Encrypted: i=1; AJvYcCULWvXozwpAUHCy55Vh/VdHp3PXWBEDK8JmfUqXUph+A8H8bYhi3TO28ebc1nUNSkgDWK4Q86fEwEu/vfzGX2A1qrY= X-Gm-Message-State: AOJu0YzRmPsWNnr746LDp1NTbkA0g1IVT0DR5n2trywAAQTp6U7SL/1v 2vUVC9zHYrSVCmYEcAWY5Tk1LP1wAE9qOElHlb+LWz3F+S376QAu X-Google-Smtp-Source: AGHT+IG6Wbc+HpZnPJAIkBLRjtEPwFwwp0fU1uZJ4MVK7qA9lP0PQrlK4lJvsis2L2DdAAjdHrrJGQ== X-Received: by 2002:a05:6a00:9141:b0:70d:2583:7227 with SMTP id d2e1a72fcca58-70ece9eba1emr9492005b3a.6.1722352364104; Tue, 30 Jul 2024 08:12:44 -0700 (PDT) Received: from [192.168.0.128] ([98.97.103.43]) by smtp.googlemail.com with ESMTPSA id d2e1a72fcca58-70ead8748a1sm8568598b3a.150.2024.07.30.08.12.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 30 Jul 2024 08:12:42 -0700 (PDT) Message-ID: Subject: Re: [RFC v11 08/14] mm: page_frag: some minor refactoring before adding new API From: Alexander H Duyck To: Yunsheng Lin Cc: davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Andrew Morton , linux-mm@kvack.org Date: Tue, 30 Jul 2024 08:12:42 -0700 In-Reply-To: References: <20240719093338.55117-1-linyunsheng@huawei.com> <20240719093338.55117-9-linyunsheng@huawei.com> <5a0e12c1-0e98-426a-ab4d-50de2b09f36f@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-Stat-Signature: g8wop64ttsqjrdkz3ourtyw6gw69goxm X-Rspamd-Queue-Id: C9D5FA0011 X-Rspam-User: X-Rspamd-Server: rspam10 X-HE-Tag: 1722352365-872358 X-HE-Meta: U2FsdGVkX1+KCElA30gML+/+8L/FZyH7TFpeXcs3b39S7OTPkH9zyijXPCpSX2estp0jedXpwKNP4gsF+6emfgArixIGyjVe1r+hUF0tcagRaZXgASJJi/rv9PBn0WudsOTX+SS6I+HWvsmXbhp0YbUUcW3eDEnC1FdYNTQmJiMmu4kSCdECNuMhR26BmnL+tzLhEOaCC7diFmve0S/3c7aFVClN/s9ggCxfn+22rHYNMEgWXqP0nBWqr5jEhLsTXjYOfN+Fwb4fJg/Ne7JFCgp+9tiYFlcnzFND52/bPFvGPOT7ylspt4sM2ZT4moG2HHXu9jNnynx8nU/x76pmECrLD3uQ+sBY9cmvKLz79qEH/1jLSoemNgGIFaVQAnz/stqCZTlJErLV5Yp42YvJcZyLV/CHRcSOwF8f2DC/EsxOplW5yeA9lYH3NlFCPm45aBK9Y1Vg7uJNpjGxYQ6G5Fy0aoT0cFB/a7p9XIb0b8Xsv9Q2WcHApk24No+kqZX28vzTaClj8QQzR+d5GZPbhUKaODqKv9b6t5p7iIVwN0k2BSnumuIuswm6r8RrCKnXTLysJT0rK3UpZw8XRCtKjb9iXboHBkmh0YTlwewZodVk7rcTr7C/Pvmnrq+YzwIczpg2rekhGLYzXsuZ9wOrw30gJw1OFFomnBOsWSE4fTlRh2w6O1q5uvj+9XA9onflBI3gopusTIJTqzuD81DPb8kDXEVOEjqgQXkUU6JNMeUvLkwijV2V8uhdGXeSx3cBh6MpfvGylj03sCC0lJtO8XWUoHAfGFyRXlKCzl/2S1GKJVNgNCq8h3hQD0itrcxq2uI5REeqNx+wYvCoRMBY3Y6XXtEUpj9xzZxQh/TwHVtxRtQ2TGp0dUBNGCknZbkFm9Hkaqo+7KDuZkTgSZmJ2DM/j1BPJcSy5agUdj8H0F72JE7nxoSLAa39u1a6gZm6AtqCzO2nPRlLLXUt0Ec lGqnOAi0 CKO90FrVRJ1LppKlUlR7FEJKcjjx12x8PM99j7jCkt/0+dVg0PiEbOW7VHj9eikSjdvTh7+07xbOwGzaUC0HqMrVaIjDi8s6VCPFAH+iZXg1/0PTpl6JtJ/7fW+7GdvXpO6iCoHzG8S/Wvo5zBlQkVSPVCWF4i0tfpF2O/cRRrAbhJP1iVrzJPP97aDm0+j1rGTx3H0qAkE+lmjV5iiH/H0Fs9itO3aDs6qmKv2wUHiIxovXDUE+uZskUJmqDL+clgrwNe4X1YoRJayhnzcl3mhwZfhJheBGjbI+jkUc9CawtIKzcnhUuWRCc9ur3xoX+WB6dn0h8VG8fwC+AsGH2w98FF2XR9nGwnjHOg4ckFpXiWF14kGy+PiZdSMivvULOUcoL/Ew8mYKmf1bOTud1YDH3UUQsMpXWes5g 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-07-30 at 21:20 +0800, Yunsheng Lin wrote: > On 2024/7/23 21:19, Yunsheng Lin wrote: > > >=20 ... > > > 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 yo= u > > > 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. > >=20 > > I am assuming you would like to reserve old behavior as below? > >=20 > > if(!encoded_va) { > > refill: > > __page_frag_cache_refill() > > } > >=20 > >=20 > > if(remaining < fragsz) { > > if(!__page_frag_cache_recharge()) > > goto refill; > > } > >=20 > > As we are adding new APIs, are we expecting new APIs also duplicate > > the above pattern? > >=20 > > >=20 >=20 > 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. This is better. Still though with the code getting so spread out we probably need to start adding more comments to explain things. > static struct page *__page_frag_cache_reuse(unsigned long encoded_va, > unsigned int pagecnt_bias) > { > struct page *page; >=20 > page =3D virt_to_page((void *)encoded_va); > if (!page_ref_sub_and_test(page, pagecnt_bias)) > return NULL; >=20 > if (unlikely(encoded_page_pfmemalloc(encoded_va))) { > VM_BUG_ON(compound_order(page) !=3D > encoded_page_order(encoded_va)); This VM_BUG_ON here makes no sense. If we are going to have this anywhere it might make more sense in the cache_refill case below to verify we are setting the order to match when we are generating the encoded_va. > free_unref_page(page, encoded_page_order(encoded_va)); > return NULL; > } >=20 > /* OK, page count is 0, we can safely set it */ > set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1); > return page; Why are you returning page here? It isn't used by any of the callers. We are refilling the page here anyway so any caller should already have access to the page since it wasn't changed. > } >=20 > static struct page *__page_frag_cache_refill(struct page_frag_cache *nc, > gfp_t gfp_mask) > { > unsigned long order =3D PAGE_FRAG_CACHE_MAX_ORDER; > struct page *page =3D NULL; > gfp_t gfp =3D gfp_mask; >=20 > #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE) > gfp_mask =3D (gfp_mask & ~__GFP_DIRECT_RECLAIM) | __GFP_COMP | > __GFP_NOWARN | __GFP_NORETRY | __GFP_NOMEMALLOC; > page =3D alloc_pages_node(NUMA_NO_NODE, gfp_mask, > PAGE_FRAG_CACHE_MAX_ORDER); I suspect the compliler is probably already doing this, but we should probably not be updating gfp_mask but instead gfp since that is our local variable. > #endif > if (unlikely(!page)) { > page =3D alloc_pages_node(NUMA_NO_NODE, gfp, 0); > if (unlikely(!page)) { > memset(nc, 0, sizeof(*nc)); > return NULL; > } >=20 > order =3D 0; > } >=20 > nc->encoded_va =3D encode_aligned_va(page_address(page), order, > page_is_pfmemalloc(page)); >=20 > /* 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); >=20 > return page; Again, returning page here doesn't make much sense. You are better off not exposing internals as you have essentially locked the page down for use by the frag API so you shouldn't be handing out the page directly to callers. > } >=20 > static struct page *__page_frag_cache_reload(struct page_frag_cache *nc, > gfp_t gfp_mask) > { > struct page *page; >=20 > if (likely(nc->encoded_va)) { > page =3D __page_frag_cache_reuse(nc->encoded_va, nc->page= cnt_bias); > if (page) > goto out; > } >=20 > page =3D __page_frag_cache_refill(nc, gfp_mask); > if (unlikely(!page)) > return NULL; >=20 > out: > nc->remaining =3D page_frag_cache_page_size(nc->encoded_va); > nc->pagecnt_bias =3D PAGE_FRAG_CACHE_MAX_SIZE + 1; > return page; Your one current caller doesn't use the page value provided here. I would recommend just not bothering with the page variable until you actually need it. > } >=20 > 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 =3D nc->encoded_va; > unsigned int remaining; >=20 > remaining =3D nc->remaining & align_mask; > if (unlikely(remaining < fragsz)) { You might as well swap the code paths. It would likely be much easier to read the case where you are handling remaining >=3D fragsz in here rather than having more if statements buried within the if statement. With that you will have more room for the comment and such below. > if (unlikely(fragsz > PAGE_SIZE)) { > /* > * The caller is trying to allocate a fragment > * with fragsz > PAGE_SIZE but the cache isn't bi= g > * 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; > } >=20 > if (unlikely(!__page_frag_cache_reload(nc, gfp_mask))) > return NULL; This is what I am talking about in the earlier comments. You go to the trouble of returning page through all the callers just to not use it here. So there isn't any point in passing it through the functions. >=20 > nc->pagecnt_bias--; > nc->remaining -=3D fragsz; >=20 > return encoded_page_address(nc->encoded_va); > } >=20 > nc->pagecnt_bias--; > nc->remaining =3D remaining - fragsz; >=20 > return encoded_page_address(encoded_va) + > (page_frag_cache_page_size(encoded_va) - remaining); Parenthesis here shouldn't be needed, addition and subtractions operations can happen in any order with the result coming out the same.