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 78883C4345F for ; Tue, 16 Apr 2024 16:33:09 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 124936B009A; Tue, 16 Apr 2024 12:33:09 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0D5586B009B; Tue, 16 Apr 2024 12:33:09 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EDF4C6B009C; Tue, 16 Apr 2024 12:33:08 -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 D64E56B009A for ; Tue, 16 Apr 2024 12:33:08 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 8C58240BF8 for ; Tue, 16 Apr 2024 16:33:08 +0000 (UTC) X-FDA: 82015939656.18.1AE3F15 Received: from mail-pf1-f171.google.com (mail-pf1-f171.google.com [209.85.210.171]) by imf28.hostedemail.com (Postfix) with ESMTP id B4982C0021 for ; Tue, 16 Apr 2024 16:33:06 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=LzMjKjYt; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf28.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.210.171 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=1713285186; 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=tK9x7x40WZ7DIzDRnFtv8z/geV5DKL/J2+8lsUGQtTc=; b=TWapsmYeXmj0lgqkH2nQWj6WByL5n8saLhXFU3kSi6Ey/wNVfLU8Ef9aqyZcG3RB1Eefnr roBbFhnOSmLgllfLDJYdSawqs396qrn4zQKTKUCkLGwojim6WE9b+Ft0mazR0S2cUc2i5q Fhcs+cEE4ZHp92rznj/3tkNFCkjTUgY= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=LzMjKjYt; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf28.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.210.171 as permitted sender) smtp.mailfrom=alexander.duyck@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713285186; a=rsa-sha256; cv=none; b=RfXK+YTDmuAcIU/F+qwBbDJXqgNMaGE7SYMC5WCYrMXmZDiP2OTfGLoIUWhZt2mSp/kp5D zc4bFU+YUMwGeoDmSQpBLdBXxio7NM5Ug0jII2qXk+ut5NFmIVtmNrUBZ1Ty5hvEh1yq5i 48oTBg04/mHoKmO7xXrtswyW6HWv1yI= Received: by mail-pf1-f171.google.com with SMTP id d2e1a72fcca58-6ed32341906so4098252b3a.1 for ; Tue, 16 Apr 2024 09:33:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1713285185; x=1713889985; 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=tK9x7x40WZ7DIzDRnFtv8z/geV5DKL/J2+8lsUGQtTc=; b=LzMjKjYtYDfdcvfQSDSyONV+qnN61p5ioHewxRZkxEM5vFci9odhhzau3g4Eju1dpu 4nmsHzxa8xe5vlluRE00UWBhSCm6XUOh4cESZsG2kIqvChyjHkgE36Sg2wn+62lZ/dFP iSAm/10xKJJ75fnzobkbCoT3Phery/bdGranVA6Jcz6KAi8XvXDVTHFqntY+5tjxkJR+ tgJ8pElOK3VVR1klKKOedMxCQ686FblkgSwqnWABIrsHp4Eu0oZ6ZfPAqUok/QSnBbBO +4L+jpQgo2lrH6ObZOsiuCxoVf3oO+1RY366jA8intQFn6VkURqDLJgqdgQNSA/pJbey oDiA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713285185; x=1713889985; 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=tK9x7x40WZ7DIzDRnFtv8z/geV5DKL/J2+8lsUGQtTc=; b=GeleYV1lu3bnUYvCdDuYk+qMrSJ+6jXUrFi050Lyy0Nbp3Vx2L3Fmo6xlHkJOSWFns 043mVpYjdKgBBQGAMPMx1bP4AEreDRPq6akFmZ0xTYNLEqxeOSOrE7UVOblv61p1qjNp 6DaD09rwY7WJU8GJL7poBXUaWQ2imCXnLL2XWSl0lDdjGrGX/8XCu+AMdWUXdHVUX0Y2 WV2sO05IF1/abXlPCV7HuO8hLsHqXPPYL4IvKzjDhoGu57c0WRTBE3TUT3hzENin8XCW 172X9NPRB1Ga4I4+JZWa5feVXx83est9IZr92yn9X10BVjTtA9OQp5fGtUNO2Tc1eDH5 qk5w== X-Forwarded-Encrypted: i=1; AJvYcCV6+wrznOdqUbkfZCjhdsvNLO26cTHQU7v3WZz+y0Yt9MVteYmO2jcynsTWLz2CUnLMZD7c5PkgnAhIShW0txVOaWo= X-Gm-Message-State: AOJu0YyFR7P0zbXwrLB4jBfktQHhuY0BmGEPaAsuyOrJc7Z8HzbKKfu1 xsOx5jji+iOOMozkkZdUWRDMTKi7G+ASk9D+bZB/0xaS9skjEwU4 X-Google-Smtp-Source: AGHT+IFpTA0jTqw9y9iYKwIdPqS5F46X2wkv0G7mkYRbbd5RWB6PG9jrId9b8vMkN48RU8IXHCYhug== X-Received: by 2002:a05:6a00:98d:b0:6ed:7684:484b with SMTP id u13-20020a056a00098d00b006ed7684484bmr13708804pfg.27.1713285185462; Tue, 16 Apr 2024 09:33:05 -0700 (PDT) Received: from ?IPv6:2605:59c8:43f:400:82ee:73ff:fe41:9a02? ([2605:59c8:43f:400:82ee:73ff:fe41:9a02]) by smtp.googlemail.com with ESMTPSA id o11-20020a62f90b000000b006e6fc52ecd0sm9079650pfh.123.2024.04.16.09.33.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Apr 2024 09:33:04 -0700 (PDT) Message-ID: <68d1c7d3dfcd780fa3bed0bb71e41d7fb0a8c15d.camel@gmail.com> Subject: Re: [PATCH net-next v2 10/15] mm: page_frag: reuse existing bit field of 'va' for pagecnt_bias 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, 16 Apr 2024 09:33:03 -0700 In-Reply-To: <20240415131941.51153-11-linyunsheng@huawei.com> References: <20240415131941.51153-1-linyunsheng@huawei.com> <20240415131941.51153-11-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: rspam04 X-Rspamd-Queue-Id: B4982C0021 X-Rspam-User: X-Stat-Signature: xw41ei5pmigujuxjscgrxpemn4axc8zj X-HE-Tag: 1713285186-410629 X-HE-Meta: U2FsdGVkX18A2Ea84HktJAIB1wu3Fqy39MycEUwN0iEHKK9IHsmx5uAWFqviGxTuQXUUzhYUyrN+Vc2nlpULfMIpKRe78eseaFKji0MOr4d5hTUe61sIEII5dGWp8w/MZVaPmPjguQXxIlwVr349JP42y5hUK7mlcpkwDVI19pkgSTUM+FpDNgPQE7pE1oSRe18++otRQmpGlXN7WCUvOD1LhGnXad9KqQ5aZw//zmYam4gEyMFqJTbk4qXlstG4lbqsRfjMFuPMnheQ1BBZITAfCBCk2su0//V8bvEV+WnSE9bh22Tf8cfvD9UEvTY9Bioew2fuQdzBKhXku4n8KVPTaUpKlWInaEO2z45xKgruowuQE2tTpmSgEkmX0nXOdliAvUg5iu48SBY9kB0DYtgg4ez1O0uhEDY9E1EioNQXLjhUYRDBdLCmpkTyS6ImKMIut/7zoubmjJKS5Q4lZJS8WbVmQGPUv1EaVrZVhQWo47F3G0vRIsy5hZRMrp0a0G9lRHqSWuJe0+Qq0vX/rhz9pw4fesCUVf+A2pX4HsCGM9NoMGsU2n0CXIE7l3JNMl3x7VeoM+c1KkoxSVlclophzqLWezJOfZdokrrhfGB/X/jJowGKFqyvrvelHFWei/NgY21QV92lsSPGNIArld+n1sHgwDFmeozDqiPNM/PYN3zKz0T2t0fHlYJiNP0yzg85ICWBUQnEZVOhvgFeHRbJSG9tL8XvYRSqrbkwDWyYLU90m5jqzJky2cMJIAVRA6Uyi3miBiqTnRXjiFgmhWQBa280RlzCx/1enEk3PVzF97Euvxth2vp1M6oxFJFsyGG7icPm4CNip6AanxXhvUHV/LeQfjK4IR6WKbSsLqMmmAws8K8mD41BTxxzSlFkxsb4lCrU2AKO9D7+rF2Tqu3v8eVfjAFjAk0b+B6PW+Bo3kD1Iu6gkTfg3WUbLC5KuFH/dEmzd3pryeOh2ZF NnFHkvoJ s7hTND6BDCJu5PASv2RP5b6XmmDnzfdm9qsZ1zDGfL8R3Rz9g5fCKcZ62d6GkOuq/uCCtNhTI9LyzguaxTVvy/1D3iuMyo745ScyBJPt2Ik14bfbSytGCPhsSbQxziowW3vDhuo9eNMbR0+GtnjuQqnb2wA== 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 Mon, 2024-04-15 at 21:19 +0800, Yunsheng Lin wrote: > As alignment of 'va' is always aligned with the order of the > page allocated, we can reuse the LSB bits for the pagecount > bias, and remove the orginal space needed by 'pagecnt_bias'. > Also limit the 'fragsz' to be at least the size of > 'usigned int' to match the limited pagecnt_bias. >=20 > Signed-off-by: Yunsheng Lin What is the point of this? You are trading off space for size on a data structure that is only something like 24B in size and only allocated a few times. > --- > include/linux/page_frag_cache.h | 20 +++++++---- > mm/page_frag_cache.c | 63 +++++++++++++++++++-------------- > 2 files changed, 50 insertions(+), 33 deletions(-) >=20 > diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_ca= che.h > index 40a7d6da9ef0..a97a1ac017d6 100644 > --- a/include/linux/page_frag_cache.h > +++ b/include/linux/page_frag_cache.h > @@ -9,7 +9,18 @@ > #define PAGE_FRAG_CACHE_MAX_ORDER get_order(PAGE_FRAG_CACHE_MAX_SIZE) > =20 > struct page_frag_cache { > - void *va; > + union { > + void *va; > + /* we maintain a pagecount bias, so that we dont dirty cache > + * line containing page->_refcount every time we allocate a > + * fragment. As 'va' is always aligned with the order of the > + * page allocated, we can reuse the LSB bits for the pagecount > + * bias, and its bit width happens to be indicated by the > + * 'size_mask' below. > + */ > + unsigned long pagecnt_bias; > + > + }; Both va and pagecnt_bias are frequently accessed items. If pagecnt_bias somehow ends up exceeding the alignment of the page we run the risk of corrupting data or creating an page fault. In my opinion this is not worth the risk especially since with the previous change your new change results in 0 size savings on 64b systems as the structure will be aligned to the size of the pointer. > #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE) > __u16 offset; > __u16 size_mask:15; > @@ -18,10 +29,6 @@ struct page_frag_cache { > __u32 offset:31; > __u32 pfmemalloc:1; > #endif > - /* we maintain a pagecount bias, so that we dont dirty cache line > - * containing page->_refcount every time we allocate a fragment. > - */ > - unsigned int pagecnt_bias; > }; > =20 > static inline void page_frag_cache_init(struct page_frag_cache *nc) > @@ -56,7 +63,8 @@ static inline void *page_frag_alloc_va_align(struct pag= e_frag_cache *nc, > gfp_t gfp_mask, > unsigned int align) > { > - WARN_ON_ONCE(!is_power_of_2(align) || align >=3D PAGE_SIZE); > + WARN_ON_ONCE(!is_power_of_2(align) || align >=3D PAGE_SIZE || > + fragsz < sizeof(unsigned int)); What is the reason for this change? Seems like it is to account for an issue somewhere. > =20 > return __page_frag_alloc_va_align(nc, fragsz, gfp_mask, align); > } > diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c > index 8d93029116e1..5f7f96c88163 100644 > --- a/mm/page_frag_cache.c > +++ b/mm/page_frag_cache.c > @@ -18,8 +18,8 @@ > #include > #include "internal.h" > =20 > -static struct page *__page_frag_cache_refill(struct page_frag_cache *nc, > - gfp_t gfp_mask) > +static bool __page_frag_cache_refill(struct page_frag_cache *nc, > + gfp_t gfp_mask) > { > struct page *page =3D NULL; > gfp_t gfp =3D gfp_mask; > @@ -38,9 +38,26 @@ static struct page *__page_frag_cache_refill(struct pa= ge_frag_cache *nc, > if (unlikely(!page)) > page =3D alloc_pages_node(NUMA_NO_NODE, gfp, 0); > =20 > - nc->va =3D page ? page_address(page) : NULL; > + if (unlikely(!page)) { > + nc->va =3D NULL; > + return false; > + } > + > + nc->va =3D page_address(page); > =20 > - return page; > +#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE) > + VM_BUG_ON(nc->pagecnt_bias & nc->size_mask); > + page_ref_add(page, nc->size_mask - 1); > + nc->pagecnt_bias |=3D nc->size_mask; > +#else > + VM_BUG_ON(nc->pagecnt_bias & (PAGE_SIZE - 1)); > + page_ref_add(page, PAGE_SIZE - 2); > + nc->pagecnt_bias |=3D (PAGE_SIZE - 1); > +#endif > + > + nc->pfmemalloc =3D page_is_pfmemalloc(page); > + nc->offset =3D 0; > + return true; > } > =20 > void page_frag_cache_drain(struct page_frag_cache *nc) > @@ -65,38 +82,31 @@ EXPORT_SYMBOL(__page_frag_cache_drain); > void *page_frag_alloc_va(struct page_frag_cache *nc, unsigned int fragsz= , > gfp_t gfp_mask) > { > - unsigned int size, offset; > + unsigned long size_mask; > + unsigned int offset; > struct page *page; > + void *va; > =20 > if (unlikely(!nc->va)) { > refill: > - page =3D __page_frag_cache_refill(nc, gfp_mask); > - if (!page) > + if (!__page_frag_cache_refill(nc, gfp_mask)) > return NULL; > - > - /* 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); > - > - /* reset page count bias and offset to start of new frag */ > - nc->pfmemalloc =3D page_is_pfmemalloc(page); > - nc->pagecnt_bias =3D PAGE_FRAG_CACHE_MAX_SIZE + 1; > - nc->offset =3D 0; > } > =20 > #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE) > /* if size can vary use size else just use PAGE_SIZE */ > - size =3D nc->size_mask + 1; > + size_mask =3D nc->size_mask; > #else > - size =3D PAGE_SIZE; > + size_mask =3D PAGE_SIZE - 1; > #endif > =20 > + va =3D (void *)((unsigned long)nc->va & ~size_mask); > offset =3D nc->offset; > - if (unlikely(offset + fragsz > size)) { > - page =3D virt_to_page(nc->va); > =20 > - if (!page_ref_sub_and_test(page, nc->pagecnt_bias)) > + if (unlikely(offset + fragsz > (size_mask + 1))) { > + page =3D virt_to_page(va); > + > + if (!page_ref_sub_and_test(page, nc->pagecnt_bias & size_mask)) > goto refill; > =20 > if (unlikely(nc->pfmemalloc)) { > @@ -105,12 +115,11 @@ void *page_frag_alloc_va(struct page_frag_cache *nc= , unsigned int fragsz, > } > =20 > /* OK, page count is 0, we can safely set it */ > - set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1); > + set_page_count(page, size_mask); > + nc->pagecnt_bias |=3D size_mask; > =20 > - /* reset page count bias and offset to start of new frag */ > - nc->pagecnt_bias =3D PAGE_FRAG_CACHE_MAX_SIZE + 1; > offset =3D 0; > - if (unlikely(fragsz > size)) { > + if (unlikely(fragsz > (size_mask + 1))) { > /* > * The caller is trying to allocate a fragment > * with fragsz > PAGE_SIZE but the cache isn't big > @@ -127,7 +136,7 @@ void *page_frag_alloc_va(struct page_frag_cache *nc, = unsigned int fragsz, > nc->pagecnt_bias--; > nc->offset =3D offset + fragsz; > =20 > - return nc->va + offset; > + return va + offset; > } > EXPORT_SYMBOL(page_frag_alloc_va); > =20 The rest of this seems like unnecessary obfuscation and change. Basically it is adding more overhead to page allocation for no reward.