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