Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1554194

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

Show all headers | View raw

On 01/06/2017 11:20 PM, David Rientjes wrote:
> 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.  

Why not just "madvise+defer"?

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

Sure, my prediction was only about "users setting this" :) Agreed that
those will be a small minority of all users.

[...]

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

OK, well I'm reluctant to break existing userspace knowingly over such
silliness. Also apparently sysfs files in general should accept only one
value, so I'm not going to push my approach.

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

Suggestion is above. I however think your cleanup isn't really needed,
we can simply keep the existing 3 internal flags, and "madvise+defer"
would enable two of them, like in my patch. Nothing says that internally
each option should correspond to exactly one flag.

Vlastimil

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[patch] mm, thp: add new background defrag option David Rientjes <rientjes@google.com> - 2017-01-05 00:50 +0100
  Re: [patch] mm, thp: add new background defrag option Mel Gorman <mgorman@techsingularity.net> - 2017-01-05 11:20 +0100
    Re: [patch] mm, thp: add new background defrag option Michal Hocko <mhocko@kernel.org> - 2017-01-05 11:40 +0100
    Re: [patch] mm, thp: add new background defrag option Vlastimil Babka <vbabka@suse.cz> - 2017-01-05 15:00 +0100
      Re: [patch] mm, thp: add new background defrag option Michal Hocko <mhocko@kernel.org> - 2017-01-05 17:00 +0100
      Re: [patch] mm, thp: add new background defrag option David Rientjes <rientjes@google.com> - 2017-01-06 00:00 +0100
        Re: [patch] mm, thp: add new background defrag option Vlastimil Babka <vbabka@suse.cz> - 2017-01-06 10:00 +0100
          Re: [patch] mm, thp: add new background defrag option Michal Hocko <mhocko@kernel.org> - 2017-01-06 15:10 +0100
          Re: [patch] mm, thp: add new background defrag option David Rientjes <rientjes@google.com> - 2017-01-06 23:30 +0100
            Re: [patch] mm, thp: add new background defrag option Vlastimil Babka <vbabka@suse.cz> - 2017-01-09 11:10 +0100
              Re: [patch] mm, thp: add new background defrag option Vlastimil Babka <vbabka@suse.cz> - 2017-01-09 13:10 +0100
              Re: [patch] mm, thp: add new background defrag option David Rientjes <rientjes@google.com> - 2017-01-10 03:30 +0100
                Re: [patch] mm, thp: add new background defrag option Hugh Dickins <hughd@google.com> - 2017-01-10 04:40 +0100
                Re: [patch] mm, thp: add new background defrag option Vlastimil Babka <vbabka@suse.cz> - 2017-01-10 09:50 +0100
                Re: [patch] mm, thp: add new background defrag option David Rientjes <rientjes@google.com> - 2017-01-11 01:00 +0100
                Re: [patch] mm, thp: add new background defrag option Michal Hocko <mhocko@kernel.org> - 2017-01-10 14:10 +0100
                [patch v2] mm, thp: add new defer+madvise defrag option David Rientjes <rientjes@google.com> - 2017-01-11 01:20 +0100
                Re: [patch v2] mm, thp: add new defer+madvise defrag option Vlastimil Babka <vbabka@suse.cz> - 2017-01-11 08:40 +0100
                Re: [patch v2] mm, thp: add new defer+madvise defrag option Michal Hocko <mhocko@kernel.org> - 2017-01-12 09:10 +0100
                Re: [patch v2] mm, thp: add new defer+madvise defrag option Mel Gorman <mgorman@techsingularity.net> - 2017-01-11 10:00 +0100
                Re: [patch v2] mm, thp: add new defer+madvise defrag option Andrew Morton <akpm@linux-foundation.org> - 2017-01-12 01:20 +0100

csiph-web