linux-kernel - Re: [PATCH v3 3/7] mm: Add write-protect and clean utilities for address space ranges

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]

Date:   Wed, 2 Oct 2019 11:06:43 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Thomas Hellström (VMware) 
        <thomas_os@...pmail.org>
Cc:     Linux-MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Thomas Hellstrom <thellstrom@...are.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Matthew Wilcox <willy@...radead.org>,
        Will Deacon <will.deacon@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        Rik van Riel <riel@...riel.com>,
        Minchan Kim <minchan@...nel.org>,
        Michal Hocko <mhocko@...e.com>,
        Huang Ying <ying.huang@...el.com>,
        Jérôme Glisse <jglisse@...hat.com>,
        "Kirill A . Shutemov" <kirill@...temov.name>
Subject: Re: [PATCH v3 3/7] mm: Add write-protect and clean utilities for
 address space ranges

On Wed, Oct 2, 2019 at 6:48 AM Thomas Hellström (VMware)
<thomas_os@...pmail.org> wrote:
>
> From: Thomas Hellstrom <thellstrom@...are.com>
>
> Add two utilities to a) write-protect and b) clean all ptes pointing into
> a range of an address space.

This one I still don't exactly love.

I'm not entirely sure what rubs me the wrong way, but part of it is
naming. We don't use the name "as", because it reads as (sic) an
English word.

The name we use for address_space pointers is "mapping", both for
variables and for existing functions.

See for example "pte_same_as_swp()" which uses "as" as the _word_ 'as'.

Contrast that with "unmap_mapping_range()" or
"mapping_set_unevictable()" or "read_mapping_page()" or
"invalidate_mapping_pages()", that all work on address spaces.

So please don't use 'as' as shorthand for that - eithe rin the
function names or the filename.

I'm not sure if that's the _only_ thing that raises my heckles when I
read this patch, but I think it might be. So I'm not saying "ack with
naming change", but I suspect that if the naming was changed, it would
look much better to me.

Yes, it's a bit more typing. But I really think
"clean_mapping_dirty_pages()" is just not only more in line with the
mm naming, I think it's a lot more legible and understandable than
"as_dirty_clean()", which just makes me go "what the heck does that
function do?"

And I really think it needs more than just "as" -> "mapping".
"mapping_dirty_clean()" still makes me go "what?" in a way that
"clean_mapping_dirty_pages()" does not. One name reads as a series or
random words, the other reads as a "this is what the function does".

I know I sometimes get hung up about naming, but I do think naming
matters.  A descriptive name that just reads as what the function does
makes it much easier to read the logic of code, imnsho.

              Linus