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 DB83BC001CC for ; Wed, 17 Apr 2024 13:18:29 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 653356B0088; Wed, 17 Apr 2024 09:18:29 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 602696B0089; Wed, 17 Apr 2024 09:18:29 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4F3C66B008A; Wed, 17 Apr 2024 09:18:29 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 312EA6B0088 for ; Wed, 17 Apr 2024 09:18:29 -0400 (EDT) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id E3F3A1A0E25 for ; Wed, 17 Apr 2024 13:18:28 +0000 (UTC) X-FDA: 82019077896.15.ADA987B Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by imf18.hostedemail.com (Postfix) with ESMTP id 448F31C001C for ; Wed, 17 Apr 2024 13:18:24 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=none; spf=pass (imf18.hostedemail.com: domain of linyunsheng@huawei.com designates 45.249.212.255 as permitted sender) smtp.mailfrom=linyunsheng@huawei.com; dmarc=pass (policy=quarantine) header.from=huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1713359906; 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; bh=Jz4xvmv4YCr6OpuvQhE8upgaIMR26JOUQdmeZiNRIXQ=; b=QeKab6obwir2iWFu6tkJQQG2w4UdAJLGrgaiiRY9cMar43dB9+tPX+98tDehIVXeqOm261 579yNTqPtYdSfE8LBTWERdEJjbavlt/CuPESJKKFH4RVAOXjBFXDlxVOVQIMm3fSxR9GvN VZHy2Zu535y5CbGkBgHYnFSyHh0cjU4= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713359906; a=rsa-sha256; cv=none; b=nLCggkzRi8eSgZaovY97FxC88LDck7P2m3JjGbjl+X6AVHVwx8KMo+SKDOx8oQiiuPMyCE gOAdW2S3JwjEP18j0fHJU35l5TFLSR99t7LHLr/Zv02dcuwKn5Expewly1Sw1VUE8sX5Ua 5FfVKlftpaLxlDgtXSlPVX+heW+TKnU= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=none; spf=pass (imf18.hostedemail.com: domain of linyunsheng@huawei.com designates 45.249.212.255 as permitted sender) smtp.mailfrom=linyunsheng@huawei.com; dmarc=pass (policy=quarantine) header.from=huawei.com Received: from mail.maildlp.com (unknown [172.19.163.48]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4VKLxX6y74z1R5q8; Wed, 17 Apr 2024 21:15:28 +0800 (CST) Received: from dggpemm500005.china.huawei.com (unknown [7.185.36.74]) by mail.maildlp.com (Postfix) with ESMTPS id 7C5E718005D; Wed, 17 Apr 2024 21:18:21 +0800 (CST) Received: from [10.69.30.204] (10.69.30.204) by dggpemm500005.china.huawei.com (7.185.36.74) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.35; Wed, 17 Apr 2024 21:18:21 +0800 Subject: Re: [PATCH net-next v2 06/15] mm: page_frag: change page_frag_alloc_* API to accept align param To: Alexander Duyck CC: , , , , , Andrew Morton , Eric Dumazet , David Howells , Marc Dionne , , References: <20240415131941.51153-1-linyunsheng@huawei.com> <20240415131941.51153-7-linyunsheng@huawei.com> From: Yunsheng Lin Message-ID: <68d28743-1f07-4985-8fc5-9f5558879ac2@huawei.com> Date: Wed, 17 Apr 2024 21:18:20 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [10.69.30.204] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To dggpemm500005.china.huawei.com (7.185.36.74) X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: 448F31C001C X-Stat-Signature: p1gcb498tcpkoieiz37prwdp9duxbxn9 X-Rspam-User: X-HE-Tag: 1713359904-502847 X-HE-Meta: U2FsdGVkX1/p9893OSiWCXr3Iq81GHwXTkRBW86GGQ2etfz7fkPFIPppfcGcje08WtrejWjMyu2y/wMY1A/1NmzKFYQ7YLghgjA/xQMy/JwLjuFAauICKYmgj30g5JjQfbiRTMxX0vr5qhyM4PJL3c5to3FhCIo12r6oP4TkucCI9rwlZgmkctnQhQIk1iZxxAtxiVCGu66a7kfBuGj6VVu/BWjdpTMbmsRc1RW6sdrl0emDE92Al9rUCiy9/LQz/xnI5fj0GHrfckbmN4RHYLl5pNLwhxwcVzmHryGiwBcqTuGUwvnSz5yOk78rQKLCRawRu7jDTlecEYZDYcpTdvbqM5CFFatWowEmDKAFMiVfl67LGCnQy/oRZ6NsFh5CPV6LJpZAARHI5gJbDvdWavb/rMzIw56ZgCsvvepJL95KKHFToF7t3BAi8IVG+y/Z34D46m3yxdFC9OHyDYGZboidxiKMTMFx6nQZLICD2Bp7wpzvyz7WtDUuMjUprqDRrnrajACty2Gy6XDA2vi/t44zaNApr2G23tNbcSMLBzKn2etfDz/Jjqe3jJ0QBuLjMsTAA4OOwSKa9Gu0VLKFMSXW/xdE1keLTqsdFWzqDIXQfxJsJIJKE7ENxXD2rGo9e4gWSyFaUYJcPjCSJjaI4jO8U6nXFX3NPFVx0WOVfpyLUZ5Y+XpFA0QGVuLJMba7ghiqXb0f6F8qZ05Ry1BLLEhJ6D2FsWoDcphbyUHNRB93We0eIQfEtVYpwtrJPnhK7Y1i1rRaiGq2b1zAgp7ttD9dLecI8AuhrQVeA1iCp6hEvbmcpzBaSaIi5Nlkflw99DPnuQR7cmTG+4X3Y7JSudIy09BBHzX7ZETEUON3F8Lpps6kkH70OJHcwuBNqzOBFFxrnMKdLS1jrfI1g+z+ZHwC+VK2QZKdfiJUpbK3jSwpuSsPdoRdzWUY4HWoXfmMbLNfiOMYdjby1sxx1hN lM+3S8/m yBN0jQPG3BR7AYcXXV/EcSp/Nn3SJWF97GSxWCOHWCakBqSHK55iZZ+tl3Bb7V9MqOQv3TM0SFQ6WpdRR/At44LGzW58o+CEoVUQYeQxMt1FLptj0lxk1mGf2crAVyvaOEOMWhMvLadAOzq8PMoc4q0YqrNDas1ztzAanTUiYBGdb5ksAnHJglVD+AZA4FxVttzfWm6fkOWoy8aweW4MxB4rOsI1fEo3FotmdZfQgQoM9MUDwpNw9HOx/UnBzpL0S/NQr7mlG1bbkO+lYZkfne1nsIQ== 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 2024/4/17 0:08, Alexander Duyck wrote: > On Mon, Apr 15, 2024 at 6:22 AM Yunsheng Lin wrote: >> >> When page_frag_alloc_* API doesn't need data alignment, the >> ALIGN() operation is unnecessary, so change page_frag_alloc_* >> API to accept align param instead of align_mask param, and do >> the ALIGN()'ing in the inline helper when needed. >> >> Signed-off-by: Yunsheng Lin > > The vast majority of callers are using this aligned one way or > another. If anything with your recent changes we should probably be > making sure to align the fragsz as well as the offset since most > callers were only using the alignment of the fragsz in order to get > their alignment. > > My main concern is that this change implies that most are using an > unaligned setup when it is in fact quite the opposite. I think the above is depending on what we are about is 'offset unaligned' or 'fragsz unaligned'. 'offset unaligned' seems like the most case here. > >> --- >> include/linux/page_frag_cache.h | 20 ++++++++++++-------- >> include/linux/skbuff.h | 12 ++++++------ >> mm/page_frag_cache.c | 9 ++++----- >> net/core/skbuff.c | 12 +++++------- >> net/rxrpc/txbuf.c | 5 +++-- >> 5 files changed, 30 insertions(+), 28 deletions(-) >> >> diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h >> index 04810d8d6a7d..cc0ede0912f3 100644 >> --- a/include/linux/page_frag_cache.h >> +++ b/include/linux/page_frag_cache.h >> @@ -25,21 +25,25 @@ struct page_frag_cache { >> >> 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 fragsz, >> - gfp_t gfp_mask, unsigned int align_mask); >> +void *page_frag_alloc(struct page_frag_cache *nc, unsigned int fragsz, >> + gfp_t gfp_mask); >> + >> +static inline void *__page_frag_alloc_align(struct page_frag_cache *nc, >> + unsigned int fragsz, gfp_t gfp_mask, >> + unsigned int align) >> +{ >> + nc->offset = ALIGN(nc->offset, align); >> + >> + return page_frag_alloc(nc, fragsz, gfp_mask); >> +} >> > > I would rather not have us breaking up the alignment into another > function. It makes this much more difficult to work with. In addition > you are adding offsets without actually adding to the pages which > makes this seem exploitable. Basically just pass an alignment value of > 32K and you are forcing a page eviction regardless. Yes, as you mentioned in patch 9: The "align >= PAGE_SIZE" fix should probably go with your change that > reversed the direction. > >> static inline void *page_frag_alloc_align(struct page_frag_cache *nc, >> unsigned int fragsz, gfp_t gfp_mask, >> unsigned int align) >> { >> WARN_ON_ONCE(!is_power_of_2(align)); >> - return __page_frag_alloc_align(nc, fragsz, gfp_mask, -align); >> -} >> >> -static inline void *page_frag_alloc(struct page_frag_cache *nc, >> - unsigned int fragsz, gfp_t gfp_mask) >> -{ >> - return page_frag_alloc_align(nc, fragsz, gfp_mask, ~0u); >> + return __page_frag_alloc_align(nc, fragsz, gfp_mask, align); >> } >> ... >> /* >> * Frees a page fragment allocated out of either a compound or order 0 page. >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >> index ea052fa710d8..676e2d857f02 100644 >> --- a/net/core/skbuff.c >> +++ b/net/core/skbuff.c >> @@ -306,18 +306,17 @@ void napi_get_frags_check(struct napi_struct *napi) >> local_bh_enable(); >> } >> >> -void *__napi_alloc_frag_align(unsigned int fragsz, unsigned int align_mask) >> +void *__napi_alloc_frag_align(unsigned int fragsz, unsigned int align) >> { >> struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache); >> >> fragsz = SKB_DATA_ALIGN(fragsz); >> > > So this is a perfect example. This caller is aligning the size by > SMP_CACHE_BYTES. This is the most typical case. Either this or > L1_CACHE_BYTES. As such all requests should be aligned to at least > that. I would prefer it if we didn't strip the alignment code out of > our main allocating function. If anything, maybe we should make it > more specific that the expectation is that fragsz is a multiple of the > alignment. Let's discuss the above in patch 5.