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

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

Message-ID: <alpine.DEB.2.10.1701061407300.138109@chino.kir.corp.google.com>
Date:   Fri, 6 Jan 2017 14:20:14 -0800 (PST)
From:   David Rientjes <rientjes@...gle.com>
To:     Vlastimil Babka <vbabka@...e.cz>
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 Fri, 6 Jan 2017, Vlastimil Babka wrote:

> Deciding between "defer" and "background" is however confusing, and also
> doesn't indicate that the difference is related to madvise.
> 

Any suggestions for a better name for "background" are more than welcome.  

> > 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.
> 

I think the far majority will go with a third option: simply use the 
kernel default and be unaware of other settings or consider it to be the 
most likely choice solely because it is the kernel default.

I think the kernel default could easily be changed to "background" after 
this and nobody would actually notice, but I don't have a strong 
preference for that.  I think users who notice large thp_fault_fallback 
and want to get the true "transparent" nature of hugepages will 
investigate defragmentation behavior and see "background" is exactly what 
they want.  Indeed, I think that the new "background" mode meshes well 
with the expectation of "transparent" hugepages.  I don't foresee any 
usecase, present or future, for "defer" so I'll simply ignore it.

So whether it's better to do echo background or echo "madvise defer" is 
not important to me, I simply imagine that the combination will be more 
difficult to describe to users.  It would break our userspace to currently 
tests for "[madvise]" and reports that state as strictly madvise to our 
mission control, but I can work around that; not sure if others would 
encounter the same issue (would "[defer madvise]" or "[defer] [madvise]" 
break fewer userspaces?).

I'd leave it to Andrew to decide whether sysfs files should accept 
multiple modes or not.  If you are to propose a patch to do so, I'd 
encourage you to do the same cleanup of triple_flag_store() that I did and 
make the gfp mask construction more straight-forward.  If you'd like to 
suggest a different name for "background", I'd be happy to change that if 
it's more descriptive.