All of lore.kernel.org
 help / color / mirror / Atom feed
From: Michal Hocko <mhocko@kernel.org>
To: vinayak menon <vinayakm.list@gmail.com>
Cc: Vinayak Menon <vinmenon@codeaurora.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Johannes Weiner <hannes@cmpxchg.org>,
	mgorman@techsingularity.net, vbabka@suse.cz,
	Rik van Riel <riel@redhat.com>,
	vdavydov.dev@gmail.com, anton.vorontsov@linaro.org,
	Minchan Kim <minchan@kernel.org>,
	shashim@codeaurora.org, "linux-mm@kvack.org" <linux-mm@kvack.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2 v5] mm: vmscan: do not pass reclaimed slab to vmpressure
Date: Fri, 10 Feb 2017 10:05:01 +0100	[thread overview]
Message-ID: <20170210090501.GE10893@dhcp22.suse.cz> (raw)
In-Reply-To: <CAOaiJ-nJWeMWeY1S5rBmC3M1EiT+HbiLcPwEMZsDMHemhGO0jA@mail.gmail.com>

On Fri 10-02-17 14:15:20, vinayak menon wrote:
> On Thu, Feb 9, 2017 at 5:50 PM, Michal Hocko <mhocko@kernel.org> wrote:
[...]
> > I have already said I will _not_ NAK the patch but we need a much better
> > description and justification why the older behavior was better to
> > consider this a regression before this can be merged. It is hard to
> > expect that the underlying implementation of the vmpressure will stay
> > carved in stone and there might be changes in this area in the future. I
> > want to hear why we believe that the tested workload is sufficiently
> > universal and we won't see another report in few months because somebody
> > else will see higher vmpressure levels even though we make reclaim
> > progress. I have asked those questions already but it seems those were
> > ignored.
> 
> The tested workload is not universal. The lowmemorykiller example was used just
> to mention the effect of vmpressure change on one of the workloads. 

My point is whether this workload even matters. AFAIU the test benefits
from killing as quickly as possible, right? So it directly benefits from
seeing critical events as soon as possible even when the reclaim makes
progress.

> I can drop the reclaim stats and just keep the stats of change
> observed in vmpressure critical events.  I am not sure whether we
> would see another issue reported with this patch. We may because
> someone would have written a code that works with this new vmpressure
> values. I am not sure whether that matters because the core issue
> is whether the kernel is reporting the right values.

Right. THe right values is a bit fuzzy, though.

> This could be
> termed as a regression because,
> 
> 1) Accounting only reclaimed pages to a model which works on scanned
> and reclaimed seems like a wrong thing. It is just adding noise to
> it. There could be issues with vmpressure implementation, but it at
> least gives an estimate on what the pressure on LRU is. There are many
> other shrinkers like zsmalloc which does not report reclaimed pages,
> and when add those also in a similar fashion without considering the
> cost part, vmpressure values would always remain low. So util we
> have a way to give correct information to vmpressure about non-LRU
> reclaimers, I feel its better to keep it in its original form.

Yeah, I understand that the current cost model is far from ideal and it
needs fixing. My main question would be whether the model would be much
better if we exclude pages freed from the slab shrinkers. I can only say
it would be more pesimistic that way. Is this a good thing? If yes, why?
 
> 2) As Minchan mentioned, the cost model is different and thus adding
> slab reclaimed would not be the right thing to do at this point.
> 
> But if you feel we don't have to fix this now and that it is better
> to fix the core problems with vmpressure first, that's ok.

Yes, I believe we should reconsider how we calculate the pressure
levels. This seems a larger project but definitely something we need. I
do not have a good ideas how to do this properly
-- 
Michal Hocko
SUSE Labs

WARNING: multiple messages have this Message-ID
From: Michal Hocko <mhocko@kernel.org>
To: vinayak menon <vinayakm.list@gmail.com>
Cc: Vinayak Menon <vinmenon@codeaurora.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Johannes Weiner <hannes@cmpxchg.org>,
	mgorman@techsingularity.net, vbabka@suse.cz,
	Rik van Riel <riel@redhat.com>,
	vdavydov.dev@gmail.com, anton.vorontsov@linaro.org,
	Minchan Kim <minchan@kernel.org>,
	shashim@codeaurora.org, "linux-mm@kvack.org" <linux-mm@kvack.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2 v5] mm: vmscan: do not pass reclaimed slab to vmpressure
