Re: [PATCH v8.1 00/31] Memory Folios

From: John Hubbard <jhubbard@nvidia.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: Nicholas Piggin <npiggin@gmail.com>,
	Hugh Dickins <hughd@google.com>, <akpm@linux-foundation.org>,
	<linux-fsdevel@vger.kernel.org>, <linux-mm@kvack.org>,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH v8.1 00/31] Memory Folios
Date: Sat, 1 May 2021 17:42:21 -0700	[thread overview]
Message-ID: <f8c968d9-e66a-3588-3811-3efe704650ae@nvidia.com> (raw)
In-Reply-To: <20210502001705.GW1847222@casper.infradead.org>

On 5/1/21 5:17 PM, Matthew Wilcox wrote:
...
>> In addition to pointing out that the name was a page flag, the weird
>> camel case also meant, "if you try to search for this symbol, you will
>> be defeated", because the darn thing is constructed via macro
>> concatenation.
> 
> I've always hated that, FWIW.  And you can't add kernel-doc for them
> because kernel-doc doesn't understand cpp.  So my current plan (quoting
> my other email):
> 
> folio_dirty() -- defined in page-flags.h
> would have kernel-doc, would be greppable
> 
> folio_test_set_dirty_flag()
> folio_test_clear_dirty_flag()
> __folio_clear_dirty_flag()
> __folio_set_dirty_flag()
> folio_clear_dirty_flag()
> folio_set_dirty_flag() -- generated in filemap.h under #ifndef MODULE
> would not have kernel-doc, would not be greppable, would only be used
> in core vfs and core mm.
> 
> folio_mark_dirty() -- declared in mm.h (this is rare; turns out all kinds of
> 			crap wants to mark pages as being dirty)
> folio_clear_dirty_for_io() -- declared in filemap.h
> already have kernel-doc, are greppable, used by filesystems and sometimes
> other random code.

Yes, the page dirty stuff is definitely not simple, so it's very good to
move away from the auto-generated names there. Looks like you are down to
just a couple of generated names now, if I'm reading this correctly.

> 
>> Except that over time, it turned out to be not quite that simple, and
>> people started adding functionality. So now it's "cannot find it, and
>> it's also got little goodies hiding in there--maybe!".
> 
> I also don't like that.  With what I'm thinking, there are no special
> cases hidden in the autogenerated names.  Special things like the current

Yes, that's a good guideline: no special cases in the auto-generated function
names. Because, either it is a simple, standard auto-generated thing, or
it is something that one needs to read, in order to see exactly what it does.

> SetPageUptodate would be in folio_mark_uptodate() and filesystems couldn't
> even call folio_set_uptodate().
> 
>> Given all that, I'd argue for either:
>>      b) changing a bunch of the items to actual written-out names. What's
>>         the harm? We'd end up with a longer file, but one could grep or
>>         cscope for the names.
> 
> I hope the above makes you happy -- everything a filesystem author needs
> gets kernel-doc.  People working inside the VM/VFS still get exposed

If "kernel-doc" is effectively a proxy for "file names are directly visible
in the source code, then I'm a lot happier than I was, yes. :)

> to undocumented folio_test_set_foo_flag(), but it's all regular and
> autogenerated.
> 

Sounds pretty good.

thanks,
-- 
John Hubbard
NVIDIA