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 C14B6D3DEA0 for ; Fri, 18 Oct 2024 17:27:28 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4D57E6B0088; Fri, 18 Oct 2024 13:27:28 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 45E1D6B008A; Fri, 18 Oct 2024 13:27:28 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2D8286B008C; Fri, 18 Oct 2024 13:27:28 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 0EFA46B0088 for ; Fri, 18 Oct 2024 13:27:28 -0400 (EDT) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 6F731A86B4 for ; Fri, 18 Oct 2024 17:27:03 +0000 (UTC) X-FDA: 82687404282.15.64F1FBA Received: from mail-wr1-f48.google.com (mail-wr1-f48.google.com [209.85.221.48]) by imf12.hostedemail.com (Postfix) with ESMTP id 6467040020 for ; Fri, 18 Oct 2024 17:27:20 +0000 (UTC) Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="RXXRCL/h"; spf=pass (imf12.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.221.48 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=1729272371; 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=92JIeKcyj6DKUiC6YryG10zMLa3wiMB0RTYm/202bFU=; b=jKll7Mu+mKA1nzP63zyvsN9Ij3mLH5UJ38RdNQzjLRf/ldGA/TQK327zypXLBRzFuXbXga Za9I/nBq6zl7CgC/yTI2+FweYhjYbX9gWzwydbcKIjZKIVdkGPQK0M9NTvRclpuy/upCgf qFUwe1mtgr29RyxjT61i4TEbYtRHiBY= ARC-Authentication-Results: i=1; imf12.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="RXXRCL/h"; spf=pass (imf12.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.221.48 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=1729272371; a=rsa-sha256; cv=none; b=hUgsyVZ/tzU/z9kIyVsVz/NOXX/LKnEdSN12sJBmRBCsbU3tbuOlQBrDNhjCAt9Qu3Ao+x L82O8CwQAuw0SChSLT6gvnO5USJEIJZyIwWpclUeI/ogKSXeHdOi3xk0lA7e1WqMMd8047 SbLEm9wR7aAkGdG2T36OccElm8+Igao= Received: by mail-wr1-f48.google.com with SMTP id ffacd0b85a97d-37d495d217bso2228951f8f.0 for ; Fri, 18 Oct 2024 10:27:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1729272444; x=1729877244; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=92JIeKcyj6DKUiC6YryG10zMLa3wiMB0RTYm/202bFU=; b=RXXRCL/hUKuOy8ks+W8JeXMjdSwY6yN5wdfoxxBwaWGjop9VjOkWWAn5djarMWLFuq PFQqN5qrjfX8eRYQQ2RY7IIlwEmtEPd1mTrCxh71wFrprcLlnaGKKWojb1lRwq2GGVOO BdWyBu1X1uHHv/m4ZkhgNVx5TbzrslOTGWo7m8i5m/gFLbd6FDajbf5tTObzPTYb5dWk c6i3uV8S28dXKlg/lXxnL+csFqPntidJRRodMxZ4y80ec+XSslqUVGtzpFF9uvNrr7At GZLEH2MCVUr0hKNBzREvjegnduaDRZVZqq3x7c/5iSTEAhYVbFgvg+a6wVmeK8jmuScH Y5/A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729272444; x=1729877244; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=92JIeKcyj6DKUiC6YryG10zMLa3wiMB0RTYm/202bFU=; b=Qzyqtgw2Gefe1zpKDKHDAJ4BTzWotnsZTj0SxB0EwSRMnyOaqKKQovdacvb+yAXG3u /CDROqv1hO2E+z1re/DK0ppT6tOSBZY3aR6rruklgyeXq2osHpKnaUMUI2XygwL0w3CO pFUAtPG/vqnl39VKnMwaGHV+8ckBVikx9LmS50tOur54oSsiYyEU2SRpETxFt64kqQLQ MKrz4jgkFD7kabRIPCY7MvuIAGgQQ6nSeK6vZ53I7cskcq2OQvjLHsUxXnG0rRslJLLg 5XqCrvAv9n2+KTV1qlBD+/0p0iUwf9VaKLZUGUo8icVbSVpJx0ljjbfm1MQoLBNz2Oea 8g0Q== X-Forwarded-Encrypted: i=1; AJvYcCUVTkFevZDmNeEBFZMtFaogfC9If5V32eoa1Ds/Ku4x7nB4kibIFPgtgRyPA/zjq9vp15vuoPxYlg==@kvack.org X-Gm-Message-State: AOJu0YyUjv+nnAEuwwzOZ8TPjugqgm2BEP9Vv5Bvi/IajAUC7zn8CUc6 0AkIp6h0cEvKxkoCYvBP8hYGDgySlp/A1SVBee7Qi0tpE62JTb8WAaoeWVdbkLMC/i3+fXE0fZt 4waY3cciVPUoNna8Li4DFnbUPiuUjhA== X-Google-Smtp-Source: AGHT+IGys2BLDgm2mjdEHNZ6Cc0H/aMWsKZJ9DL+zpQUKfQJKUmosmgEMX9Iuu36JZigYsTfGl8dSriYMsqkkLs2r2s= X-Received: by 2002:a5d:4308:0:b0:37d:4619:f975 with SMTP id ffacd0b85a97d-37ea21d8b00mr2875211f8f.19.1729272443899; Fri, 18 Oct 2024 10:27:23 -0700 (PDT) MIME-Version: 1.0 References: <20241018105351.1960345-1-linyunsheng@huawei.com> <20241018105351.1960345-8-linyunsheng@huawei.com> In-Reply-To: <20241018105351.1960345-8-linyunsheng@huawei.com> From: Alexander Duyck Date: Fri, 18 Oct 2024 10:26:47 -0700 Message-ID: Subject: Re: [PATCH net-next v22 07/14] mm: page_frag: some minor refactoring before adding new API 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam03 X-Rspam-User: X-Rspamd-Queue-Id: 6467040020 X-Stat-Signature: oy1115u5stqw4jqxmkcmbwqwntcf8ubr X-HE-Tag: 1729272440-936809 X-HE-Meta: U2FsdGVkX1/cVqgddkSH+j4wjmYMpuI90CwHQ6keU9Hg90C9BsH8N7ByZABSdU81XXW7C5lXiCBY7nqapbD1S5uCi77AdkRrFVgzrmwBqxmGZMtJLEo8eDeIQ41xF58k84YIFA0Y2Z7VpzXMtenVOGlSQlHkMWvAA5iNGqQ30X7bm/fmZ0g7erA+AnASxMQ0VtAR7iDHY15oXUkO66lI/bfsuAWYytiQoYQ3/mg7JvwkEymTmRww46EShiPkiTukHCZY53MdEOEI+EdIoxZjeQ/YfbgrdOejuwwi52C5JZCMNhCvBGtxBbRBfdJTSLnB9yPYv8GHBMMIggRSnYHiMeMajZDUgs6zLiLp425SZkq8bLYGx0+sX4Vu4lCbQQSsl6E+TnDVlu58FtEwq4OiLMlhurHs2Bgm9el53iLMAehsGXeHMg1qrDmQeHfQ2wHyg4sWu/kabVTn5VcEge47JywjpsZmECeWzqsi43wCGmN6bPpY209FfY36Y+3AVCUQda5JDtvDxe/HpmeEinmN1WOsVeK+mx3FH1ACACtMRf3oyE2ylbkc6RdJK95cQX9RNPgTo58bFfR4VIUmzxQeS25XBPiH4kfQXGf8SWO8z6uHpcIOb9yga4VFH9FTOnxWYEN5sUzrIydZbCKg6/wnjc5mjpw/LdsKhiufyIc6vuUg+rweiHsvOcBXKbwj7LpMzDjXJ4sV7NPdt1Ly4qJG/o0oUiL273Yu+/FEBdrRdS8wg/HeYH2hXduEERECNTkU/ICrnyveDgIrsJyDPtiwtNygcsgr1fiiYLcKM+Hcrms0Eqd2b5dLmdH4OTskV6Ip0VC4psxPILJ3t1GL0BGl7Lp1yQh/liB03cbE7q9343XHw9f4G0XBox9RbGWNPLRZwkyAc1ejXOnnhkJDrKcpehXsaIPZ4kCyf8U3cm9FeuiXCZIujcpJmboAloGlOZsWkSsouwH2/SEc6D4WGT/ cGEN/jtA 1FMBOWScGnNta8/OZCVaIzf8ElZj/WOv8tMXMtahJcxQdZ9GlqINhWNv4DumfVgaQ4XE1T+EcgkjALYM+B6d3XZ1rh4kw4k2dxjGbDwi7ZumzbOJdv7yrozyH5Up1PHGwMT38dC9uDQ1JWzWIZLxL8BLERV6q74YniXtNflc0k+CfNetOklF2wn/8k0Sfqqu7edoWd+8/keuJVTXlfzQWmU8Ph/+DXDswmvXFc4txqazDr17m7NHeJUE4i9G6yqVLYisxHVFYnFDc91R8Wz8FrjgwOlaXPXzXxQd1B6qctuqn1Xz9WuB8Qp12lwG5cJDioV9v93wlMshAyE1hj07TdiJ93WppNNcrJt6hui76HOxmKrKBZSlstIK8j86ketaX5D7NxcsYiw1NSCOv9teLGLntdmJM/oWzTTZ9y5iD/AoxZtfQG4JflWgGD92v+8E1h7Iv 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 Fri, Oct 18, 2024 at 4:00=E2=80=AFAM Yunsheng Lin wrote: > > Refactor common codes from __page_frag_alloc_va_align() to > __page_frag_cache_prepare() and __page_frag_cache_commit(), > so that the new API can make use of them. > > CC: Alexander Duyck > Signed-off-by: Yunsheng Lin > --- > include/linux/page_frag_cache.h | 36 +++++++++++++++++++++++++++-- > mm/page_frag_cache.c | 40 ++++++++++++++++++++++++++------- > 2 files changed, 66 insertions(+), 10 deletions(-) > > diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_ca= che.h > index 41a91df82631..feed99d0cddb 100644 > --- a/include/linux/page_frag_cache.h > +++ b/include/linux/page_frag_cache.h > @@ -5,6 +5,7 @@ > > #include > #include > +#include > #include > #include > > @@ -39,8 +40,39 @@ static inline bool page_frag_cache_is_pfmemalloc(struc= t page_frag_cache *nc) > > void page_frag_cache_drain(struct page_frag_cache *nc); > void __page_frag_cache_drain(struct page *page, unsigned int count); > -void *__page_frag_alloc_align(struct page_frag_cache *nc, unsigned int f= ragsz, > - gfp_t gfp_mask, unsigned int align_mask); > +void *__page_frag_cache_prepare(struct page_frag_cache *nc, unsigned int= fragsz, > + struct page_frag *pfrag, gfp_t gfp_mask, > + unsigned int align_mask); > +unsigned int __page_frag_cache_commit_noref(struct page_frag_cache *nc, > + struct page_frag *pfrag, > + unsigned int used_sz); > + > +static inline unsigned int __page_frag_cache_commit(struct page_frag_cac= he *nc, > + struct page_frag *pfr= ag, > + unsigned int used_sz) > +{ > + VM_BUG_ON(!nc->pagecnt_bias); > + nc->pagecnt_bias--; > + > + return __page_frag_cache_commit_noref(nc, pfrag, used_sz); > +} > + > +static inline void *__page_frag_alloc_align(struct page_frag_cache *nc, > + unsigned int fragsz, gfp_t gf= p_mask, > + unsigned int align_mask) > +{ > + struct page_frag page_frag; > + void *va; > + > + va =3D __page_frag_cache_prepare(nc, fragsz, &page_frag, gfp_mask= , > + align_mask); > + if (unlikely(!va)) > + return NULL; > + > + __page_frag_cache_commit(nc, &page_frag, fragsz); Minor nit here. Rather than if (!va) return I think it might be better to just go with if (likely(va)) __page_frag_cache_commit. > + > + return va; > +} > > static inline void *page_frag_alloc_align(struct page_frag_cache *nc, > unsigned int fragsz, gfp_t gfp_= mask, > diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c > index a36fd09bf275..a852523bc8ca 100644 > --- a/mm/page_frag_cache.c > +++ b/mm/page_frag_cache.c > @@ -90,9 +90,31 @@ void __page_frag_cache_drain(struct page *page, unsign= ed int count) > } > EXPORT_SYMBOL(__page_frag_cache_drain); > > -void *__page_frag_alloc_align(struct page_frag_cache *nc, > - unsigned int fragsz, gfp_t gfp_mask, > - unsigned int align_mask) > +unsigned int __page_frag_cache_commit_noref(struct page_frag_cache *nc, > + struct page_frag *pfrag, > + unsigned int used_sz) > +{ > + unsigned int orig_offset; > + > + VM_BUG_ON(used_sz > pfrag->size); > + VM_BUG_ON(pfrag->page !=3D encoded_page_decode_page(nc->encoded_p= age)); > + VM_BUG_ON(pfrag->offset + pfrag->size > > + (PAGE_SIZE << encoded_page_decode_order(nc->encoded_pag= e))); > + > + /* pfrag->offset might be bigger than the nc->offset due to align= ment */ > + VM_BUG_ON(nc->offset > pfrag->offset); > + > + orig_offset =3D nc->offset; > + nc->offset =3D pfrag->offset + used_sz; > + > + /* Return true size back to caller considering the offset alignme= nt */ > + return nc->offset - orig_offset; > +} > +EXPORT_SYMBOL(__page_frag_cache_commit_noref); > + I have a question. How often is it that we are committing versus just dropping the fragment? It seems like this approach is designed around optimizing for not commiting the page as we are having to take an extra function call to commit the change every time. Would it make more sense to have an abort versus a commit? > +void *__page_frag_cache_prepare(struct page_frag_cache *nc, unsigned int= fragsz, > + struct page_frag *pfrag, gfp_t gfp_mask, > + unsigned int align_mask) > { > unsigned long encoded_page =3D nc->encoded_page; > unsigned int size, offset; > @@ -114,6 +136,8 @@ void *__page_frag_alloc_align(struct page_frag_cache = *nc, > /* reset page count bias and offset to start of new frag = */ > nc->pagecnt_bias =3D PAGE_FRAG_CACHE_MAX_SIZE + 1; > nc->offset =3D 0; > + } else { > + page =3D encoded_page_decode_page(encoded_page); > } > > size =3D PAGE_SIZE << encoded_page_decode_order(encoded_page); This makes no sense to me. Seems like there are scenarios where you are grabbing the page even if you aren't going to use it? Why? I think you would be better off just waiting to the end and then fetching it instead of trying to grab it and potentially throw it away if there is no space left in the page. Otherwise what you might do is something along the lines of: pfrag->page =3D page ? : encoded_page_decode_page(encoded_page); > @@ -132,8 +156,6 @@ void *__page_frag_alloc_align(struct page_frag_cache = *nc, > return NULL; > } > > - page =3D encoded_page_decode_page(encoded_page); > - > if (!page_ref_sub_and_test(page, nc->pagecnt_bias)) > goto refill; > > @@ -148,15 +170,17 @@ void *__page_frag_alloc_align(struct page_frag_cach= e *nc, > > /* reset page count bias and offset to start of new frag = */ > nc->pagecnt_bias =3D PAGE_FRAG_CACHE_MAX_SIZE + 1; > + nc->offset =3D 0; > offset =3D 0; > } > > - nc->pagecnt_bias--; > - nc->offset =3D offset + fragsz; > + pfrag->page =3D page; > + pfrag->offset =3D offset; > + pfrag->size =3D size - offset; I really think we should still be moving the nc->offset forward at least with each allocation. It seems like you end up doing two flavors of commit, one with and one without the decrement of the bias. So I would be okay with that being pulled out into some separate logic to avoid the extra increment in the case of merging the pages. However in both cases you need to move the offset, so I would recommend keeping that bit there as it would allow us to essentially call this multiple times without having to do a commit in between to keep the offset correct. With that your commit logic only has to verify nothing changes out from underneath us and then update the pagecnt_bias if needed. > > return encoded_page_decode_virt(encoded_page) + offset; > } > -EXPORT_SYMBOL(__page_frag_alloc_align); > +EXPORT_SYMBOL(__page_frag_cache_prepare); > > /* > * Frees a page fragment allocated out of either a compound or order 0 p= age. > -- > 2.33.0 >