Re: [PATCH v11 2/8] mm: rust: add vm_area_struct methods that require read access

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Thu, Jan 09, 2025 at 09:02:11AM +0100, Andreas Hindborg wrote:
> "Alice Ryhl" <aliceryhl@google.com> writes:
>
> > On Mon, Dec 16, 2024 at 3:51 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >>
> >> > +
> >> > +    /// Zap pages in the given page range.
> >> > +    ///
> >> > +    /// This clears page table mappings for the range at the leaf level, leaving all other page
> >> > +    /// tables intact,
> >>
> >> I don't fully understand this docstring. Is it correct that the function
> >> will unmap the address range given by `start` and `size`, _and_ free the
> >> pages used to hold the mappings at the leaf level of the page table?
> >
> > If the vma owns a refcount on those pages, then the refcounts are dropped.
>
> Maybe drop the "at the leaf level leaving all other page tables intact".
> 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 traverse
leaf level page table mappings, clearing the entries, leaving the other
page table entries intact.

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?

For a series at v11 where there is broad agreement with maintainers within
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 range. That
> is, anonymous memory is completely freed, file-backed memory has its
> reference count on page cache folio's dropped, any dirty data will still
> 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.

>
> >
> >> > and freeing any memory referenced by the VMA in this range. That is,
> >> > +    /// anonymous memory is completely freed, file-backed memory has its reference count on page
> >> > +    /// cache folio's dropped, any dirty data will still be written back to disk as usual.
> >> > +    #[inline]
> >> > +    pub fn zap_page_range_single(&self, address: usize, size: usize) {
>
>
> 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 people),
I want to see this go in.