linux-kernel - Re: [patch] mm, thp: add new background defrag option

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: <558ce85c-4cb4-8e56-6041-fc4bce2ee27f@suse.cz>
Date:   Fri, 6 Jan 2017 09:41:57 +0100
From:   Vlastimil Babka <vbabka@...e.cz>
To:     David Rientjes <rientjes@...gle.com>
Cc:     Mel Gorman <mgorman@...hsingularity.net>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Michal Hocko <mhocko@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [patch] mm, thp: add new background defrag option

On 01/05/2017 11:54 PM, David Rientjes wrote:
> On Thu, 5 Jan 2017, Vlastimil Babka wrote:
> 
>> Hmm that's probably why it's hard to understand, because "madvise
>> request" is just setting a vma flag, and the THP allocation (and defrag)
>> still happens at fault.
>>
>> I'm not a fan of either name, so I've tried to implement my own
>> suggestion. Turns out it was easier than expected, as there's no kernel
>> boot option for "defer", just for "enabled", so that particular worry
>> was unfounded.
>>
>> And personally I think that it's less confusing when one can enable defer
>> and madvise together (and not any other combination), than having to dig
>> up the difference between "defer" and "background".
>>
> 
> I think allowing only two options to be combined amongst four available 
> solo options is going to be confusing and then even more difficult for the 
> user to understand what happens when they are combined.  Thus, I think 

Well, the other options are named "always" and "never", so I wouldn't
think so confusing that they can't be combined with anything else.
Deciding between "defer" and "background" is however confusing, and also
doesn't indicate that the difference is related to madvise.

> these options should only have one settable mode as they have always done.
> 
> The kernel implementation takes less of a priority to userspace 
> simplicitly, imo, and my patch actually cleans up much of the existing 
> code and ends up adding fewer lines that yours.  I consider it an 
> improvement in itself.  I don't see the benefit of allowing combined 
> options.

I don't like bikesheding, but as this is about user-space API, more care
should be taken than for implementation details that can change. Even
though realistically there will be in 99% of cases only two groups of
users setting this
- experts like you who know what they are doing, and confusing names
won't prevent them from making the right choice
- people who will blindly copy/paste from the future cargo-cult websites
(if they ever get updated from the enabled="never" recommendations), who
likely won't stop and think about the other options.

Well, so we'll probably disagree, maybe others can add their opinions.