Re: [PATCH] docs/mm: add VMA locks documentation
From: Lorenzo Stoakes
Date: Mon Nov 11 2024 - 05:45:03 EST
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@xxxxxxxxxx/
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]