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 8F011E77197 for ; Thu, 9 Jan 2025 20:33:05 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id F2E7A8D0003; Thu, 9 Jan 2025 15:33:04 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id EDECA8D0002; Thu, 9 Jan 2025 15:33:04 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D591A8D0003; Thu, 9 Jan 2025 15:33:04 -0500 (EST) 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 B1CED8D0002 for ; Thu, 9 Jan 2025 15:33:04 -0500 (EST) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 42AD2A051F for ; Thu, 9 Jan 2025 20:33:04 +0000 (UTC) X-FDA: 82989062688.10.1D8102E Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by imf25.hostedemail.com (Postfix) with ESMTP id 77F15A0009 for ; Thu, 9 Jan 2025 20:33:02 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=cOYiY66I; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf25.hostedemail.com: domain of a.hindborg@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=a.hindborg@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1736454782; a=rsa-sha256; cv=none; b=Z20WQx/QZhJcoTX/xzyXfxMhlEhrNQxW3JsVu+B0uMJbiM3Kzi4/4ZQYWeQk1VDKjhSR0K xzbum4MM+NhEse0J5U9uFXFg/OURj1gaYBJz76qyO8v6ia93rXxsfNRXBuLPNLrwHLUVbf R4IxdxqeVe51gZRxSgnKQNWahUXoWoo= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=cOYiY66I; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf25.hostedemail.com: domain of a.hindborg@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=a.hindborg@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1736454782; 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=CsHp1d32yE1YqsYbxSPmW+93I01ryxo5RZPG5sOYnoY=; b=lELtSwKBVh4tvtMHsS1XzVIz1Ipjv4uBH1oMfZb6+k7y1Rz42bp1CUBNCPTWxI3nQaUASs JyRVW8FT8fOoY4HQ1+nmflZvMI02DroHqlM2LQVTGA4eoaARKfqsxJvkEK0QXuULgXWN/K W/9+sBCGvu4gmdPJlCB0ScEy8ykT+JA= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id BC794A42660; Thu, 9 Jan 2025 20:31:12 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 07FCAC4CED2; Thu, 9 Jan 2025 20:32:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1736454780; bh=ux8AdvBQRWc8x5GVq76mOl2ItrF9N8b+fdfFajokARk=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=cOYiY66IWYKR7zk/Oa3TAcGrU/3Ks0vqdMSJ91RIpXSDdJzFWo7qv4dHM+J6uzzje o+4+ivSfoXTq3f9WFL/DHwcZmqjQt0qIO3KJCkyYLn1JIlJc6/uL72sswLMh3FSe5B TZ/kOlfPR+wfOieBaO8QnPXe9SSOan+hSphOIquRXIDj08tPcnXLP31RV9WgX6dw/y vQ8wme07AGQ1dVwZ5aYevxBPhmZa0Yhxu684F6oBr2WPEpaysHEDgS/2kd/9+55fTb Hb/pA6FH5AyiabMpUPYjZtZfL4OLD+TUOwT1wOYeAytekV0icvVv7vdOF1ufQlVXXm bYrZxnWGT+CNw== From: Andreas Hindborg To: "Lorenzo Stoakes" Cc: "Alice Ryhl" , "Miguel Ojeda" , "Matthew Wilcox" , "Vlastimil Babka" , "John Hubbard" , "Liam R. Howlett" , "Andrew Morton" , "Greg Kroah-Hartman" , "Arnd Bergmann" , "Christian Brauner" , "Jann Horn" , "Suren Baghdasaryan" , "Alex Gaynor" , "Boqun Feng" , "Gary Guo" , =?utf-8?Q?Bj=C3=B6rn?= Roy Baron , "Benno Lossin" , "Trevor Gross" , , , Subject: Re: [PATCH v11 2/8] mm: rust: add vm_area_struct methods that require read access In-Reply-To: <8b803030-0ca3-4591-b2f3-bb9bcc2aca21@lucifer.local> (Lorenzo Stoakes's message of "Thu, 09 Jan 2025 11:29:43 +0000") References: <20241211-vma-v11-0-466640428fc3@google.com> <20241211-vma-v11-2-466640428fc3@google.com> <874j33ddxt.fsf@kernel.org> <51PsGz5tctBZlyC7TWAGRwZbM13r71BM1gtm1Y8F4j2w3FtKSXtkVrsvwILAqvSBPrFJzahyUVDum-JXO3yZUw==@protonmail.internalid> <87frlsbekc.fsf@kernel.org> <18bc911a-ede5-410b-9955-5382bcef975c@lucifer.local> <87msg09uzu.fsf@kernel.org> <8b803030-0ca3-4591-b2f3-bb9bcc2aca21@lucifer.local> User-Agent: mu4e 1.12.7; emacs 29.4 Date: Thu, 09 Jan 2025 16:32:13 +0100 Message-ID: <871pxcgg02.fsf@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 77F15A0009 X-Stat-Signature: mi1rbky5du1oixfinth7zamzs8ey8e6n X-HE-Tag: 1736454782-817358 X-HE-Meta: U2FsdGVkX19fS16aTj2CJAePDg4WyQHPlkOaSA4bCEgF4ytVu4fDjCljUhyVgGcsO1IHYfY2PxheftudHSt+UyI+pP0wKkZVKbyeaV41BPT8wDPxVU9qSm0JOehYIJtzVYF83PIijQJJpqkmlBftmFOqn5Pte4PQs7T35ApOf2KLEQj2FHJASnH+wOj1k5Lqxg1Bnm3e+PUh3JIrDEF2izll3pPWMo27/M5ag4iJXVk2jJyelG3PCZBkKyrDDuFyo6gnxYTOu/4ZwCDhkA7o/2fnNqgRlL6L20XRKHi1LJ9v/Z+KJ9zcrku5XLjadP4UwQipB16UU0nIQxSPye+Q+gLNDmlnxiQQW2LLDPxMrAlBWKImB9LHQTw1q7CGVcE58Kb5UMZYudsecfiUS7UlbXxvfG7IG3ML41nBpjNkNdwGhMsi16obuNDjiIG3/szIddUs92ysCkCqm1tFPAWHq8draXJZr+1n03BzgC3ljCl5zviHfSm5w3fTi2a93IoIm+EKVhnqV/qpp4lF3OpMxzaBXb7SJJzVqE2K0HSD63anUlQtkLw3dQq2Uh0I6+JxJUwmTkNFBibU0assBOlrng+5/VB0K7DOAOFdBuOXEngecfnCbwqblIGIN+noF/gwxQcujku9m+SgwnptVA4zI/+d2dA3eIqqXWh73jhOESu+sNIVv2UHArPNFOdmNUWCvgCvVWCEtwvEO4BU1vgilkNK9sT57pOSRzSL3Fzc6f1wN+4KPeHS1ylPo5mIF6zUkUEC0iz54G1xiwZ/rIUMW4tm/Ec0Pz6I/7kfQbJ9/VexvMBTq5KcqLbLzEpFj9IoYQCf57qaOwFlVfvXomTatKiqCcfVHy0KihvZPzTdQf8rRpQbWBB66X1x8BYUQkS8+1SCKu6IFX3TmTqhHxfNKCEd+SjHKThv8siiPdxQG5yt03BjvdLRicXGNV5o+CHRu6H/VCqk0Q2SHR6ZnK0 dE3Jton2 cuVKRv3nNu8GlE7zN8FZvc7XjnssNh8VEkTIZkfR3qN2XY7hrjQmmNPiZ6xm+O0hMKnr9vglmrTwDVA2sU+YZ9Aj+3rLdTXCfEpiZWfcDwONPBXSSaYJlaw1Ecl4+5KI4pW0BMuPnefbN3Q698l4b/7piijYShwpyF6LS+CtOHuS+J55hcwRzXtdekjSbEi6dOxulNPrgfewlj/Eptjak5SEzmxMfajxz64SurJ7VF894DesFeyCc4kirCjHaOJK6xHr/t4Sev5VLk3lZgzlbPzau31M6hm+42u1e1lylQ1DE+62d5OpklJlSa+2Xw/Zd72yfBzawdbjjK3qY+e2QDLo5KLkZUt6jockgAGHje1cSY82rixNrtIZJ3PWq73v+1vn6ZCnfsEkvFx4POlEefdwOyL0Wg9MqfFi8XPtD+tVJlj8= X-Bogosity: Ham, tests=bogofilter, spamicity=0.380596, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: "Lorenzo Stoakes" writes: > On Thu, Jan 09, 2025 at 10:50:13AM +0100, Andreas Hindborg wrote: >> "Lorenzo Stoakes" writes: >> >> > On Thu, Jan 09, 2025 at 09:02:11AM +0100, Andreas Hindborg wrote: >> >> "Alice Ryhl" writes: >> >> >> >> > On Mon, Dec 16, 2024 at 3:51=E2=80=AFPM Andreas Hindborg wrote: >> >> >> >> >> >> >> >> >> > + >> >> >> > + /// Zap pages in the given page range. >> >> >> > + /// >> >> >> > + /// This clears page table mappings for the range at the le= af level, leaving all other page >> >> >> > + /// tables intact, >> >> >> >> >> >> I don't fully understand this docstring. Is it correct that the fu= nction >> >> >> will unmap the address range given by `start` and `size`, _and_ fr= ee the >> >> >> pages used to hold the mappings at the leaf level of the page tabl= e? >> >> > >> >> > If the vma owns a refcount on those pages, then the refcounts are d= ropped. >> >> >> >> Maybe drop the "at the leaf level leaving all other page tables intac= t". >> >> It confuses me, since when would this not be the case? >> > >> > I don't understand your objection. The whole nature of a zap is to tra= verse >> > leaf level page table mappings, clearing the entries, leaving the other >> > page table entries intact. >> >> As someone not deeply familiar with this function and it's use, I became >> uncertain of my understanding when I read this sentence. As I asked >> above: When would you not clear mappings at the leaf level and leave all >> other mappings alone? > > Because these are page tables and page tables can span multiple PTE > tables. Correctly removing at the time of clearing would be expensive and > require very careful handling. What is the distinction between clearing a PTE and removing it? I asked above if the leaf page holding the PTEs would be dropped if all the PTEs it holds are cleared. Alice "If the vma owns a refcount on those p= ages, then the refcounts are dropped.". But from your message I am guessing maybe not? > >> >> Imagine you have a collection structure backed by a tree and the >> `remove_item` function has the sentence "remove item at the leaf level >> but leave all other items in the collection alone". That would be over >> specifying. It is enough information in the user facing documentation >> that the item is removed. You don't need to state that a remove >> operation on an item does not remove other items. Does this example >> transfer to this function, or am I missing something? > > No, because we're dealing with page tables and you are explicitly request= ing a > page table operation. Knowing what is touched is meaningful. When would a page table operation to remove (clear?) the PTEs corresponding to an address range touch PTEs corresponding to addresses outside of the range? > >> >> > That is, precisely what is written here. In fact I think this >> > characterisation is derived from discussions had with us in mm, and it= is >> > one with which I am happy. >> > >> > Why is it problematic to accurately describe what this does? >> >> Again, it might be that I don't properly understand what the function >> actually does, but if it is just removing the entries described by the >> range - write that. Don't add irrelevant details or specify what the >> function does not do. It slows down the user when reading documentation. > > It is highly pertinent as mentioned above. > > I mean we can expand the comment to explicitly add some detail around this > since obviously this is confusing (hey - a lot of mm is confusing - this = is > an ongonig problem and why I have gone to lengths to try to improve > documentation and wrote a book about it :) That would be nice :) > >> >> > >> > For a series at v11 where there is broad agreement with maintainers wi= thin >> > the subsystem which it wraps, perhaps the priority should be to try to= have >> > the series merged unless there is significant technical objection from= the >> > rust side? >> > >> >> >> >> How about this: >> >> >> >> This clears the virtual memory map for the range given by `start` and >> >> `size`, dropping refcounts to memory held by the mappings in this ran= ge. That >> >> is, anonymous memory is completely freed, file-backed memory has its >> >> reference count on page cache folio's dropped, any dirty data will st= ill >> >> be written back to disk as usual. >> > >> > Sorry I object to this, 'clears the virtual memory map' is really >> > vague. What is already there is better. >> >> Would you like the proposed paragraph if we replaced "virtual memory >> map" with "page table mappings", or do you object to the entirety of the >> new suggestion? > > I object to the suggestion in general. The description is fine as it is. Ok. I'm raising a flag because I had more questions after reading the docstring than before. > >> >> > >> >> >> >> > >> >> >> > and freeing any memory referenced by the VMA in this range. That= is, >> >> >> > + /// anonymous memory is completely freed, file-backed memor= y has its reference count on page >> >> >> > + /// cache folio's dropped, any dirty data will still be wri= tten back to disk as usual. >> >> >> > + #[inline] >> >> >> > + pub fn zap_page_range_single(&self, address: usize, size: u= size) { >> >> >> >> >> >> Best regards, >> >> Andreas Hindborg >> >> >> >> >> > >> > Let's please get this series merged. I think Alice has demonstrated >> > remarkable patience already, and modulo significant technical pushback= on >> > the rust side (on which I defer entirely to the expertise of rust peop= le), >> > I want to see this go in. >> >> I am sensing that you don't feel my comments are relevant at the current >> stage of this series (v11). Alice asked for reviews of the series. These= are my >> comments. Feel free do whatever you want with them. > > I think you're getting the wrong end of the stick - you are making commen= ts > on something relevant to mm, as an mm maintainer I'm giving you my point = of > view. I appreciate that. > > Your comments elsewhere seem highly useful, and review is always > appreciated, if you read what I said above - I defer entirely to the rust > community on things of which you are expert - so there is clearly no > disrespect intended. I did not read any disrespect in your message. I understand if you think I am late at the party at v11. Normally I would not pick up review of a series that late. > > I'd also ask you to respect that I have gone to great lengths to review > this series from mm side, motivated by a strong desire to help the rust > commnuity. I absolutely appreciate that! > > So where I am coming from is nothing negative, quite the opposite, I simp= ly > feel _on this issue_ it is not worth holding up the series for. > > This is no way intended to do down, disrespect or seem ungrateful for your > review or efforts. Apologies if it seemed that way, was not the intent. > > And to reiterate what I said above - I want to see this series merge :) so > there is no ill will anywhere. We can always merge this as is and then discuss the finer points of documentation later - I am fine with that. But obviously I cannot put my review tag on it, when I don't understand the semantics of the functions from reading the documentation strings. Perhaps we have someone who is more well versed in mm that can. > >> >> >> Best regards, >> Andreas Hindborg >> > > Perhaps the correct approach here, as alluded above, is for Alice to add = an > extra commentary pointing out the role of page tables here? That would be nice. Perhaps a bit of module level documentation is also a good addition. > > To complicate matters further (of course) there are recent series which > actually _do_ unused clean up page tables, though not (I believe... I have > to check...) on zap. But of course we in mm JUST LOVE to complicate > everything... ;) We should make sure to document that :) Best regards, Andreas Hindborg