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 47563C531DC for ; Tue, 20 Aug 2024 16:03:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A49E16B0083; Tue, 20 Aug 2024 12:03:54 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 9F9D86B0085; Tue, 20 Aug 2024 12:03:54 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 89CBA6B0088; Tue, 20 Aug 2024 12:03:54 -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 6B34A6B0083 for ; Tue, 20 Aug 2024 12:03:54 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 97A09140346 for ; Tue, 20 Aug 2024 16:03:52 +0000 (UTC) X-FDA: 82473094704.23.303E2C8 Received: from mail-wr1-f42.google.com (mail-wr1-f42.google.com [209.85.221.42]) by imf29.hostedemail.com (Postfix) with ESMTP id D3785120045 for ; Tue, 20 Aug 2024 16:03:36 +0000 (UTC) Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Wgg4fyUg; spf=pass (imf29.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.221.42 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=1724169740; 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=ACxpErlOvFQuBRokph/wthh0GeLIFddWl+sG+UC+KHk=; b=eAPNhtc0hgUdX1PJfU5G2dJZKbz/kIPZnxliETj+R/rkZIwBosGfL2Qjm+8c6buZaAyxHA K4+2lj1JENVg3GyaokXlPakMwjh8aIUITemwiSuPXsD2KxUrZQZA5ZQa6/FnUS1vJjUVX7 EdU7FQVBeIR6LDJuleXBZ359tv9FGMY= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1724169740; a=rsa-sha256; cv=none; b=Vv8Hzf0LQn6VyJHScuJbqBeHQqYShmWhw9Kgjam8xFROWYUoiYQ6PXHToB76sfMqZrfFpq hycwkTTVIunzoxAtLlDT9IAos6arWyTWj/r/N4CWALo7ZnWq8r1zmOwF2BcX2vF08SeWfw rVzsDNBpsTBvYoTHESUTNOTs7r03S30= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Wgg4fyUg; spf=pass (imf29.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.221.42 as permitted sender) smtp.mailfrom=alexander.duyck@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-wr1-f42.google.com with SMTP id ffacd0b85a97d-371a9bea8d4so1894064f8f.2 for ; Tue, 20 Aug 2024 09:03:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1724169815; x=1724774615; 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=ACxpErlOvFQuBRokph/wthh0GeLIFddWl+sG+UC+KHk=; b=Wgg4fyUgsICizTsorAn+ENQg5551guWT8BVmj8vKuE9MdfxHODhLhAATR0rSUK10fG TSW9wvzzQbtOUFCqr84+Ev283pcLb19skf3NolYh7syLWnGxz2ZTvfH1GvPEU8p5+w8k L2BLiALJO4Ft+hwM0dm3WNw9QMMANtM1jY1lAhncZ5ofH8prOm6KFM/c8o6RggM0dLYc FaIetQcpP6pDHKL8wTTlhb8JZ7sveVKjwv8GjlIN0jWOuAqgtO+XoMSyy8xVr+6xx1BX fkvBBqb47yuEBRsHbO/1JXNsdpBri7mEckpi+eSsZY6N3BKKUIeTUB+lqbhe6AqfSffu BGbA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724169815; x=1724774615; 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=ACxpErlOvFQuBRokph/wthh0GeLIFddWl+sG+UC+KHk=; b=q5oYhlPgXf8EkgNe2e9SJzDbaXrJX+hM9BZiSTYx87t/Ebo8RBv0HOYFxLfpD+CjAf 7ZLu6NNu7q8ZSUZhDgCK0kD43kwWTtnRtFU3gUlkeUISJJMtzyPNuzYIrItUKwYu6SJE 6vOtFnPvuD91QVPxymgcygkfKtyiqGLmHPCIZGw+iiL6ycMskWH0JeZFKiFh3Y9DpUKk +K+Xrt1D2J9IcEa3jrxRI+WOq3ZAZd9K19RQ4KZL0eWgcVS5oWXayZ6stoPmgbCzhEX8 pKIQ5ujFx558KFoD3A9KPxiXqwI5jYPgGw8gBY/rob1KhXHrvD+DR7WberT3uQiIDQag 9TkQ== X-Forwarded-Encrypted: i=1; AJvYcCV4+oT5587F1TtSGe06fRJM4aDQwKH/P7kkwmvUN8jFjf1DoJljfwA4qUsk4kJtkGW8VZIfnFfjnQ==@kvack.org X-Gm-Message-State: AOJu0Yzrbceu4PfGXMKXkCoBzXDAVHpI/A8Ynaq+IkrPydurMtu7m+7L KuqndYDDPGqfQqwIkZgRlHTJkPky3w/FNSnVY9YfBv72S7AEacnANXRT9RbZDQt4e1weFqbF33G tiyQf1qpyBfd4SBrY2E/lRoqH8ZE= X-Google-Smtp-Source: AGHT+IF4DoS60WZqDqVaq8Y/5SjjI7zscaIxzpj60ezQwLm3viTYLpGZpmD+jTekneRZUAoDJMoW7JwgYwWaXYZHKdQ= X-Received: by 2002:adf:f285:0:b0:371:888d:7aaa with SMTP id ffacd0b85a97d-371946b1ae9mr9087932f8f.49.1724169814881; Tue, 20 Aug 2024 09:03:34 -0700 (PDT) MIME-Version: 1.0 References: <20240808123714.462740-1-linyunsheng@huawei.com> <20240808123714.462740-5-linyunsheng@huawei.com> <676a2a15-d390-48a7-a8d7-6e491c89e200@huawei.com> <3e069c81-a728-4d72-a5bb-3be00d182107@huawei.com> <98ceade3-8d60-45bf-a419-ff3982a96101@huawei.com> In-Reply-To: <98ceade3-8d60-45bf-a419-ff3982a96101@huawei.com> From: Alexander Duyck Date: Tue, 20 Aug 2024 09:02:57 -0700 Message-ID: Subject: Re: [PATCH net-next v13 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 , Chuck Lever , Sagi Grimberg , 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 , 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 , Jeff Layton , Neil Brown , Olga Kornievskaia , Dai Ngo , Tom Talpey , Trond Myklebust , Anna Schumaker , Shuah Khan , 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, linux-kselftest@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: D3785120045 X-Stat-Signature: j1xm1ntdkbhqdgs7gxqnbm918jk1kowf X-HE-Tag: 1724169816-271990 X-HE-Meta: U2FsdGVkX18OWBbwphp3oV1l7nuGVZTfvqrG0LhDm1GMGMZUu26g3UwHMAV4vn9ud9VmvttJ7vXa5ZkLK0e5GQHatBlGyoLkyOubjXvlTQfK8OYPn24iekwn0yHXyFVCny872M9ub9ZhqV+J4FATsW2XKT4IdYfXuSFd5laRF5SUOBMonJXZ0yNAvZw8D8Dccp7LOBncI7nfZ8FCGWGYm0URKkrayNMNMAIka4A2+niQm3BRNy3ouHwMZa3a5tP9nn6NE/tHv2TIA0BmoiwpWcDGf658l31no49JZz1dvR+JMjGYACyauHCCsUoXHohKLLhvCID3kAyTzroa5OFDRdQKBpH6UAMWjZSThYGLFl0KqCkz2md3KX6/FU757a/hSKb0YVepIF9ug3ot7lJQSOmFBgXcdrTXll1RlP9bZXjWRQfxukJkiwHruhVDFjmas/8b/fiSQ9HTcMthCFPGrCIesYhkUru7cYoHGY2uvloooXFXg+kaK8e9fDv5XXcf6XmZYccbkUFEUYf5hjUOAm0b+TuSIQF23/9uFf2YHTnyeZeVrlo782FsCC6bUlRJz5VgBN6soWiIL15iBL121zTSSDTlMVHy0ndkm+/gbL+8KXkWqbduJq2058zL0yksNCl7l8m5r9R7fwdT54zyouY91J5L+D9WCmpVldSYTTWNouVtTk0GrEU71Cfbe7WjRV7AlfnoKZaU71pVkrc6QO81bBC1RQNtDWjydt+nfo+oOOKrnG4rUwGiO30TT1dU59kgi8wuKHgf4oaOwsWEITYk+mO4fv9Q4KfTedI99u01IxNSfRMoVtn+0PaiLmMcP/laEAW7ltXzmyG5Gv2YFWvWU53WVrvY/IDfDdVm5c2pKIMrDczO7eggrmemjr39s5RbWWdsp5ds/MVt+xh3YMtjBYE8fjTsUgEkCdz5+68M4LVNH/jUaKhuRBnEAHd2CCKrJ+26mkr0m9+7yHn 6AW7P3OP ddZBeEgVIXbTL0Wy9DlhT5zc3NHRGvekbgS/ilIweVOAPZjkkQ9WCWX5OPHAUoI9Cga9+7PTNogmit8GkEQrFaKUAYklO/QdXh/5LJEV1ZvpRsoRhVU3CbPEtSK5C5c+DlvfYepiqL+dCaCFvbzXwtH9mXHIuCdhjLT9arxJWb0pF69hzZaCM+P3uGOz73EbbDGCjwtSHga0PTrQZnXE10I24sFgO+5Mj64fK8jPSv4EiHruIVrt5WEvh5UdKlicJjV6icNFk0OCtAw5Rdmsa0yDhnrU0B5+z89/xNAe52V9y5rObx5mSONwgvwyEoZxSDQyQKFay82U361j7lCaFrHUJyIo9UWwqZuBUjNIBGQRwDVc/JNx7eQzAI0FMC/RZihkAGE8VJnjNpUIRRYJgpBs4RmR8ejF4oqvlsiAup0mT3pz8GFIkp4WsMH6yUISHUrLSc0loOBo53qABhQC9bOBKoA== 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 Tue, Aug 20, 2024 at 6:07=E2=80=AFAM Yunsheng Lin wrote: > > On 2024/8/19 23:54, Alexander Duyck wrote: > > ... > > >>>> > >>>> "There are three types of API as proposed in this patchset instead o= f > >>>> two types of API: > >>>> 1. page_frag_alloc_va() returns [va]. > >>>> 2. page_frag_alloc_pg() returns [page, offset]. > >>>> 3. page_frag_alloc() returns [va] & [page, offset]. > >>>> > >>>> You seemed to miss that we need a third naming for the type 3 API. > >>>> Do you see type 3 API as a valid API? if yes, what naming are you > >>>> suggesting for it? if no, why it is not a valid API?" > >>> > >>> I didn't. I just don't see the point in pushing out the existing API > >>> to support that. In reality 2 and 3 are redundant. You probably only > >>> need 3. Like I mentioned earlier you can essentially just pass a > >> > >> If the caller just expect [page, offset], do you expect the caller als= o > >> type 3 API, which return both [va] and [page, offset]? > >> > >> I am not sure if I understand why you think 2 and 3 are redundant here= ? > >> If you think 2 and 3 are redundant here, aren't 1 and 3 also redundant > >> as the similar agrument? > > > > The big difference is the need to return page and offset. Basically to > > support returning page and offset you need to pass at least one value > > as a pointer so you can store the return there. > > > > The reason why 3 is just a redundant form of 2 is that you will > > normally just be converting from a va to a page and offset so the va > > should already be easily accessible. > > I am assuming that by 'easily accessible', you meant the 'va' can be > calculated as below, right? > > va =3D encoded_page_address(encoded_va) + > (page_frag_cache_page_size(encoded_va) - remaining); > > I guess it is easily accessible, but it is not without some overhead > to calculate the 'va' here. It is just the encoded_page_address + offset that you have to calculate anyway. So the only bit you actually have to do is 2 instructions, one to mask the encoded_va and then the addition of the offset that you provided to the page. As it stands those instruction can easily be slipped in while you are working on converting the va to a page. > > > >>> page_frag via pointer to the function. With that you could also look > >>> at just returning a virtual address as well if you insist on having > >>> something that returns all of the above. No point in having 2 and 3 b= e > >>> seperate functions. > >> > >> Let's be more specific about what are your suggestion here: which way > >> is the prefer way to return the virtual address. It seems there are tw= o > >> options: > >> > >> 1. Return the virtual address by function returning as below: > >> void *page_frag_alloc_bio(struct page_frag_cache *nc, struct bio_vec *= bio); > >> > >> 2. Return the virtual address by double pointer as below: > >> int page_frag_alloc_bio(struct page_frag_cache *nc, struct bio_vec *bi= o, > >> void **va); > > > > I was thinking more of option 1. Basically this is a superset of > > page_frag_alloc_va that is also returning the page and offset via a > > page frag. However instead of bio_vec I would be good with "struct > > page_frag *" being the value passed to the function to play the role > > of container. Basically the big difference between 1 and 2/3 if I am > > not mistaken is the fact that for 1 you pass the size, whereas with > > 2/3 you are peeling off the page frag from the larger page frag cache > > Let's be clear here: The callers just expecting [page, offset] also need > to call type 3 API, which return both [va] and [page, offset]? and it > is ok to ignore the overhead of calculating the 'va' for those kinds > of callers just because we don't want to do the renaming for a existing > API and can't come up with good naming for that? > > > after the fact via a commit type action. > > Just be clear here, there is no commit type action for some subtype of > type 2/3 API. > > For example, for type 2 API in this patchset, it has below subtypes: > > subtype 1: it does not need a commit type action, it just return > [page, offset] instead of page_frag_alloc_va() returning [va], > and it does not return the allocated fragsz back to the caller > as page_frag_alloc_va() does not too: > struct page *page_frag_alloc_pg(struct page_frag_cache *nc, > unsigned int *offset, unsigned int fragsz= , > gfp_t gfp) > > subtype 2: it does need a commit type action, and @fragsz is returned to > the caller and caller used that to commit how much fragsz to > commit. > struct page *page_frag_alloc_pg_prepare(struct page_frag_cache *nc, > unsigned int *offset, > unsigned int *fragsz, gfp_t gfp) > > Do you see subtype 1 as valid API? If no, why? Not really, it is just a wrapper for page_frag_alloc that is converting the virtual address to a page and offset. They are the same data and don't justify the need for two functions. It kind of explains one of the complaints I had about this code. Supposedly it was refactoring and combining several different callers into one, but what it is actually doing is fracturing the code path into 3 different variants based on little if any actual difference as it is doing unnecessary optimization. > If yes, do you also expect the caller to use "struct page_frag *" as the > container? If yes, what is the caller expected to do with the size field = in > "struct page_frag *" from API perspective? Just ignore it? It should be populated. You passed a fragsz, so you should populate the output fragsz so you can get the truesize in the case of network packets. The removal of the page_frag from the other callers is making it much harder to review your code anyway. If we keep the page_frag there it should reduce the amount of change needed when you replace page_frag with the page_frag_cache. Honestly this is eating up too much of my time. As I said before this patch set is too big and it is trying to squeeze in more than it really should for a single patch set to be reviewable. Going forward please split up the patch set as I had suggested before and address my comments. Ideally you would have your first patch just be some refactor and cleanup to get the "offset" pointer moving in the direction you want. With that we can at least get half of this set digested before we start chewing into all this refactor for the replacement of page_frag with the page_frag_cache.