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

Groups > linux.kernel > #1250985 > unrolled thread

[PATCH] mm: vmpressure: fix scan window after SWAP_CLUSTER_MAX increase

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm: vmpressure: fix scan window after SWAP_CLUSTER_MAX increase Johannes Weiner <hannes@cmpxchg.org> - 2015-10-19 20:20 +0200
    Re: [PATCH] mm: vmpressure: fix scan window after SWAP_CLUSTER_MAX  increase Mel Gorman <mgorman@techsingularity.net> - 2015-10-20 09:50 +0200
      Re: [PATCH] mm: vmpressure: fix scan window after SWAP_CLUSTER_MAX  increase Johannes Weiner <hannes@cmpxchg.org> - 2015-10-20 16:00 +0200
        Re: [PATCH] mm: vmpressure: fix scan window after SWAP_CLUSTER_MAX  increase Mel Gorman <mgorman@techsingularity.net> - 2015-10-21 11:20 +0200
    Re: [PATCH] mm: vmpressure: fix scan window after SWAP_CLUSTER_MAX  increase Johannes Weiner <hannes@cmpxchg.org> - 2015-10-21 21:40 +0200
      Re: [PATCH] mm: vmpressure: fix scan window after SWAP_CLUSTER_MAX  increase Hugh Dickins <hughd@google.com> - 2015-10-21 22:10 +0200

#1250985 — [PATCH] mm: vmpressure: fix scan window after SWAP_CLUSTER_MAX increase

mm-increase-swap_cluster_max-to-batch-tlb-flushes.patch changed
SWAP_CLUSTER_MAX from 32 pages to 256 pages, inadvertantly switching
the scan window for vmpressure detection from 2MB to 16MB. Revert.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/vmpressure.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmpressure.c b/mm/vmpressure.c
index c5afd57..74f206b 100644
--- a/mm/vmpressure.c
+++ b/mm/vmpressure.c
@@ -38,7 +38,7 @@
  * TODO: Make the window size depend on machine size, as we do for vmstat
  * thresholds. Currently we set it to 512 pages (2MB for 4KB pages).
  */
