Re: [PATCH 1/1] mm, compaction: correct the bounds of __fragmentation_index()

Michal Hocko
2018-02-23T09:10:20Z

On Mon 19-02-18 14:30:36, Robert Harris wrote:
>
>
> > On 19 Feb 2018, at 12:39, Michal Hocko <mhocko@kernel.org> wrote:
> >
> > On Mon 19-02-18 12:14:26, Robert Harris wrote:
> >>
> >>
> >>> On 19 Feb 2018, at 08:26, Michal Hocko <mhocko@kernel.org> wrote:
> >>>
> >>> On Sun 18-02-18 16:47:55, robert.m.harris@oracle.com wrote:
> >>>> From: "Robert M. Harris" <robert.m.harris@oracle.com>
> >>>>
> >>>> __fragmentation_index() calculates a value used to determine whether
> >>>> compaction should be favoured over page reclaim in the event of allocation
> >>>> failure. The calculation itself is opaque and, on inspection, does not
> >>>> match its existing description. The function purports to return a value
> >>>> between 0 and 1000, representing units of 1/1000. Barring the case of a
> >>>> pathological shortfall of memory, the lower bound is instead 500. This is
> >>>> significant because it is the default value of sysctl_extfrag_threshold,
> >>>> i.e. the value below which compaction should be avoided in favour of page
> >>>> reclaim for costly pages.
> >>>>
> >>>> This patch implements and documents a modified version of the original
> >>>> expression that returns a value in the range 0 <= index < 1000. It amends
> >>>> the default value of sysctl_extfrag_threshold to preserve the existing
> >>>> behaviour.
> >>>
> >>> It is not really clear to me what is the actual problem you are trying
> >>> to solve by this patch. Is there any bug or are you just trying to
> >>> improve the current implementation to be more effective?
> >>
> >> There is not a significant bug.
> >>
> >> The first problem is that the mathematical expression in
> >> __fragmentation_index() is opaque, particularly given the lack of
> >> description in the comments or the original commit message. This patch
> >> provides such a description.
> >>
> >> Simply annotating the expression did not make sense since the formula
> >> doesn't work as advertised. The fragmentation index is described as
> >> being in the range 0 to 1000 but the bounds of the formula are instead
> >> 500 to 1000. This patch changes the formula so that its lower bound is
> >> 0.
> >
> > But why do we want to fix that in the first place? Why don't we simply
> > deprecate the tunable and remove it altogether? Who is relying on tuning
> > this option. Considering how it doesn't work as advertised and nobody
> > complaining I have that feeling that it is not really used in wild…
>
> I think it's a useful feature. Ignoring any contrived test case, there
> will always be a lower limit on the degree of fragmentation that can be
> achieved by compaction. If someone takes the trouble to measure this
> then it is entirely reasonable that he or she should be able to inhibit
> compaction for cases when fragmentation falls below some correspondingly
> sized threshold.

Do you have any practical examples?
--
Michal Hocko
SUSE Labs