linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Vlastimil Babka <vbabka@suse.cz>
To: David Rientjes <rientjes@google.com>
Cc: Mel Gorman <mgorman@techsingularity.net>,
	Andrew Morton <akpm@linux-foundation.org>,
	Michal Hocko <mhocko@kernel.org>,
	Jonathan Corbet <corbet@lwn.net>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [patch] mm, thp: add new background defrag option
Date: Fri, 6 Jan 2017 09:41:57 +0100	[thread overview]
Message-ID: <558ce85c-4cb4-8e56-6041-fc4bce2ee27f@suse.cz> (raw)
In-Reply-To: <alpine.DEB.2.10.1701051446140.19790@chino.kir.corp.google.com>

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

  reply	other threads:[~2017-01-06  8:42 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-01-04 23:41 David Rientjes
2017-01-05 10:13 ` Mel Gorman
2017-01-05 10:33   ` Michal Hocko
2017-01-05 13:58   ` Vlastimil Babka
2017-01-05 15:50     ` Michal Hocko
2017-01-05 22:54     ` David Rientjes
2017-01-06  8:41       ` Vlastimil Babka [this message]
2017-01-06 14:01         ` Michal Hocko
2017-01-06 22:20         ` David Rientjes
2017-01-09 10:04           ` Vlastimil Babka
2017-01-09 12:06             ` Vlastimil Babka
2017-01-10  2:19             ` David Rientjes
2017-01-10  3:38               ` Hugh Dickins
2017-01-10  8:44                 ` Vlastimil Babka
2017-01-10 23:52                   ` David Rientjes
2017-01-10 13:01               ` Michal Hocko
2017-01-11  0:15               ` [patch v2] mm, thp: add new defer+madvise " David Rientjes
2017-01-11  7:35                 ` Vlastimil Babka
2017-01-12  8:01                   ` Michal Hocko
2017-01-11  8:56                 ` Mel Gorman
2017-01-12  0:16                 ` Andrew Morton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=558ce85c-4cb4-8e56-6041-fc4bce2ee27f@suse.cz \
    --to=vbabka@suse.cz \
    --cc=akpm@linux-foundation.org \
    --cc=corbet@lwn.net \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mgorman@techsingularity.net \
    --cc=mhocko@kernel.org \
    --cc=rientjes@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox