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 47AEBC3DA4A for ; Fri, 2 Aug 2024 17:01:23 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B48F16B0096; Fri, 2 Aug 2024 13:01:22 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AD2306B0098; Fri, 2 Aug 2024 13:01:22 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 924F26B0099; Fri, 2 Aug 2024 13:01:22 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 6FD406B0096 for ; Fri, 2 Aug 2024 13:01:22 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id E5259120AA2 for ; Fri, 2 Aug 2024 17:01:21 +0000 (UTC) X-FDA: 82407921162.23.51216DC Received: from mail-wr1-f43.google.com (mail-wr1-f43.google.com [209.85.221.43]) by imf12.hostedemail.com (Postfix) with ESMTP id 8EC064002E for ; Fri, 2 Aug 2024 17:01:18 +0000 (UTC) Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Js7SVjOS; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf12.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.221.43 as permitted sender) smtp.mailfrom=alexander.duyck@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1722618035; a=rsa-sha256; cv=none; b=isWnyDjK9Plo+7BM1GJQGAS09iZ705HK/1Tj/rBlh8MK4e98zopxZFFU+sTnXUfqJWmkD2 RYMxbg9TEgRPsoy8l9l23nO9PpNaTj2I3UAR8suhFiInB1vF8230JXclncJSRVW8Aim6BI lIwQM6RGxiuVzrgpls/kfE1AA0orx2A= ARC-Authentication-Results: i=1; imf12.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Js7SVjOS; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf12.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.221.43 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=1722618035; 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=vpcsgqmtuPo+pz6tWtSNKqkTo9JxSymPCppu91onA2Q=; b=I9P88aSIt0wXL8DUFzCXJ8ZGFeNrYL6j5eG7uvzEeZDdMDxwuVEnNn59vZAzmXtrfkfj92 JSY21t52qztSPgBHEdFJ+7lVWwwFQ3imno8J4Esh61KVKtf6uMzQCNOrWdGwEwsVQFlCWt wGq/PDNldVbJ8YySBdosv4ZK1tEbI5w= Received: by mail-wr1-f43.google.com with SMTP id ffacd0b85a97d-3683178b226so4058834f8f.1 for ; Fri, 02 Aug 2024 10:01:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1722618077; x=1723222877; 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=vpcsgqmtuPo+pz6tWtSNKqkTo9JxSymPCppu91onA2Q=; b=Js7SVjOSeh3IBxrl4yqGnWPyvc1WqrwX92DtxSlJrdEPKuSYqhiU7Qv274ueU7wJTH RRhbIAdzBQwjyPdOy523bHNxCAB/EGDaHT7//Zm6WDA0qyPjlxdq2USQ0MFo413mEAT5 /tSLp1GsRBYVCFXynulLFiRwez+bBTfq8xJmi0z/8pv+0ByI0Ljutc1LpPxTTbbYVcXk B4v4d3Npijg2uP7tbuStIrkyq8nzB6N+0tWQDK9ZH7CRielt195Ox7H2V49CLce6u7k6 0LfJ7Pp3iDto82a3qrHnH2a7OT1LIQct5pLgaXjIIld6EZHZ6PhfBTGAoj0OVDyRRvQV OsCA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722618077; x=1723222877; 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=vpcsgqmtuPo+pz6tWtSNKqkTo9JxSymPCppu91onA2Q=; b=lCbVefFYDh8Mrvqa2jFtV7nqBO9SKFgyTVNaYBZtCvZ1Xcf57hLcTn+kKvyaQU1BP7 d4zGdlBPlaccNZxy1MUA+dVC6HqTzfPpgrwVyfrU7TWJi+ynyUad6tc210RNkQx0YFJw fQVcHRs62ey/Z9pHxu3cWNXwmoA2//uTPUOq5ebzfCj2zuP5BbMWM6ceOzbg0zSz+1eI g1k+CqKbM4D6tWSj/59CpZQAjyJbK8lU4JoCU1HjqhQs5joC5ekyhlsAavDkJpLiZ+/9 Xd7P5bXZQBW2OsFKSG8YKc0UXNtBEY0q1j+6mSLp1W1eXQqMLKxOQBT0Xg95xa7apoYV 8+Qw== X-Forwarded-Encrypted: i=1; AJvYcCUNdreWl6a2LtOlAvtaWHV81t1eF/7j5/nDWsb1K54jpVb9j0V21zuNNo8+171zCxpEfWxzB4mYL3ZUjAnBY5HjoeE= X-Gm-Message-State: AOJu0Yx2h1BMM0RgBlbXfKOJlmMKrDYVWnVscrApgPeMUMxnn9ko9iMT Y3kJjKTFD4NtZ455LDbJrPSUd6F0As9HTmd+s8Qt+FY+tTWAQ/KhTC7HpM2BJxaiXtzaqQMFGMU FZQ4YS/JI6plcfMNAUI4ruY0BlrU= X-Google-Smtp-Source: AGHT+IH75FU4JpBvhRKVqmFn8me2clEuJ7/a5V8Xhv6OElYPF/ofyi1s2L5ZzsGAeeZuvfLRALy59I0v9WsGM6Mhk90= X-Received: by 2002:adf:e98a:0:b0:368:4d33:9aac with SMTP id ffacd0b85a97d-36bbc0fc757mr2416504f8f.31.1722618076415; Fri, 02 Aug 2024 10:01:16 -0700 (PDT) MIME-Version: 1.0 References: <20240731124505.2903877-1-linyunsheng@huawei.com> <20240731124505.2903877-5-linyunsheng@huawei.com> <22fda86c-d688-42e7-99e8-e2f8fcf1a5ba@huawei.com> <877efebe-f316-4192-aada-dd2657b74125@huawei.com> In-Reply-To: <877efebe-f316-4192-aada-dd2657b74125@huawei.com> From: Alexander Duyck Date: Fri, 2 Aug 2024 10:00:39 -0700 Message-ID: Subject: Re: [PATCH net-next v12 04/14] mm: page_frag: add '_va' suffix to page_frag API To: Yunsheng Lin Cc: davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Subbaraya Sundeep , Jeroen de Borst , Praveen Kaligineedi , Shailend Chand , Eric Dumazet , Tony Nguyen , Przemek Kitszel , Sunil Goutham , Geetha sowjanya , hariprasad , Felix Fietkau , Sean Wang , Mark Lee , Lorenzo Bianconi , Matthias Brugger , AngeloGioacchino Del Regno , Keith Busch , Jens Axboe , Christoph Hellwig , Sagi Grimberg , Chaitanya Kulkarni , "Michael S. Tsirkin" , Jason Wang , =?UTF-8?Q?Eugenio_P=C3=A9rez?= , Andrew Morton , Alexei Starovoitov , Daniel Borkmann , Jesper Dangaard Brouer , John Fastabend , Andrii Nakryiko , Martin KaFai Lau , Eduard Zingerman , Song Liu , Yonghong Song , KP Singh , Stanislav Fomichev , Hao Luo , Jiri Olsa , David Howells , Marc Dionne , Chuck Lever , Jeff Layton , Neil Brown , Olga Kornievskaia , Dai Ngo , Tom Talpey , Trond Myklebust , Anna Schumaker , intel-wired-lan@lists.osuosl.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-nvme@lists.infradead.org, kvm@vger.kernel.org, virtualization@lists.linux.dev, linux-mm@kvack.org, bpf@vger.kernel.org, linux-afs@lists.infradead.org, linux-nfs@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 8EC064002E X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: b1t9o784dhjptzd9za5cukcisnz54kco X-HE-Tag: 1722618078-493604 X-HE-Meta: U2FsdGVkX1+c6RBB+0pRryScPXvrBchtSk75qKJalC7wUyEje62+cw1Wla2Poj6OUHzZr6YFx/ZuhGqCEnyM4Owwkaad40bhS/s/wZoX157xMmYHjs42wY4kagr3y4FOit/XZQmJRs+uF9fAPZHMXW3xu9Ex0XvnYGyhFFrSosiY9WPxceQ/7FS6z/zeOZG7Gsb0x0k/X6UbM6I8x2TJb1CfKyALEEyAXnKNg7b+1bT8NIOcZe8VsGDBfwPWK8TYBgZpSrvjcWkuA/eumdpAtj2n+KWJTAUO4qmCMUIetGSrwsTUytsv7sO/A64+cNLLOJNBB2m6KpmdRLOXyuyblVStLVP/uplx8dc7VTDnGqkWYThwi2i0i5ex1CN2RffPd5p6rCupPlvqS9nxjrqWFsuclPfrnpTt7dWhX2270YWjKCi3lDZkZWL2STjIiqiYafbM/ic83/6iG/r5a7kNIVHGE4eQ57KjeSPJyaHtQwzb3ew36XlywUApfH+MZAFnn9wPTTI8ME12T6z8irSDns2SeBNb+Xlz4Uliy7Qv8+Ob8LWVZRQvoyxmZs7QKyNrAR+646/AwW8QliEB0MNu0WHGta7nt0z1owpHHUvfzWRqWty9CxSZQIsTYolgLd84/NpcmPuo6BVotJtFM4luK9J2gx5Chg7E23rak+FWgldAUyHHUz5WwUwgujnBEtPWFFb6D/SVMU6AcfHSlUkdw+OYfMjaOfWg2Hsl6kKUSE6MxuXUQ14GRwB6gdRtuPwjZfdt9VZYZ4noKjxlIl7hGQgqrLhY8OeeBE+L5me+8bhabiomA78J1IAgAM3quX/Z2pD8ZCsNeLdR36a2z3GWh2/FTgxjRUlIBLbABezDX/PWKZavs5S6JHO5Cokxv1zSryCjw5SUcAdfavB/ChOg1UCE8VUkwWyVylekYRPHuTr+ZCQbmHIa2Ca50F69B+kjB+I0+p5ziQV3nze7vez kW0ps8iI E5QOBahYaFjxzi8KV9Os2DQ3ndeDgvOTUPfW4A2dXtsVzoDlX/lOqDcfHI12oROpc4y/PyHLe/x7qeh6EMH3u8/cD8dN0U3xFXWt76Frz2WZmCINEU/ybx6/EEVI6jRClJUjqcka2gcsW0lyknHH7rHOplGO7rrzcL1CLYosb4SuYZjnbwtNzcbv+Rgo5MY6y2ozV+MJRs+fgNOtptxLRg/cfOd+lnFVfRZ6PwVw0JWU3RxCA54V5hBJ5Jp2xJxYCggx4+nJn/ZV6qMXi3lptKSxuCEdg+5KrrYfWgTEUpvlfMGWVj944qstx2uZVy0mLATR6qN/QLjznP3MHFHCpV8hH75S65Ds3b+dn28B02pchiG5JipL4ec/OszBpu9ofQ3Rom+iHLRVauK4Mt4xgSIHza3hf0azBksBzmF/krZCqDJ8GzHPQXz5G316lxUD0w5OiLl4yvDsqiQ7Yzj9hQBWSjfHfgeUHa7J+oayctp03PSqZ2RQqyXh06nXpodVwx2HqmWMgOG2tBwk1k2QgId9UD0zEqRLLxKqmUVQB91aGSL5i4WUJSkX4sgnz9Geiq14J 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, Aug 2, 2024 at 3:05=E2=80=AFAM Yunsheng Lin wrote: > > On 2024/8/1 23:21, Alexander Duyck wrote: > > On Thu, Aug 1, 2024 at 6:01=E2=80=AFAM Yunsheng Lin wrote: > >> > >> On 2024/8/1 2:13, Alexander Duyck wrote: > >>> On Wed, Jul 31, 2024 at 5:50=E2=80=AFAM Yunsheng Lin wrote: > >>>> > >>>> Currently the page_frag API is returning 'virtual address' > >>>> or 'va' when allocing and expecting 'virtual address' or > >>>> 'va' as input when freeing. > >>>> > >>>> As we are about to support new use cases that the caller > >>>> need to deal with 'struct page' or need to deal with both > >>>> 'va' and 'struct page'. In order to differentiate the API > >>>> handling between 'va' and 'struct page', add '_va' suffix > >>>> to the corresponding API mirroring the page_pool_alloc_va() > >>>> API of the page_pool. So that callers expecting to deal with > >>>> va, page or both va and page may call page_frag_alloc_va*, > >>>> page_frag_alloc_pg*, or page_frag_alloc* API accordingly. > >>>> > >>>> CC: Alexander Duyck > >>>> Signed-off-by: Yunsheng Lin > >>>> Reviewed-by: Subbaraya Sundeep > >>> > >>> I am naking this patch. It is a pointless rename that is just going t= o > >>> obfuscate the git history for these callers. > >> > >> I responded to your above similar comment in v2, and then responded mo= re > >> detailedly in v11, both got not direct responding, it would be good to > >> have more concrete feedback here instead of abstract argument. > >> > >> https://lore.kernel.org/all/74e7259a-c462-e3c1-73ac-8e3f49fb80b8@huawe= i.com/ > >> https://lore.kernel.org/all/11187fe4-9419-4341-97b5-6dad7583b5b6@huawe= i.com/ > > > > I will make this much more understandable. This patch is one of the > > ones that will permanently block this set in my opinion. As such I > > will never ack this patch as I see no benefit to it. Arguing with me > > on this is moot as you aren't going to change my mind, and I don't > > have all day to argue back and forth with you on every single patch. > > Let's move on to more specific technical discussion then. > > > > > As far as your API extension and naming maybe you should look like > > something like bio_vec and borrow the naming from that since that is > > essentially what you are passing back and forth is essentially that > > instead of a page frag which is normally a virtual address. > > I thought about adding something like bio_vec before, but I am not sure > what you have in mind is somthing like I considered before? > Let's say that we reuse bio_vec like something below for the new APIs: > > struct bio_vec { > struct page *bv_page; > void *va; > unsigned int bv_len; > unsigned int bv_offset; > }; I wasn't suggesting changing the bio_vec. I was suggesting that be what you pass as a pointer reference instead of the offset. Basically your use case is mostly just for populating bio_vec style structures anyway. > It seems we have the below options for the new API: > > option 1, it seems like a better option from API naming point of view, bu= t > it needs to return a bio_vec pointer to the caller, it seems we need to h= ave > extra space for the pointer, I am not sure how we can avoid the memory wa= ste > for sk_page_frag() case in patch 12: > struct bio_vec *page_frag_alloc_bio(struct page_frag_cache *nc, > unsigned int fragsz, gfp_t gfp_mask); > > option 2, it need both the caller and callee to have a its own local spac= e > for 'struct bio_vec ', I am not sure if passing the content instead of > the pointer of a struct through the function returning is the common patt= ern > and if it has any performance impact yet: > struct bio_vec page_frag_alloc_bio(struct page_frag_cache *nc, > unsigned int fragsz, gfp_t gfp_mask); > > option 3, the caller passes the pointer of 'struct bio_vec ' to the calle= e, > and page_frag_alloc_bio() fills in the data, I am not sure what is the po= int > of indirect using 'struct bio_vec ' instead of passing 'va' & 'fragsz' & > 'offset' through pointers directly: > bool page_frag_alloc_bio(struct page_frag_cache *nc, > unsigned int fragsz, gfp_t gfp_mask, struct bio_= vec *bio); > > If one of the above option is something in your mind? Yes, please be more= specific > about which one is the prefer option, and why it is the prefer option tha= n the one > introduced in this patchset? > > If no, please be more specific what that is in your mind? Option 3 is more or less what I had in mind. Basically you would return an int to indicate any errors and you would be populating a bio_vec during your allocation. In addition you would use the bio_vec as a tracker of the actual fragsz so when you commit you are committing with the fragsz as it was determined at the time of putting the bio_vec together so you can theoretically catch things like if the underlying offset had somehow changed from the time you setup the allocation. It would fit well into your probe routines since they are all essentially passing the page, offset, and fragsz throughout the code.