Re: [PATCH] docs/mm: add VMA locks documentation

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

Wei - a side note, when reviewing huuuuge patches like this _please_ remove
irrelevant lines, it's really hard for me to respond and easy for me to
miss stuff scrolling through >600 lines. Thanks!

Also - you're reviewing an out of date series, the new version has changed
a lot, see [0].

[0]:https://lore.kernel.org/all/20241108135708.48567-1-lorenzo.stoakes@oracle.com/


On Mon, Nov 11, 2024 at 07:07:19AM +0000, Wei Yang wrote:
> On Thu, Nov 07, 2024 at 07:01:37PM +0000, Lorenzo Stoakes wrote:
> >+If you want to **write** VMA metadata fields, then things vary depending on the
> >+field (we explore each VMA field in detail below). For the majority you must:
> >+
> >+* Obtain an mmap write lock at the MM granularity via :c:func:`!mmap_write_lock` (or a
> >+  suitable variant), unlocking it with a matching :c:func:`!mmap_write_unlock` when
> >+  you're done with the VMA, *and*
> >+* Obtain a VMA write lock via :c:func:`!vma_start_write` for each VMA you wish to
> >+  modify, which will be released automatically when :c:func:`!mmap_write_unlock` is
> >+  called.
> >+* If you want to be able to write to **any** field, you must also hide the VMA
> >+  from the reverse mapping by obtaining an **rmap write lock**.
>
> Here I am a little confused.
>
> I guess it wants to say mmap write lock and VMA write lock should be obtained,
> while rmap write lock is optional for write operation. And this is what
> illustrated in below section "VMA fields".
>
> But here says 'write to "any" field'. This seems not what it shows below.
>
> Or maybe I misunderstand this part.

It opens with 'for the majority' as in, for the _majority_ you need the
mmap write, VMA write lock.

To able to adjust _all_ you need the rmap write lock too.

I have to trade off with 'summarising' how you get a write lock vs. the
specifics, I mean I literally give a table immediately after this
explicitly listing what is required.

If I gave just that, it'd be confusing and people wouldn't maybe realise
that _mostly_ mmap write, VMA write is enough.

If I don't mention the rmap thing then people might be confused as to why.

Given I give the correct requirements in the table I don't tnink this is a
problem.

[snip]

> >+.. table:: Virtual layout fields
> >+
> >+   ===================== ======================================== ===========
> >+   Field                 Description                              Write lock
> >+   ===================== ======================================== ===========
> >+   :c:member:`!vm_start` Inclusive start virtual address of range mmap write,
> >+                         VMA describes.                           VMA write,
> >+                                                                  rmap write.
> >+   :c:member:`!vm_end`   Exclusive end virtual address of range   mmap write,
> >+                         VMA describes.                           VMA write,
> >+                                                                  rmap write.
> >+   :c:member:`!vm_pgoff` Describes the page offset into the file, rmap write.
>
>                                                                     ^
> I am afraid this one is a typo?

This is fixed in v2.

[snip]