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 6321BC54731 for ; Tue, 27 Aug 2024 16:00:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D1E216B0082; Tue, 27 Aug 2024 12:00:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id CCE396B0083; Tue, 27 Aug 2024 12:00:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B95D66B0089; Tue, 27 Aug 2024 12:00:50 -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 9D4616B0082 for ; Tue, 27 Aug 2024 12:00:50 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 181BA1A1766 for ; Tue, 27 Aug 2024 16:00:50 +0000 (UTC) X-FDA: 82498488660.29.D553DC4 Received: from mail-io1-f45.google.com (mail-io1-f45.google.com [209.85.166.45]) by imf10.hostedemail.com (Postfix) with ESMTP id 8F1D4C0021 for ; Tue, 27 Aug 2024 16:00:47 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=KEt86LpF; spf=pass (imf10.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.166.45 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=1724774403; a=rsa-sha256; cv=none; b=Hklw4zrGJDxrM3NK8kIjSuw+EHxekqGhMKs0/VJPRhB6kBv75zLIzJ7QNMw3hqPUOEl4TD EHWzu81vJKGHWFUXKStwRBj7u1cL8kd409ajS3lqGHTUocQHERMs6KnSba5XYSPfrTMAO1 BvRCMSYB011QJLVULzBDndYlYrpXgkc= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=KEt86LpF; spf=pass (imf10.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.166.45 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=1724774403; 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=fKLMWrg6g9OU+cV8ThfWWZDxMIrdaGFl7c8JSKNiFKQ=; b=MdIcJk2uvH4q4n6T8a2a7iwYyvSZ9QRA5R/T5st1EVxVuWzYnHXrEE7NzHQK90ZFHosCz2 DaeydWW6h50s9Q0+a6nJZyBhXfOW2BZlHO/G7iHyfSWSTvGwr54h9R7r6LRiBcU73curOg 1KxTCaJCVSu/YBASDfr9eVr5S1/NtY0= Received: by mail-io1-f45.google.com with SMTP id ca18e2360f4ac-81fd925287eso206001339f.3 for ; Tue, 27 Aug 2024 09:00:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1724774446; x=1725379246; 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=fKLMWrg6g9OU+cV8ThfWWZDxMIrdaGFl7c8JSKNiFKQ=; b=KEt86LpFUrScvarou943HM9n8KqbItEhk737t81s+N1BMS3Ud50unb3m30cIkbIJSY 2wTKaO2Imq2yYq6z0AkZafICoxkcxVgThYZbqvMaHrH9eg+3YUzOp1KrB1OxC8NONihH C2UdggFjU/vKGxsctX8RoLqOlrZToQNxxpYyGjYCXZ+I+phLuEeBTxzKBfo4H9avXk7j BmRqEtrvWOXHqS9g4dq15gc5ima6k8ZrZanqZRo8l1YkmlbyIZWhYLIhCBhubl2B+WKU JtYyYRv3tZOeZl3bSfpCtgWW/ARq4GUsy47SBuelZ/BVgC3ktEzKS2r6sqm5uc/URX+a wL1w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724774446; x=1725379246; 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=fKLMWrg6g9OU+cV8ThfWWZDxMIrdaGFl7c8JSKNiFKQ=; b=qlHFo0nhhh4fJxRp7lJEoaO/cn2AbyNO0K4RvXFe9DIIdTaZB1it4/3HFRbdTT6are DbmhpFDedca1rvjhlFsUbfdLn/MpCj9zw+WuaEycXJdkLnyWQgg29lfqOampF50qzK+U S7bU/KlvWlICyCJx/cQYbpMWdJtyuj7PctHqOa1l4d/32sSJWeIAGjMfOCmeBhkYQHbx iQpOfnI01PHodcY1pQJiVCSUh0140oOghhLO/DZxWSu8KqnTy3s3ow49AgANMGIxcfVm GJaHLDqD0egX+e8MKJS6SxBWWiJmUxKgpd7a92PARdlRynGmuribTNFEkdqyXrVhrRfZ 3mzw== X-Forwarded-Encrypted: i=1; AJvYcCWPzMarrNxeQcT6/PmQTkq2borvnD6JiyAYxg0BJTuaDpMVRSnGuC6+hWnWDJjq3mpGrtDPELZDaQ==@kvack.org X-Gm-Message-State: AOJu0YxB6Z1lPKpnW5MPqK+/apwXcX3fAzTTchoxWZzvd/5Nqf5TQMtA zAynTFQN+kBg44OzixiuQals8SWlimbvk8pzxwQDGMRCsh3mQtSPXiDCuDMdX9Gi/JR98RsPcHz 2zvrNV/CX4yLM0tPMgQCdTyHCbF4= X-Google-Smtp-Source: AGHT+IEXbBzBotSspCNr329ppunAPqBGFxziRxfGF+s9zT3UuNt8tn9xzLCMG8+eOSmv8TOYSl7rZYAUpWO6AbxuoKU= X-Received: by 2002:a05:6602:6421:b0:824:d5ff:6a55 with SMTP id ca18e2360f4ac-829f132a1a3mr381013839f.16.1724774446130; Tue, 27 Aug 2024 09:00:46 -0700 (PDT) MIME-Version: 1.0 References: <20240826124021.2635705-1-linyunsheng@huawei.com> <20240826124021.2635705-8-linyunsheng@huawei.com> In-Reply-To: <20240826124021.2635705-8-linyunsheng@huawei.com> From: Alexander Duyck Date: Tue, 27 Aug 2024 09:00:08 -0700 Message-ID: Subject: Re: [PATCH net-next v15 07/13] 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-Stat-Signature: 95jm7mhuec3ko3e41wb644sd48u7rxo7 X-Rspamd-Queue-Id: 8F1D4C0021 X-Rspam-User: X-Rspamd-Server: rspam10 X-HE-Tag: 1724774447-342981 X-HE-Meta: U2FsdGVkX18qUWT82QSgyPg+k/9kvBqEHbjFMBGuVGWFtcI44jzNxk65+GaznNcxiFb/VEr5IBK/OotBnESseHzG5XjmarQeGxC7bVCBOUxa3gFL+Whc6oSNpFWz4WVAPQCYZiJl+EpQPIAWrfKBDn8PIEwXnjVrxXvbAl6JGA/ZiJmE18oFX7EYjT3jSwPBqKirit5YYFbwxKV+udaW4lb9OKaBsjvvLU1iGEfUdwSX+Gi0Nw5a0JYFdxUsLWk1Mla7Y9InvMMe0nIFXzhtD5Ae/lw4z74v+267zbxpX3cVCe4WrnsQoUD7mm6ZMwievtrqHwVXKItY3V7ePBQuPiQ8Tnq8mIDdWPWJjElfLGZktQhseEK01Wr+PWnbyOYDJ3mWmowR7I9r9TVvEYYTlrQuj9D0AwBMCOcXf/B8S3PgnjHT/VIbRN5aFNkEw8EHSHpMYTGY2SJd0BsKIbj3DTNc3TDnjZQ5k9VecU41WeWW7ForNToMfltTWP9OxbA3/rdAryltN5ElFDKKxeUfUWls9JvjJajXrdPinZRbywJSCaUSk1pUKxDjSUuovZHg9sIohG4xS7ETS8m6e2edbQvNNZL1G95z9cryNbodFrHAgQPh52eg8qNnYG5ou0NnKo7oelGBd0s1NtA0l5ImBbLWoDnXlPOgk84bzQEMHuDf7pDSt3G1LFfQ9WbQX89Q8L0pjLUfd1bZy2aWfLILArX14Nbqr4ZJTo9EPBtrdX/ku+m0JbXCjhhjWdom6G1NetNSGdbJpT4rVYaRkN9T87yGKwlXHKlyA+7LKN5auUqCiAlsgbYtIzS5pn3lsYOf1cl+1srvrJms3brrkPzVE10wHjrN+K02UhyqCadMhwX5gSm+J7fkXr9UXBJHtLVY/At3eng7lNSyTview8Co/soP7ubqpu3UBV3Q70XHzm/nUEy2TgsxsD9c3Seokt35DxfCJ4K897h9vYAdGTG xT+f/6zw ISecG/rahmP8oYIfJ8ZcZhSKTgG9kFn0koL5FBmnKshkja+OY0kZqcLbF8zoTmxObg5RJrwOtntey4ihgwlwv5vvqxWcHOvXbUVj+js1v4kDOv9lKvqajZsER62LXCNyGUiF5IEKFz2uGI6gaT7jjJy6Qz43BBe1lMwSbT8RcKnsOi9Dt0ifY8sJjDLZd3pqYin1/77OpHYhloLXYOzUcPbxYKiwN/N5TmruBb2MYl7HY+8V95NS3jo1hgeAaJ3KYA0ti03pNhtWRXYB73P9+bOGEccR05UVwGKlG0tDjan3Tq+rkWj9vzG/y2PbyoSTorkd3q46UvucO3TcQZRelauFzfgqHKlBKdAyJJwni8mmtaPdIdm1U3HLUBq6f9Q6yHXtd+dlEQigqpKpFMsNJfqMXwgz36ZNIu7biHrda49WJtohQ0RcXvMJfLzgX2gvulTUT 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, Aug 26, 2024 at 5:46=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 | 51 +++++++++++++++++++++++++++++++-- > mm/page_frag_cache.c | 20 ++++++------- > 2 files changed, 59 insertions(+), 12 deletions(-) > > diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_ca= che.h > index 372d6ed7e20a..2cc18a525936 100644 > --- a/include/linux/page_frag_cache.h > +++ b/include/linux/page_frag_cache.h > @@ -7,6 +7,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -75,8 +76,54 @@ static inline unsigned int page_frag_cache_page_size(u= nsigned long encoded_page) > > 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); > + > +static inline void __page_frag_cache_commit(struct page_frag_cache *nc, > + struct page_frag *pfrag, bool= referenced, > + unsigned int used_sz) > +{ > + if (referenced) { > + VM_BUG_ON(!nc->pagecnt_bias); > + nc->pagecnt_bias--; > + } > + > + VM_BUG_ON(used_sz > pfrag->size); > + VM_BUG_ON(pfrag->page !=3D page_frag_encoded_page_ptr(nc->encoded= _page)); > + > + /* nc->offset is not reset when reusing an old page, so do not ch= eck for the > + * first fragment. > + * Committed offset might be bigger than the current offset due t= o alignment > + */ nc->offset should be reset when you are allocating a new page. I would suggest making that change as you should be able to verify that the fragment you are working with contains the frag you are working with. The page and offset should essentially be equal. > + VM_BUG_ON(pfrag->offset && nc->offset > pfrag->offset); > + VM_BUG_ON(pfrag->offset && > + pfrag->offset + pfrag->size > page_frag_cache_page_size= (nc->encoded_page)); > + > + pfrag->size =3D used_sz; > + > + /* Calculate true size for the fragment due to alignment, nc->off= set is not > + * reset for the first fragment when reusing an old page. > + */ > + pfrag->size +=3D pfrag->offset ? (pfrag->offset - nc->offset) : 0= ; The pfrag->size should be the truesize already. You should have stored it as fragsz so that all you really need to do is push the offset forward by pfrag->size. > + > + nc->offset =3D pfrag->offset + used_sz; > +} > + I think this function might be better to keep in the .c file versus having it in the header file. ... > diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c > index 228cff9a4cdb..bba59c87d478 100644 > --- a/mm/page_frag_cache.c > +++ b/mm/page_frag_cache.c > @@ -67,16 +67,14 @@ void __page_frag_cache_drain(struct page *page, unsig= ned 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) > +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; The 3 changes below can all be dropped. They are unnecessary optimizations of the unlikely path. > struct page *page; > > - size =3D page_frag_cache_page_size(encoded_page); > - > if (unlikely(!encoded_page)) { > refill: > page =3D __page_frag_cache_refill(nc, gfp_mask); > @@ -94,6 +92,9 @@ void *__page_frag_alloc_align(struct page_frag_cache *n= c, > /* 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; Your code above said that offset wasn't reset. But it looks like it is reset here isn't it? > + } else { > + size =3D page_frag_cache_page_size(encoded_page); > + page =3D page_frag_encoded_page_ptr(encoded_page); > } > > offset =3D __ALIGN_KERNEL_MASK(nc->offset, ~align_mask); > @@ -111,8 +112,6 @@ void *__page_frag_alloc_align(struct page_frag_cache = *nc, > return NULL; > } > > - page =3D page_frag_encoded_page_ptr(encoded_page); > - > if (!page_ref_sub_and_test(page, nc->pagecnt_bias)) > goto refill; > These 3 changes to move the size and page are unnecessary optimization. I would recommend just dropping them and leave the code as is as you are just optimizing for unlikely paths. > @@ -130,12 +129,13 @@ void *__page_frag_alloc_align(struct page_frag_cach= e *nc, > offset =3D 0; > } > > - nc->pagecnt_bias--; > - nc->offset =3D offset + fragsz; > + pfrag->page =3D page; > + pfrag->offset =3D offset; > + pfrag->size =3D size - offset; Why are you subtracting the offset from the size? Shouldn't this just be fr= agsz? > > return page_frag_encoded_page_address(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 >