Fixing doc warnings for struct folio

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxx> -----

Date: Tue, 29 Mar 2022 00:04:01 +0100
From: Matthew Wilcox <willy@xxxxxxxxxxxxx>
To: Hugh Dickins <hughd@xxxxxxxxxx>
Cc: Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx>, Andrew Morton
	<akpm@xxxxxxxxxxxxxxxxxxxx>, Linux Kernel Mailing List
	<linux-kernel@xxxxxxxxxxxxxxx>, Linux Next Mailing List
	<linux-next@xxxxxxxxxxxxxxx>
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 -----



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux