Re: [PATCH v3 12/14] mm: Improve struct page documentation

[Matthew Wilcox <willy@infradead.org>]

On Wed, Apr 18, 2018 at 04:32:27PM -0700, Randy Dunlap wrote:
> > + * If you allocate the page using alloc_pages(), you can use some of the
> > + * space in struct page for your own purposes.  The five words in the first
> 
> Using "first union" here...
> 
> > + * If your page will not be mapped to userspace, you can also use the 4
> > + * bytes in the second union, but you must call page_mapcount_reset()
> 
> and "second union" here bother me, but it looks like they are anonymous.
> 
> I'm concerned about someone other than you modifying struct page at some
> later time.  If these unions were named (and you could use that name here
> instead of "first" or "second"), then there would be less chance for that
> next person to miss modifying that comment or it just becoming stale.

Yeah, it bothers me too.  I was first bothered by this when writing the
patch descriptions for the earlier patches in the series "Combine first
three unions in struct page" and "Combine first two unions in struct
page" really suck as patch descriptions.  But I couldn't come up with
anything better, so here we are ...

If we name the union, then either we have to put in some grotty macros
or change every instance of page->mapping to page->u1.mapping (repeat
ad nauseam).  I mean, I'd rather leave the unions anonymous and name
the structs (but again, I don't want to rename every user).

We can put a comment on the union and name them that way, but I
don't even know what to call them.  "main union" "auxiliary union".
"secondary union".  I don't know.

> Reviewed-by: Randy Dunlap <rdunlap@infradead.org>

Thanks.  I also did some kernel-doc'ifying of some other comments earlier
in the series.  I'm sure they could be improved.  And there's a whole
bunch of comments which aren't in kernel-doc format and might or might
not want to be.

(eg: do we want to comment _refcount?  Other than to tell people to not
use it?)