Message-ID: <ZPiU7/Rg8g+B58Wa@casper.infradead.org>
Date:   Wed, 6 Sep 2023 16:04:15 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Jonathan Corbet <corbet@....net>
Cc:     Mike Rapoport <rppt@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org
Subject: Re: [PATCH] docs/mm: Physical Memory: add "Memory map" section

On Wed, Sep 06, 2023 at 08:41:28AM -0600, Jonathan Corbet wrote:
> > +  All flags are declared in ``include/linux/page-flags.h``. There are a
> > +  number of macros defined for testing, clearing and setting the flags. Page
> > +  flags should not be accessed directly, but only using these macros.
> 
> It would sure be nice if we had documentation for what all the flags
> mean :)

When I figure them out, I'll let you know!

> > +``_mapcount``
> > +  If the page can be mapped to userspace, encodes the number of times this
> > +  page is referenced by a page table.
> > +  Do not use directly, call page_mapcount().
> 
> Have we figured out what mapcount really means yet? :)

Hah!  I know what this field means today!  In two hours time, I might
be less sure!  (Does LWN want to come along to that MM meeting and write
it up for an article?)

> > +``virtual``
> > +  Virtual address in the kernel direct map. Will be ``NULL`` for highmem
> > +  pages. Only defined for some architectures.
> 
> I'd say virtual is absent more often than present anymore, right?
> Perhaps it's worth being more explicit about that.  And maybe say to use
> page_address() rather than accessing it directly?

That's something I've been thinking about for the folio kernel-doc.
Just stop documenting the things that you "shouldn't use".
Non-kernel-doc comments in the source about what you should use instead,
but no kernel-doc comments to say "Use page_address() instead of this".

> > +For pages on unevictable "LRU list" ``lru`` is overlayed with an anonymous
> > +struct containing two fields:
> > +
> > +``__filler``
> > +  A dummy field that must be always even to avoid conflict with compound
> > +  page encoding.
> 
> Do we care about the constraints on this field's contents?  Presumably
> that is taken care of somewhere and nobody should mess with it?

I also think that documenting here which things are in a union with
other things is unnecessary.  If someone cares for such a level of
detail, they'd better be reading the source code instead of this.
Nobody should be using it, better to just leave it undocumented.

> > +``mapping``
> > +  The file this page belongs to. Can be pagecache or swapcahe. For

Oh, actually, no, it can't be swapcache.  If the page is in the
swapcache, you find its swapcache through swapcache_mapping().
That's because ->mapping is reused as an anon_vma pointer for anon
pages.

> > +  anonymous memory refers to the `struct anon_vma`.
> > +  See also ``include/linux/page-flags.h`` for ``PAGE_MAPPING_FLAGS``
> 
> It seems like putting in the types for fields like this would be useful;
> readers of the HTML docs can then follow the links and see what is
> actually pointed to.
> 
> > +``index``
> > +  Page offset within mapping. Overlaps with ``share``.
> > +
> > +``share``
> > +  Share count for fsdax. Overlaps with ``index``.

fsdax is not pagecache, so this probably shouldn't be documented here.

> I wonder if it might be better to structure it as if the splitting of
> struct page were already complete, with a section for each page
> descriptor type, even the ones that don't exist as separate entities
> yet?  Maybe that would make it easier for people to keep it current as
> they hack pieces out of struct page?

Yes.  Although I don't think we quite know what it's all going to
look like yet, which makes it challenging to document!

Powered by blists - more mailing lists