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 CB6CBC3DA59 for ; Mon, 22 Jul 2024 15:21:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3356D6B0096; Mon, 22 Jul 2024 11:21:55 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2E4B36B0098; Mon, 22 Jul 2024 11:21:55 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1ACE56B0099; Mon, 22 Jul 2024 11:21:55 -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 E77446B0096 for ; Mon, 22 Jul 2024 11:21:54 -0400 (EDT) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id A81D1C1967 for ; Mon, 22 Jul 2024 15:21:54 +0000 (UTC) X-FDA: 82367753748.09.727E710 Received: from mail-wm1-f43.google.com (mail-wm1-f43.google.com [209.85.128.43]) by imf05.hostedemail.com (Postfix) with ESMTP id 973C0100034 for ; Mon, 22 Jul 2024 15:21:52 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=R18EmYMx; spf=pass (imf05.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.128.43 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=1721661690; 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=N29LyaaQQQ49iGyrUShmgCdAG5mooMQdbT0YkAs6M2k=; b=qcq1jUNYfItCTyVW8JdnuwfLDWK5XQcLinrijZl3ZtPlbLwf29GPWxP49+7Xetglk/Jz6M 0bs0paitgx3wmvoo01MfU16J+3l7LdRRMt85vBrDFsX17LShS+Lv2cfT9NIJTWOcl8rwl5 vrP3Ylv4/mZnH8uOfNTF5mW3TIoEMqw= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=R18EmYMx; spf=pass (imf05.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.128.43 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=1721661690; a=rsa-sha256; cv=none; b=VcKPkhI60GbQl7ZNIoxvM5SVACbRhAKn5bjifnRo+CmWg3qffd3r1KJVH9MpmrAmCiO4+X 6bYFTiS//sKLHAB03gdd1OUUTCO8g1/MOwF8ir8KfKXo5B/yGcjhGMoySlhHyU47pb2HV1 uXy5iFxH4yw3mwovU/ZlNUjhLtYxn0A= Received: by mail-wm1-f43.google.com with SMTP id 5b1f17b1804b1-4267345e746so33225625e9.0 for ; Mon, 22 Jul 2024 08:21:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1721661711; x=1722266511; 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=N29LyaaQQQ49iGyrUShmgCdAG5mooMQdbT0YkAs6M2k=; b=R18EmYMxwwF8lcQZff2PcAx5emgOyOZN8ziFIzp+i4BuYpX/9xdnS17G42SwGlP3lL R4/cGIo4Mnx5QOZVVkoDccS4fV3wPchil1mttb12ncRFM0GCYxiu11P6xHDg7h9Wbgat zcoPVeqYuikAjv6KDjsHoi0RZTYdEqhqJD1EEQUrYi/zxi6q7mGTJdfTFiiBtB/tl1pk CZgsRjrdga5fNwqf1gm7UwE3L/ws6SfJnuW3DeM0WhAyPHs4PBf954bVkiBlR6qT6ZdX ZE3Um5iyN16LnK2wIujTqTN56DEYRyOOD6mTG/HqohiA0YyBWI4/Mwj7uuH+ehN6jQYE 6zDw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1721661711; x=1722266511; 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=N29LyaaQQQ49iGyrUShmgCdAG5mooMQdbT0YkAs6M2k=; b=VbckO0LO8wYAuTc9pDE4gJ2SeZvEye0W7uEGoL+02NF9okZK1yxsNbMe3LJkqKrJ7c HMQg6+6KNZKYfwGnBYpIxjbyeiAdkbQSwejB/esDLPUFS5EhPqqFpPjNN60YGDb/wzoD W5x8mvY6rGS8n6VFu6BM4mH30TzCiglGyjWnFbqR0rSRUo1NbisQA2m6WJjdN+2vaza8 jpdMqWEdXRfbmwq5tN7Pg86ZC8D8fs5+HnnCPRTjBFMn5MZMuZhjlAPtePIb4t1Ze56A UthyWMtsiY2XBWNLFn/30zJuJYmFT/ar6ntB+NPC2nyp7no3omOby6bgcOom9eD0hFbt 2UdA== X-Forwarded-Encrypted: i=1; AJvYcCXb0wiFmp2RXuFlwMYZMeXQT0z7LpZiEa1NyeZSYiBG9w3E6+Gga0WwgU7a4QqwDhVeCb4Bc9dyj9Tf6guk84fPqRI= X-Gm-Message-State: AOJu0YwUh1N8j2rUJXHjpP6rBE1/LJaxuHji5csK7fDT46OE1+oL4/sL kKhpYYyz9D0fAgOfhfkmBoqzvq6Rm+LMRs9KnY47+iHoIu2PtGU4QNjWcNx0gduWzcbKbMMw/1v oP8hLS2hUw/T8BTz+GwjRvoVm9ww= X-Google-Smtp-Source: AGHT+IEz0LWym/I2F/H6DDah+69CAvBRKYq/jME2gt+KZ5jhSdIg1hrDcfrW6U37Wl92SDJaPpLlKLeTAeOoZ0S0P7s= X-Received: by 2002:a05:6000:b45:b0:368:12ef:92d0 with SMTP id ffacd0b85a97d-369bae703damr4117992f8f.51.1721661710503; Mon, 22 Jul 2024 08:21:50 -0700 (PDT) MIME-Version: 1.0 References: <20240719093338.55117-1-linyunsheng@huawei.com> In-Reply-To: From: Alexander Duyck Date: Mon, 22 Jul 2024 08:21:13 -0700 Message-ID: Subject: Re: [RFC v11 00/14] Replace page_frag with page_frag_cache for sk_page_frag() To: Yunsheng Lin Cc: davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Alexei Starovoitov , Daniel Borkmann , Jesper Dangaard Brouer , John Fastabend , Matthias Brugger , AngeloGioacchino Del Regno , bpf@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-mm Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam03 X-Rspam-User: X-Rspamd-Queue-Id: 973C0100034 X-Stat-Signature: aym5ky1hzjidezwnggmpn43hkmgo4kur X-HE-Tag: 1721661712-757979 X-HE-Meta: U2FsdGVkX1+NLqOqLS0g623ITVVw+CTl14kP3xBYK9EoAO+i0ijA3DruZ2nWuNHlhPNEAQYt6e3d9g4eiB0p6818hzqb7eYouKBjAzyadnxihOdr7VoB1zZRLJp7Lu+fB4rLIlh8gb60Gp6LGICTE/da+fAUYp0Tv7SmQeyQ8TUPMAPdtuSd4whjpzNSYTwLSXp3NLQ8ub3kFHpeX8VnxDntAOa0Pqg6zFdgxOEgBWDDJ+yMLUBktIkv+z+rsmDOvx3uAifh1rer92uw/3ccDDiGoW5RVxTDG/D4PCuoq16cJgN1Y/yUpgat7Vi8WqQAjDKT2Z5f0VJAGuDzTr/FluU/1y/s0d7tiZGEqRHbnsaRaMEm9nKbY+ZtEikCinghxHizDkyt/xQk22cNMv6tg7ao4gxv+/m1kn3az7RUuD1E9eKaUbraAkvTfotorvmii8XaE0RXaAreILr+fLwW3bwI7ludNy9DNaup/Sr6pRxQYzXsLt3ZXoAjWetGnwafv/cAg1J+e1T/seQVFBQ8t+luBXmgn96x6Aj4N6Id/zv7lff9VSzx7eDF3Njda5ct2qu3CrCpbtPFDf0xrOdyYCKlOW4XKMOokYEqM1x75EBSF1Pa7udDaM0QV7XQ+/+7VqoGqMWxET+4AEYz1bZyY7krMyPDfn1CfBiaRSbvQuLj7uCgaMv2Cs0rUs3aETqvWT5PINyxGPrE+HYjQR+roiwDH37JVY3WEkpq5g/+YpSV9qSWizU+3GzFs5U0eddzRpQc9assSYExTXUrqHYaBseM6TieEMVUrGQTGzR3dvfbyCnCo+TSCVqqvQ+RG7iozfshgYCUGD4wOhbM+aWLMBFas2AtoKQ0jGN5jR+282Ie6kTh2Ciutili5AYpVvgpfGap0qyF7pshLsIezpy54lKXChMO+NuouOk4YU63Jyeg70ei7e1p5e6EVuB93Gl/Y0CEVXF5V8ncJUssLaa Ryvxtes9 tYmsdxCkpFH/mjEtvlQE9NKGUtDBXrj8AuvK724CeRSLUafnNM/BITIW9XErR7EfWxzHwQzrfgf5RQg36WSkQDdtESgqakRWj9RtlDqxodeqDR1wWiwlCEz8frt3UQ+AWYpX4BYbkQffyWxDk+CJb0AtfGx2t4pqre29gxvIbnJEWsyhDF5cVdGI/63qyH65gVrQnSM//iF6GXDSFVsX4w/dgN4rwtlTCJll0FMt/NRSz8C5rxxVoFxw7M+nUZCdL4iiQf2oznsBiCIIvONRLqMyVehJjn2ZuhmbbBxx2TTTFDiOyJ2LcnMW5xOZ8YZu1Ovyd8/6uy5YX0+0FyM7TW2U0djuEY2Vxk553+Bdeu9kRxPcjMiAdliSlop0lgxGU+hzW0Y1SL6EXcVm+L5CSv/TzMfz12ZD9d9AZYcnO7HEbJ3JS4Swl1XMfMn/zn/KXno9buq0nLBTkrZzvYMzzvTX3vxlSCvroaBHtqb2BxE3DvPDPyES08GayHxt07a2YQa7uHbrMM6Kirf+f9cUq9kZtY+/xj64y+e0b 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, Jul 22, 2024 at 5:41=E2=80=AFAM Yunsheng Lin wrote: > > On 2024/7/22 7:49, Alexander Duyck wrote: > > On Fri, Jul 19, 2024 at 2:36=E2=80=AFAM Yunsheng Lin wrote: > >> > >> After [1], there are still two implementations for page frag: > >> > >> 1. mm/page_alloc.c: net stack seems to be using it in the > >> rx part with 'struct page_frag_cache' and the main API > >> being page_frag_alloc_align(). > >> 2. net/core/sock.c: net stack seems to be using it in the > >> tx part with 'struct page_frag' and the main API being > >> skb_page_frag_refill(). > >> > >> This patchset tries to unfiy the page frag implementation > >> by replacing page_frag with page_frag_cache for sk_page_frag() > >> first. net_high_order_alloc_disable_key for the implementation > >> in net/core/sock.c doesn't seems matter that much now as pcp > >> is also supported for high-order pages: > >> commit 44042b449872 ("mm/page_alloc: allow high-order pages to > >> be stored on the per-cpu lists") > >> > >> As the related change is mostly related to networking, so > >> targeting the net-next. And will try to replace the rest > >> of page_frag in the follow patchset. > > > > So in reality I would say something like the first 4 patches are > > probably more applicable to mm than they are to the net-next tree. > > Especially given that we are having to deal with the mm_task_types.h > > in order to sort out the include order issues. > > > > Given that I think it might make more sense to look at breaking this > > into 2 or more patch sets with the first being more mm focused since > > the test module and pulling the code out of page_alloc.c, gfp.h, and > > mm_types.h would be pretty impactful on mm more than it is on the > > networking stack. After those changes then I would agree that we are > > mostly just impacting the network stack. > > I am sure there are plenty of good precedents about how to handling a > patchset that affecting multi subsystems. > Let's be more specific about what are the options here: > 1. Keeping all changing as one patchset targetting the net-next tree > as this version does. > 2. Breaking all changing into two patchsets, the one affecting current AP= Is > targetting the mm tree and the one supporting new APIs targetting > net-next tree. > 3. Breaking all changing into two patchset as option 2 does, but both pat= chsets > targetting net-next tree to aovid waiting for the changing in mm tree > to merged back to net-next tree for adding supporting of new APIs. > > I am not sure your perference is option 2 or option 3 here, or there are = others > options here, it would be better to be more specific about your option he= re. As > option 2 doesn't seems to make much sense if all the existing users/calle= rs of > page_frag seems to be belonged to networking for testing reasons, and the= original > code seemed to go through net-next tree too: > https://github.com/torvalds/linux/commit/b63ae8ca096dfdbfeef6a209c30a93a9= 66518853 I am suggesting option 2. The main issue is that this patch set has had a number of issues that fall into the realm of mm more than netdev. The issue is that I only have a limited amount of time for review and I feel like having this be reviewed as a submission for mm would bring in more people familiar with that side of things to review it. As it stands, trying to submit this through netdev is eating up a significant amount of my time as there aren't many people on the netdev side of things that can review the mm bits. If you insist on this needing to go through net-next my inclination would be to just reject the set as it is bound to introduce a number of issues due to the sheer size of the refactor and the fact that it is providing little if any benefit. > And the main reason I chose option 1 over option 2 is: it is hard to tell= how > much changing needed to support the new usecase, so it is better to keep = them > in one patchset to have a bigger picture here. Yes, it may make the patch= set > harder to review, but that is the tradeoff we need to make here. As my > understanding, option 1 seem to be the common practice to handle the chan= ging > affecting multi subsystems. Especially you had similar doubt about the ch= anging > affecting current APIs as below, it seems hard to explain it without a ne= w case: > > https://lore.kernel.org/all/68d1c7d3dfcd780fa3bed0bb71e41d7fb0a8c15d.came= l@gmail.com/ The issue as I see it is that you aren't getting any engagement from the folks on the mm side. In fact from what I can tell it looks like you didn't even CC this patch set to them. The patches I called out below are very memory subsystem centric. I would say this patchset has no way forward if the patches I called out below aren't reviewed by folks from the memory subsystem maintainers. > > > > ... > > > > > So specifically I would like to see patches 1 (refactored as > > selftest), 2, 3, 5, 7, 8, 13 (current APIs), and 14 done as more of an > > mm focused set since many of the issues you seem to have are problems > > building due to mm build issues, dependencies, and the like. That is > > the foundation for this patch set and it seems like we keep seeing > > issues there so that needs to be solid before we can do the new API > > work. If focused on mm you might get more eyes on it as not many > > networking folks are that familiar with the memory management side of > > things. > > I am not sure if breaking it into more patchset is the common practice > to 'get more eyes' here. > Anyways, it is fair enough ask if there is more concrete reasoning > behind the asking and it is common practice to do that, and I would > love to break it to more patchsets to perhaps make the discussion > easier. The issue here is that this patchset is 2/3 memory subsystem, and you didn't seem to include anyone from the memory subsystem side of things on the Cc list. > > > > As for the other patches, specifically 10, 11, 12, and 13 (prepare, > > probe, commit API), they could then be spun up as a netdev centered > > set. I took a brief look at them but they need some serious refactor > > as I think they are providing page as a return value in several cases > > where they don't need to. > > The above is one of the reason I am not willing to do the spliting. > It is hard for someone to tell if the refactoring affecting current APIs > will be enough for the new usecase without supporting the new usecase, > isn't it possible that some refactoring may be proved to be unnecessary > or wrong? > > It would be better to be more specific about what do you mean by > 'they are providing page as a return value in several cases where they > don't need to' as above. This patchset isn't moving forward in its current state. Part of the issue is that it is kind of an unwieldy mess and has been difficult to review due to things like refactoring code you had already refactored. Ideally each change should be self contained and you shouldn't have to change things more than once. That is why I have suggested splitting things the way I did. It would give you a logical set where you do the initial refactor to enable your changes, and then you make those changes. It is not uncommon to see this done within the kernel community. For example if I recall correctly the folio changes when in as a few patch sets in order to take care of the necessary enabling and then enable their use in the various subsystems. > > > > In my opinion with a small bit of refactoring patch 4 can just be > > dropped. I don't think the renaming is necessary and it just adds > > noise to the commit logs for the impacted drivers. It will require > > tweaks to the other patches but I think it will be better that way in > > the long run. > > It would be better to be more specific about above too so that we don't > have to have more refactoring patchsets for the current APIs. I provided the review feedback in the patch. Specifically, don't rename existing APIs. It would be better to just come up with an alternative scheme such as a double underscore that would represent the page based version while the regular version stays the same. > > > > Looking at patch 6 I am left scratching my head and wondering if you > > have another build issue of some sort that you haven't mentioned. I > > really don't think it belongs in this patch set and should probably be > > a fix on its own if you have some reason to justify it. Otherwise you > > might also just look at refactoring it to take > > "__builtin_constant_p(size)" into account by copying/pasting the first > > bits from the generic version into the function since I am assuming > > there is a performance benefit to doing it in assembler. It should be > > a net win if you just add the accounting for constants. > > I am not sure if the commit log in patch 6 needs some rephasing to > answer your question above: > "As the get_order() implemented by xtensa supporting 'nsau' > instruction seems be the same as the generic implementation > in include/asm-generic/getorder.h when size is not a constant > value as the generic implementation calling the fls*() is also > utilizing the 'nsau' instruction for xtensa. > > So remove the get_order() implemented by xtensa, as using the > generic implementation may enable the compiler to do the > computing when size is a constant value instead of runtime > computing and enable the using of get_order() in BUILD_BUG_ON() > macro in next patch." > > See the below in the next patch, as the PAGE_FRAG_CACHE_MAX_ORDER > is using the get_order(): > BUILD_BUG_ON(PAGE_FRAG_CACHE_MAX_ORDER > PAGE_FRAG_CACHE_ORDER_MASK); Are you saying that the compiler translates the get_order call into the nsau instruction? I'm still not entirely convinced and would really like to see a review by the maintainer for that architecture to be comfortable with it. Otherwise as I said before my thought would be to simply copy over the bits for __builtin_constant_p from the generic version of get_order so that we don't run the risk of somehow messing up the non-constant case. > > > > Patch 9 could probably be a standalone patch or included in the more > > mm centered set. However it would need to be redone to fix the > > underlying issue rather than working around it by changing the > > function called rather than fixing the function. No point in improving > > it for one case when you can cover multiple cases with a single > > change. > > Sure, it is just that there is only 24h a day for me to do things > more generically. So perhaps I should remove patch 9 for now so > that we can improve thing more generically. I'm not sure what that is supposed to mean. The change I am suggesting is no bigger than what you have already done. It would just mean fixing the issue at the source instead of working around the issue. Taking that approach would yield a much better return than just doing the workaround. I could make the same argument about reviewing this patch set. I feel like a I only have so much time in the day. I have already caught a few places where you were circumventing issues instead of addressing them such as using macros to cover up #include ordering issues resulting in static inline functions blowing up. It feels like labeling this as a networking patch set is an attempt to circumvent working with the mm tree by going in and touching as much networking code as you can to claim this is a networking patch when only 3 patches(5, 10 and 12) really need to touch anything in networking. I am asking you to consider my suggestions for your own benefit as otherwise I am pretty much the only reviewer for these patches and the fact is I am not a regular contributor within the mm subsystem myself. I would really like to have input from the mm subsystem maintainer on things like your first patch which is adding a new test module to the mm tree currently. I am assuming that they wouldn't want us to place the test module in there, but I could be wrong. That is why I am suggesting breaking this up and submitting the mm bits as more mm focused so that we can get that additional input.