-static const unsigned long vmpressure_win = SWAP_CLUSTER_MAX * 16;
+static const unsigned long vmpressure_win = SWAP_CLUSTER_MAX;
 
 /*
  * These thresholds are used when we account memory pressure through
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1251390 — Re: [PATCH] mm: vmpressure: fix scan window after SWAP_CLUSTER_MAX increase

On Mon, Oct 19, 2015 at 02:13:01PM -0400, Johannes Weiner wrote:
> mm-increase-swap_cluster_max-to-batch-tlb-flushes.patch changed
> SWAP_CLUSTER_MAX from 32 pages to 256 pages, inadvertantly switching
> the scan window for vmpressure detection from 2MB to 16MB. Revert.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

This was known at the time but it was not clear what the measurable
impact would be. VM Pressure is odd in that it gives strange results at
times anyway, particularly on NUMA machines. To be honest, it still isn't
clear to me what the impact of the patch is. With different base page sizes
(e.g. on ppc64 with some configs), the window is still large. At the time,
it was left as-is as I could not decide one way or the other but I'm ok
with restoring the behaviour so either way;

Acked-by: Mel Gorman <mgorman@techsingularity.net>

Out of curiosity though, what *is* the user-visible impact of the patch
though? It's different but I'm having trouble deciding if it's better
or worse. I'm curious as to whether the patch is based on a bug report
or intuition.

Thanks.

-- 
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1251709 — Re: [PATCH] mm: vmpressure: fix scan window after SWAP_CLUSTER_MAX increase

On Tue, Oct 20, 2015 at 08:47:00AM +0100, Mel Gorman wrote:
> On Mon, Oct 19, 2015 at 02:13:01PM -0400, Johannes Weiner wrote:
> > mm-increase-swap_cluster_max-to-batch-tlb-flushes.patch changed
> > SWAP_CLUSTER_MAX from 32 pages to 256 pages, inadvertantly switching
> > the scan window for vmpressure detection from 2MB to 16MB. Revert.
> > 
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> This was known at the time but it was not clear what the measurable
> impact would be. VM Pressure is odd in that it gives strange results at
> times anyway, particularly on NUMA machines.

Interesting. Strange how? In terms of reporting random flukes?

I'm interested in vmpressure because I think reclaim efficiency would
be a useful metric to export to other parts of the kernel. We have
slab shrinkers that export LRU scan rate for ageable caches--the
keyword being "ageable" here--that are currently used for everything
that wants to know about memory pressure. But unless you actually age
and rotate your objects, it does not make sense to shrink your objects
based on LRU scan rate. There is no reason to drop your costly objects
if all the VM does is pick up streaming cache pages. In those cases,
it would make more sense to hook yourself up to be notified when
reclaim efficiency drops below a certain threshold.

> To be honest, it still isn't clear to me what the impact of the
> patch is. With different base page sizes (e.g. on ppc64 with some
> configs), the window is still large. At the time, it was left as-is
> as I could not decide one way or the other but I'm ok with restoring
> the behaviour so either way;
> 
> Acked-by: Mel Gorman <mgorman@techsingularity.net>

Thanks!

> Out of curiosity though, what *is* the user-visible impact of the patch
> though? It's different but I'm having trouble deciding if it's better
> or worse. I'm curious as to whether the patch is based on a bug report
> or intuition.

No, I didn't observe a bug, it just struck me during code review.

My sole line of reasoning was: the pressure scan window was chosen
back then to be at a certain point between reliable sample size and
on-time reporting of detected pressure. Increasing the LRU scan window
for the purpose of improved TLB batching seems sufficiently unrelated
that we wouldn't want to change the vmpressure window as a side effect.

Arguably it should not even reference SWAP_CLUSTER_MAX, but on the
other hand that value has been pretty static in the past, and it looks
better than '256' :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1252607 — Re: [PATCH] mm: vmpressure: fix scan window after SWAP_CLUSTER_MAX increase

On Tue, Oct 20, 2015 at 09:50:56AM -0400, Johannes Weiner wrote:
> On Tue, Oct 20, 2015 at 08:47:00AM +0100, Mel Gorman wrote:
> > On Mon, Oct 19, 2015 at 02:13:01PM -0400, Johannes Weiner wrote:
> > > mm-increase-swap_cluster_max-to-batch-tlb-flushes.patch changed
> > > SWAP_CLUSTER_MAX from 32 pages to 256 pages, inadvertantly switching
> > > the scan window for vmpressure detection from 2MB to 16MB. Revert.
> > > 
> > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> > 
> > This was known at the time but it was not clear what the measurable
> > impact would be. VM Pressure is odd in that it gives strange results at
> > times anyway, particularly on NUMA machines.
> 
> Interesting. Strange how? In terms of reporting random flukes?
> 

I haven't investigated closely myself. What I'm told is that on NUMA
machines that there is a "sawtooth-like" pattern of notifications. At
first, they received low pressure notifications and their application
released some memory. Later they expected more "low" events to be caught
but most times they saw "critical" or "medium" events immediately followed
by multiple critical events, sometimes multiple ones in the same second.
I currently that they are receiving events for one node but freeing on
another but never proved it.

> I'm interested in vmpressure because I think reclaim efficiency would
> be a useful metric to export to other parts of the kernel. We have
> slab shrinkers that export LRU scan rate for ageable caches--the
> keyword being "ageable" here--that are currently used for everything
> that wants to know about memory pressure. But unless you actually age
> and rotate your objects, it does not make sense to shrink your objects
> based on LRU scan rate. There is no reason to drop your costly objects
> if all the VM does is pick up streaming cache pages. In those cases,
> it would make more sense to hook yourself up to be notified when
> reclaim efficiency drops below a certain threshold.
> 

That makes sense but my expectation is that it needs NUMA smarts. It's
nowhere on my radar at the moment. The only thing I had on my plate that
was reclaim-related was to revisit the one-LRU-per-node series if the
atomic reserves and watermark series makes it through.

> > To be honest, it still isn't clear to me what the impact of the
> > patch is. With different base page sizes (e.g. on ppc64 with some
> > configs), the window is still large. At the time, it was left as-is
> > as I could not decide one way or the other but I'm ok with restoring
> > the behaviour so either way;
> > 
> > Acked-by: Mel Gorman <mgorman@techsingularity.net>
> 
> Thanks!
> 
> > Out of curiosity though, what *is* the user-visible impact of the patch
> > though? It's different but I'm having trouble deciding if it's better
> > or worse. I'm curious as to whether the patch is based on a bug report
> > or intuition.
> 
> No, I didn't observe a bug, it just struck me during code review.
> 
> My sole line of reasoning was: the pressure scan window was chosen
> back then to be at a certain point between reliable sample size and
> on-time reporting of detected pressure. Increasing the LRU scan window
> for the purpose of improved TLB batching seems sufficiently unrelated
> that we wouldn't want to change the vmpressure window as a side effect.
> 

That's reasonable and roughly what I expected. The vmpressure stuff was
originally designed with mobile devices in mind. AFAIK, they're reasonably
happy with it so the decisions it uses made sense to some interested user.

> Arguably it should not even reference SWAP_CLUSTER_MAX, but on the
> other hand that value has been pretty static in the past, and it looks
> better than '256' :-)

True. If it was changed, it would be in the context of NUMA smarts with
scaling based on reclaim activity or watermarks. Certainly, it's outside
the scope of what your patch is doing.

-- 
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1253156 — Re: [PATCH] mm: vmpressure: fix scan window after SWAP_CLUSTER_MAX increase

On Mon, Oct 19, 2015 at 02:13:01PM -0400, Johannes Weiner wrote:
> mm-increase-swap_cluster_max-to-batch-tlb-flushes.patch changed
> SWAP_CLUSTER_MAX from 32 pages to 256 pages, inadvertantly switching
> the scan window for vmpressure detection from 2MB to 16MB. Revert.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  mm/vmpressure.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/vmpressure.c b/mm/vmpressure.c
> index c5afd57..74f206b 100644
> --- a/mm/vmpressure.c
> +++ b/mm/vmpressure.c
> @@ -38,7 +38,7 @@
>   * TODO: Make the window size depend on machine size, as we do for vmstat
>   * thresholds. Currently we set it to 512 pages (2MB for 4KB pages).
>   */
> -static const unsigned long vmpressure_win = SWAP_CLUSTER_MAX * 16;
> +static const unsigned long vmpressure_win = SWAP_CLUSTER_MAX;

