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: <20170106140107.GN5556@dhcp22.suse.cz>
Date:   Fri, 6 Jan 2017 15:01:07 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     Vlastimil Babka <vbabka@...e.cz>
Cc:     David Rientjes <rientjes@...gle.com>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Andrew Morton <akpm@...ux-foundation.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 06-01-17 09:41:57, Vlastimil Babka wrote:
> 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.

fully agreed. Calling a mode which does _direct_ compaction as
_background_ is not only confusng but just plain wrong.

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

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

To me the combined option sounds better than a new one with confusing
name. Maybe we can come up with a better name that would reflect the
functionality better, though. There is some minor risk that some
userspace doesn't cope with two options being selected but there has
never been any guarantee about a single option being selected in the
first place. Is anybody actually parsing this file to make further
decisions? I would expect it is write mostly thing.
-- 
Michal Hocko
SUSE Labs