(in)consistency of page/folio function naming

(in)consistency of page/folio function naming

[Matthew Wilcox]

I'm going through my patch queue implementing peterz's request to rename
FolioUptodate() as folio_uptodate().  It's going pretty well, but it
throws into relief all the places where we're not consistent naming
existing functions which operate on pages as page_foo().  The folio
conversion is a great opportunity to sort that out.  Mostly so far, I've
just done s/page/folio/ on function names, but there's the opportunity to
regularise a lot of them, eg:

	put_page		folio_put
	lock_page		folio_lock
	lock_page_or_retry	folio_lock_or_retry
	rotate_reclaimable_page	folio_rotate_reclaimable
	end_page_writeback	folio_end_writeback
	clear_page_dirty_for_io	folio_clear_dirty_for_io

Some of these make a lot of sense -- eg when ClearPageDirty has turned
into folio_clear_dirty(), having folio_clear_dirty_for_io() looks regular.
I'm not entirely convinced about folio_lock(), but folio_lock_or_retry()
makes more sense than lock_page_or_retry().  Ditto _killable() or
_async().

Thoughts?

Re: (in)consistency of page/folio function naming

[David Hildenbrand]

On 22.04.21 05:20, Matthew Wilcox wrote:
> 
> I'm going through my patch queue implementing peterz's request to rename
> FolioUptodate() as folio_uptodate().  It's going pretty well, but it
> throws into relief all the places where we're not consistent naming
> existing functions which operate on pages as page_foo().  The folio
> conversion is a great opportunity to sort that out.  Mostly so far, I've
> just done s/page/folio/ on function names, but there's the opportunity to
> regularise a lot of them, eg:
> 
> 	put_page		folio_put
> 	lock_page		folio_lock
> 	lock_page_or_retry	folio_lock_or_retry
> 	rotate_reclaimable_page	folio_rotate_reclaimable
> 	end_page_writeback	folio_end_writeback
> 	clear_page_dirty_for_io	folio_clear_dirty_for_io
> 
> Some of these make a lot of sense -- eg when ClearPageDirty has turned
> into folio_clear_dirty(), having folio_clear_dirty_for_io() looks regular.
> I'm not entirely convinced about folio_lock(), but folio_lock_or_retry()
> makes more sense than lock_page_or_retry().  Ditto _killable() or
> _async().
> 
> Thoughts?

I tend to like prefixes: they directly set the topic.

The only thing I'm concerned is that we end up with

put_page vs. folio_put

which is suboptimal.

-- 
Thanks,

David / dhildenb

Re: (in)consistency of page/folio function naming

[Jason Gunthorpe]

On Thu, Apr 22, 2021 at 11:09:45AM +0200, David Hildenbrand wrote:
> On 22.04.21 05:20, Matthew Wilcox wrote:
> > 
> > I'm going through my patch queue implementing peterz's request to rename
> > FolioUptodate() as folio_uptodate().  It's going pretty well, but it
> > throws into relief all the places where we're not consistent naming
> > existing functions which operate on pages as page_foo().  The folio
> > conversion is a great opportunity to sort that out.  Mostly so far, I've
> > just done s/page/folio/ on function names, but there's the opportunity to
> > regularise a lot of them, eg:
> > 
> > 	put_page		folio_put
> > 	lock_page		folio_lock
> > 	lock_page_or_retry	folio_lock_or_retry
> > 	rotate_reclaimable_page	folio_rotate_reclaimable
> > 	end_page_writeback	folio_end_writeback
> > 	clear_page_dirty_for_io	folio_clear_dirty_for_io
> > 
> > Some of these make a lot of sense -- eg when ClearPageDirty has turned
> > into folio_clear_dirty(), having folio_clear_dirty_for_io() looks regular.
> > I'm not entirely convinced about folio_lock(), but folio_lock_or_retry()
> > makes more sense than lock_page_or_retry().  Ditto _killable() or
> > _async().
> > 
> > Thoughts?
> 
> I tend to like prefixes: they directly set the topic.
> 
> The only thing I'm concerned is that we end up with
> 
> put_page vs. folio_put
> 
> which is suboptimal.

We have this issue across the kernel already, eg kref_put() vs its
wrapper put_device()

