linux-kernel - Re: [GIT PULL] Rename page_offset() to page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200411214818.GH21484@bombadil.infradead.org>
Date:   Sat, 11 Apr 2020 14:48:18 -0700
From:   Matthew Wilcox <willy@...radead.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] Rename page_offset() to page_pos()

On Sat, Apr 11, 2020 at 01:57:56PM -0700, Linus Torvalds wrote:
> So honestly, i the confusion is that we have "pgoff_t", which is the
> offset of the page counted in _pages_, then my reaction is that
> 
>  (a) I think the truly confusing name is "pgoff_t" (and any
> "page_offset" variable of that type). Calling that "pgindex_t" and
> "page_index" would be a real clarification.

I think you're right.  I have a patch series queued for 5.8 which
renames a lot of 'pgoff_t offset' to 'pgoff_t index'.  I wouldn't mind
at all renaming pgoff_t to pgindex_t.  If you're amenable, pgidx_t would
be shorter.

>  (b) if we really do want to rename page_offset() because of confusion
> with the page index "offset", then the logical thing would be to
> clarify that it's a byte offset, not the page index.

I wasn't entirely forthcoming ... I actually want to introduce a new

#define page_offset(page, x) ((unsigned long)(x) & (page_size(page) - 1))

to simplify handling huge pages.  So I always want to see offset be a
byte count.  offset_in_page() is already taken, and I have no idea what
else to call the function to get the offset of this address within a
particular page.

> If we'd want a _descriptive_ name, then "byte_offset_of_page()" would
> probably be that. That's hard to mis-understand.
> 
> Yes that's also more of a mouthful, and it still has the "two
> different names for the same thing" issue wrt
> stable/old/rebased/whatever patches.

That was one of the options we discussed, along with file_offset_of_page().

> Which is why I'd much rather change "pgoff_t" to "pgindex_t" and
> related "page_offset" variables to "page_index" variables.

There's only about 20 of those out of the 938 pgoff_t users.  But there's
over a hundred called 'pgoff'.  I need to get smarter about using
Coccinelle; I'm sure it can do this.