Argh, Mel's patch sets SWAP_CLUSTER_MAX to 256, so this should be
SWAP_CLUSTER_MAX * 2 to retain the 512 pages scan window.

Andrew could you please update this fix in-place? Otherwise I'll
resend a corrected version.

Thanks, and sorry about that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1253172 — Re: [PATCH] mm: vmpressure: fix scan window after SWAP_CLUSTER_MAX increase

On Wed, 21 Oct 2015, Johannes Weiner wrote:
> On Mon, Oct 19, 2015 at 02:13:01PM -0400, Johannes Weiner wrote:
> > mm-increase-swap_cluster_max-to-batch-tlb-flushes.patch changed
> > SWAP_CLUSTER_MAX from 32 pages to 256 pages, inadvertantly switching
> > the scan window for vmpressure detection from 2MB to 16MB. Revert.
> > 
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> > ---
> >  mm/vmpressure.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/mm/vmpressure.c b/mm/vmpressure.c
> > index c5afd57..74f206b 100644
> > --- a/mm/vmpressure.c
> > +++ b/mm/vmpressure.c
> > @@ -38,7 +38,7 @@
> >   * TODO: Make the window size depend on machine size, as we do for vmstat
> >   * thresholds. Currently we set it to 512 pages (2MB for 4KB pages).
> >   */
> > -static const unsigned long vmpressure_win = SWAP_CLUSTER_MAX * 16;
> > +static const unsigned long vmpressure_win = SWAP_CLUSTER_MAX;
> 
> Argh, Mel's patch sets SWAP_CLUSTER_MAX to 256, so this should be
> SWAP_CLUSTER_MAX * 2 to retain the 512 pages scan window.
> 
> Andrew could you please update this fix in-place? Otherwise I'll
> resend a corrected version.
> 
> Thanks, and sorry about that.

I don't understand why "SWAP_CLUSTER_MAX * 2" is thought better than "512".
Retaining a level of obscurity, that just bit us twice, is a good thing?

Hugh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web