Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Tue, 29 Mar 2022 00:04:01 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Hugh Dickins <hughd@google.com>
Cc:     Stephen Rothwell <sfr@canb.auug.org.au>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux Next Mailing List <linux-next@vger.kernel.org>
Subject: Re: linux-next: build warnings after merge of the akpm-current tree
Message-ID: <YkI+4SzcoaqZIQIG@casper.infradead.org>
References: <20220209170245.08968c92@canb.auug.org.au>
 <d2701072-99e6-762b-bc80-64bda193c792@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d2701072-99e6-762b-bc80-64bda193c792@google.com>
Precedence: bulk

On Wed, Feb 09, 2022 at 08:03:26AM -0800, Hugh Dickins wrote:
> On Wed, 9 Feb 2022, Stephen Rothwell wrote:
> > include/linux/mm_types.h:272: warning: Function parameter or member '__filler' not described in 'folio'
> > include/linux/mm_types.h:272: warning: Function parameter or member 'mlock_count' not described in 'folio'
> 
> Thank you for including the patches and reporting this, Stephen.
> Is this a warning you can live with for a week or two?
> 
> I've never tried generating htmldocs (I'm tempted just to replace a few
> "/**"s by "/*"s!), and I'm fairly sure Matthew will have strong feelings
> about how this new union (or not) will be better foliated - me messing
> around with doc output here is unlikely to be helpful at this moment.

I have a substantial question, and then some formatting / appearance
questions.

The first is, what does mlock_count represent for a multi-page folio
that is partially mlocked?  If you allocate an order-9 page then mmap()
13 pages of it and mlockall(), does mlock_count increase by 1, 13 or 512?

Then we have a tradeoff between prettiness of the source code and
prettiness of the htmldocs.  At one maximum, we can make it look like
this in the htmldocs:

struct folio {
  unsigned long flags;
  struct list_head lru;
  unsigned int mlock_count;
  struct address_space *mapping;
  pgoff_t index;
  void *private;
  atomic_t _mapcount;
  atomic_t _refcount;
#ifdef CONFIG_MEMCG;
  unsigned long memcg_data;
#endif;
};

but at the cost of making the source code look like this:

struct folio {
        /* private: don't document the anon union */
        union {
                struct {
        /* public: */
                        unsigned long flags;
        /* private: */
                        union {
        /* public: */
                                struct list_head lru;
        /* private: */
                                struct {
                                        void *__filler;
        /* public: */
                                        unsigned int mlock_count;
        /* private: */
                                };
                        };
        /* public: */
                        struct address_space *mapping;

At the other extreme, the htmldocs can look more complicated:

struct folio {
  unsigned long flags;
  union {
    struct list_head lru;
    struct {
      unsigned int mlock_count;
    };
  };
  struct address_space *mapping;
  pgoff_t index;
  void *private;
  atomic_t _mapcount;
  atomic_t _refcount;
#ifdef CONFIG_MEMCG;
  unsigned long memcg_data;
#endif;
};

with the source code changes being merely:

@@ -227,6 +227,7 @@ struct page {
  * struct folio - Represents a contiguous set of bytes.
  * @flags: Identical to the page flags.
  * @lru: Least Recently Used list; tracks how recently this folio was used.
+ * @mlock_count: Number of times any page in this folio is mlocked.
  * @mapping: The file this page belongs to, or refers to the anon_vma for
  *    anonymous memory.
  * @index: Offset within the file, in units of pages.  For anonymous memory,
@@ -256,7 +257,9 @@ struct folio {
                        union {
                                struct list_head lru;
                                struct {
+       /* private: */
                                        void *__filler;
+       /* public: */
                                        unsigned int mlock_count;
                                };
                        };

Various steps in between are possible, such as removing the anonymous
struct from the documentation, but leaving the union.  We could also
choose to document __filler, but that seems like a poor choice to me.

Anyway, that's all arguable and not really too important.  My mild
preference is for the private/public markers around __filler alone,
but I'll happily abide by the preferences of others.

The important part is what mlock_count really means.  We can be
reasonably verbose here (two or three lines of text, I'd suggest).