Personally I tend to think the regularity of 'thing'_'action' is
easier to remember than to try to guess/remember that someone judged
'action'_'thing' to be more englishy.

Jason

Re: (in)consistency of page/folio function naming

[Matthew Wilcox]

On Thu, Apr 22, 2021 at 09:21:17AM -0300, Jason Gunthorpe wrote:
> On Thu, Apr 22, 2021 at 11:09:45AM +0200, David Hildenbrand wrote:
> > On 22.04.21 05:20, Matthew Wilcox wrote:
> > > 
> > > I'm going through my patch queue implementing peterz's request to rename
> > > FolioUptodate() as folio_uptodate().  It's going pretty well, but it
> > > throws into relief all the places where we're not consistent naming
> > > existing functions which operate on pages as page_foo().  The folio
> > > conversion is a great opportunity to sort that out.  Mostly so far, I've
> > > just done s/page/folio/ on function names, but there's the opportunity to
> > > regularise a lot of them, eg:
> > > 
> > > 	put_page		folio_put
> > > 	lock_page		folio_lock
> > > 	lock_page_or_retry	folio_lock_or_retry
> > > 	rotate_reclaimable_page	folio_rotate_reclaimable
> > > 	end_page_writeback	folio_end_writeback
> > > 	clear_page_dirty_for_io	folio_clear_dirty_for_io
> > > 
> > > Some of these make a lot of sense -- eg when ClearPageDirty has turned
> > > into folio_clear_dirty(), having folio_clear_dirty_for_io() looks regular.
> > > I'm not entirely convinced about folio_lock(), but folio_lock_or_retry()
> > > makes more sense than lock_page_or_retry().  Ditto _killable() or
> > > _async().
> > > 
> > > Thoughts?
> > 
> > I tend to like prefixes: they directly set the topic.
> > 
> > The only thing I'm concerned is that we end up with
> > 
> > put_page vs. folio_put
> > 
> > which is suboptimal.
> 
> We have this issue across the kernel already, eg kref_put() vs its
> wrapper put_device()
> 
> Personally I tend to think the regularity of 'thing'_'action' is
> easier to remember than to try to guess/remember that someone judged
> 'action'_'thing' to be more englishy.

Mostly agree.  object_verb_attribute is usually better, but i'm not
changing offset_in_folio() to folio_calculate_offset() (unless someone
comes up with a better name)

There are also a few places where "folio" is subordinate.
eg filemap_get_folio(), lruvec_stat_mod_folio()

Re: (in)consistency of page/folio function naming

[Vlastimil Babka]

On 4/22/21 11:09 AM, David Hildenbrand wrote:
> On 22.04.21 05:20, Matthew Wilcox wrote:
>>
>> I'm going through my patch queue implementing peterz's request to rename
>> FolioUptodate() as folio_uptodate().  It's going pretty well, but it
>> throws into relief all the places where we're not consistent naming
>> existing functions which operate on pages as page_foo().  The folio
>> conversion is a great opportunity to sort that out.  Mostly so far, I've
>> just done s/page/folio/ on function names, but there's the opportunity to
>> regularise a lot of them, eg:
>>
>>     put_page        folio_put
>>     lock_page        folio_lock
>>     lock_page_or_retry    folio_lock_or_retry
>>     rotate_reclaimable_page    folio_rotate_reclaimable
>>     end_page_writeback    folio_end_writeback
>>     clear_page_dirty_for_io    folio_clear_dirty_for_io
>>
>> Some of these make a lot of sense -- eg when ClearPageDirty has turned
>> into folio_clear_dirty(), having folio_clear_dirty_for_io() looks regular.
>> I'm not entirely convinced about folio_lock(), but folio_lock_or_retry()
>> makes more sense than lock_page_or_retry().  Ditto _killable() or
>> _async().
>>
>> Thoughts?
> 
> I tend to like prefixes: they directly set the topic.

Agreed.

> The only thing I'm concerned is that we end up with
> 
> put_page vs. folio_put
> 
> which is suboptimal.

If we start differing from page's CamelCase, then we can start differing in this
as well. Should be fine if the long-term goal is to convert as much from pages
to folios as possible. Perhaps the remaining page usages could then be converted
too for consistency as there won't be many left?