All of lore.kernel.org
 help / color / mirror / Atom feed
* Fixing doc warnings for struct folio
@ 2022-03-29  0:31 Matthew Wilcox
  0 siblings, 0 replies; only message in thread
From: Matthew Wilcox @ 2022-03-29  0:31 UTC (permalink / raw)
  To: linux-mm; +Cc: linux-doc

I realised this didn't have a cc to linux-mm.

For that matter, linux-doc might have some opinions.

----- Forwarded message from Matthew Wilcox <willy@infradead.org> -----

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

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).

----- End forwarded message -----

^ permalink raw reply	[flat|nested] only message in thread

only message in thread, other threads:[~2022-03-29  0:31 UTC | newest]

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-29  0:31 Fixing doc warnings for struct folio Matthew Wilcox

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.