linux-kernel - Re: [PATCH 1/5] mm: Introduce mm_struct.has

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 22 Sep 2020 12:11:03 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Peter Xu <peterx@...hat.com>
CC:     <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jan Kara <jack@...e.cz>, Michal Hocko <mhocko@...e.com>,
        Kirill Tkhai <ktkhai@...tuozzo.com>,
        Kirill Shutemov <kirill@...temov.name>,
        Hugh Dickins <hughd@...gle.com>,
        Christoph Hellwig <hch@....de>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Oleg Nesterov <oleg@...hat.com>,
        Leon Romanovsky <leonro@...dia.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        "Jann Horn" <jannh@...gle.com>
Subject: Re: [PATCH 1/5] mm: Introduce mm_struct.has_pinned

On 9/22/20 8:17 AM, Peter Xu wrote:
> On Mon, Sep 21, 2020 at 04:53:38PM -0700, John Hubbard wrote:
>> On 9/21/20 2:17 PM, Peter Xu wrote:
...
>>> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
>>> index 496c3ff97cce..6f291f8b74c6 100644
>>> --- a/include/linux/mm_types.h
>>> +++ b/include/linux/mm_types.h
>>> @@ -441,6 +441,16 @@ struct mm_struct {
>>>    #endif
>>>    		int map_count;			/* number of VMAs */
>>> +		/**
>>> +		 * @has_pinned: Whether this mm has pinned any pages.  This can
>>> +		 * be either replaced in the future by @pinned_vm when it
>>> +		 * becomes stable, or grow into a counter on its own. We're
>>> +		 * aggresive on this bit now - even if the pinned pages were
>>> +		 * unpinned later on, we'll still keep this bit set for the
>>> +		 * lifecycle of this mm just for simplicity.
>>> +		 */
>>> +		int has_pinned;
>>
>> I think this would be elegant as an atomic_t, and using atomic_set() and
>> atomic_read(), which seem even more self-documenting that what you have here.
>>
>> But it's admittedly a cosmetic point, combined with my perennial fear that
>> I'm missing something when I look at a READ_ONCE()/WRITE_ONCE() pair. :)
> 
> Yeah but I hope I'm using it right.. :) I used READ_ONCE/WRITE_ONCE explicitly
> because I think they're cheaper than atomic operations, (which will, iiuc, lock
> the bus).
> 

The "cheaper" argument vanishes if the longer-term plan is to use atomic ops.
Given the intent of this patchset, simpler is better, and "simpler that has the
same performance as the longer term solution" is definitely OK.

>>
>> It's completely OK to just ignore this comment, but I didn't want to completely
>> miss the opportunity to make it a tiny bit cleaner to the reader.
> 
> This can always become an atomic in the future, or am I wrong?  Actually if
> we're going to the counter way I feel like it's a must.
> 

Yes it can change. But you can get the simplicity and clarity now, rather
than waiting.

thanks,
-- 
John Hubbard
NVIDIA