Date: Fri, 10 Feb 2017 10:05:01 +0100	[thread overview]
Message-ID: <20170210090501.GE10893@dhcp22.suse.cz> (raw)
In-Reply-To: <CAOaiJ-nJWeMWeY1S5rBmC3M1EiT+HbiLcPwEMZsDMHemhGO0jA@mail.gmail.com>

On Fri 10-02-17 14:15:20, vinayak menon wrote:
> On Thu, Feb 9, 2017 at 5:50 PM, Michal Hocko <mhocko@kernel.org> wrote:
[...]
> > I have already said I will _not_ NAK the patch but we need a much better
> > description and justification why the older behavior was better to
> > consider this a regression before this can be merged. It is hard to
> > expect that the underlying implementation of the vmpressure will stay
> > carved in stone and there might be changes in this area in the future. I
> > want to hear why we believe that the tested workload is sufficiently
> > universal and we won't see another report in few months because somebody
> > else will see higher vmpressure levels even though we make reclaim
> > progress. I have asked those questions already but it seems those were
> > ignored.
> 
> The tested workload is not universal. The lowmemorykiller example was used just
> to mention the effect of vmpressure change on one of the workloads. 

My point is whether this workload even matters. AFAIU the test benefits
from killing as quickly as possible, right? So it directly benefits from
seeing critical events as soon as possible even when the reclaim makes
progress.

> I can drop the reclaim stats and just keep the stats of change
> observed in vmpressure critical events.  I am not sure whether we
> would see another issue reported with this patch. We may because
> someone would have written a code that works with this new vmpressure
> values. I am not sure whether that matters because the core issue
> is whether the kernel is reporting the right values.

Right. THe right values is a bit fuzzy, though.

> This could be
> termed as a regression because,
> 
> 1) Accounting only reclaimed pages to a model which works on scanned
> and reclaimed seems like a wrong thing. It is just adding noise to
> it. There could be issues with vmpressure implementation, but it at
> least gives an estimate on what the pressure on LRU is. There are many
> other shrinkers like zsmalloc which does not report reclaimed pages,
> and when add those also in a similar fashion without considering the
> cost part, vmpressure values would always remain low. So util we
> have a way to give correct information to vmpressure about non-LRU
> reclaimers, I feel its better to keep it in its original form.

Yeah, I understand that the current cost model is far from ideal and it
needs fixing. My main question would be whether the model would be much
better if we exclude pages freed from the slab shrinkers. I can only say
it would be more pesimistic that way. Is this a good thing? If yes, why?
 
> 2) As Minchan mentioned, the cost model is different and thus adding
> slab reclaimed would not be the right thing to do at this point.
> 
> But if you feel we don't have to fix this now and that it is better
> to fix the core problems with vmpressure first, that's ok.

Yes, I believe we should reconsider how we calculate the pressure
levels. This seems a larger project but definitely something we need. I
do not have a good ideas how to do this properly
-- 
Michal Hocko
SUSE Labs

--
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-02-10  9:25 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-09 11:59 [PATCH 1/2 v2] mm: vmpressure: fix sending wrong events on underflow Vinayak Menon
2017-02-09 11:59 ` Vinayak Menon
2017-02-09 11:59 ` [PATCH 2/2 v5] mm: vmscan: do not pass reclaimed slab to vmpressure Vinayak Menon
2017-02-09 11:59   ` Vinayak Menon
2017-02-09 12:20   ` Michal Hocko
2017-02-09 12:20     ` Michal Hocko
2017-02-10  8:45     ` vinayak menon
2017-02-10  8:45       ` vinayak menon
2017-02-10  9:05       ` Michal Hocko [this message]
2017-02-10  9:05         ` Michal Hocko
2017-02-09 12:10 ` [PATCH 1/2 v2] mm: vmpressure: fix sending wrong events on underflow Michal Hocko
2017-02-09 12:10   ` Michal Hocko
2017-02-09 12:22   ` Michal Hocko
2017-02-09 12:22     ` Michal Hocko

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=20170210090501.GE10893@dhcp22.suse.cz \
    --to=mhocko@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=anton.vorontsov@linaro.org \
    --cc=hannes@cmpxchg.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mgorman@techsingularity.net \
    --cc=minchan@kernel.org \
    --cc=riel@redhat.com \
    --cc=shashim@codeaurora.org \
    --cc=vbabka@suse.cz \
    --cc=vdavydov.dev@gmail.com \
    --cc=vinayakm.list@gmail.com \
    --cc=vinmenon@codeaurora.org \
    --subject='Re: [PATCH 2/2 v5] mm: vmscan: do not pass reclaimed slab to vmpressure' \
    /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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.