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

Groups > linux.kernel > #1561827 > unrolled thread

[RFC PATCH 0/2] fix unbounded too_many_isolated

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 0/2] fix unbounded too_many_isolated Michal Hocko <mhocko@kernel.org> - 2017-01-18 15:00 +0100
    [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages per zone Michal Hocko <mhocko@kernel.org> - 2017-01-18 15:00 +0100
      Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages  per zone Mel Gorman <mgorman@suse.de> - 2017-01-18 15:50 +0100
        Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages  per zone Michal Hocko <mhocko@kernel.org> - 2017-01-18 16:20 +0100
          Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages  per zone Mel Gorman <mgorman@suse.de> - 2017-01-18 17:00 +0100
            Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages  per zone Michal Hocko <mhocko@kernel.org> - 2017-01-18 17:20 +0100
              Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages  per zone Mel Gorman <mgorman@suse.de> - 2017-01-18 18:10 +0100
                Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages  per zone Michal Hocko <mhocko@kernel.org> - 2017-01-18 18:30 +0100
                  Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages  per zone Mel Gorman <mgorman@suse.de> - 2017-01-19 11:10 +0100
                    Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages  per zone Michal Hocko <mhocko@kernel.org> - 2017-01-19 12:50 +0100
                      Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages  per zone Mel Gorman <mgorman@suse.de> - 2017-01-19 14:20 +0100
                        Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages per zone Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-01-20 15:20 +0100
                          Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages per zone Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-01-21 08:50 +0100
                            Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages  per zone Michal Hocko <mhocko@kernel.org> - 2017-01-25 11:20 +0100
                              Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated         pages per zone Christoph Hellwig <hch@lst.de> - 2017-01-25 11:30 +0100
                                Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages  per zone Michal Hocko <mhocko@kernel.org> - 2017-01-25 11:50 +0100
                                  Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages per zone Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-01-25 12:20 +0100
                                    Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages  per zone Michal Hocko <mhocko@kernel.org> - 2017-01-25 14:10 +0100
                                      Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages  per zone Michal Hocko <mhocko@kernel.org> - 2017-01-27 16:00 +0100
                                        Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages per zone Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-01-28 18:20 +0100
                                          Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages  per zone Michal Hocko <mhocko@kernel.org> - 2017-01-30 10:00 +0100
                              Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pagesper zone Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-01-25 11:40 +0100
                                Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated  pagesper zone Michal Hocko <mhocko@kernel.org> - 2017-01-25 13:40 +0100
                                  Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages per zone Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-01-25 14:20 +0100
                          Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages  per zone Michal Hocko <mhocko@kernel.org> - 2017-01-25 11:00 +0100
                    Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages per zone "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2017-01-20 07:50 +0100
                      Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages  per zone Mel Gorman <mgorman@suse.de> - 2017-01-20 10:30 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1569473 — Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages per zone

On Sun 29-01-17 00:27:27, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > Tetsuo,
> > before we settle on the proper fix for this issue, could you give the
> > patch a try and try to reproduce the too_many_isolated() issue or
> > just see whether patch [1] has any negative effect on your oom stress
> > testing?
> > 
> > [1] http://lkml.kernel.org/r/20170119112336.GN30786@dhcp22.suse.cz
> 
> I tested with both [1] and below patch applied on linux-next-20170125 and
> the result is at http://I-love.SAKURA.ne.jp/tmp/serial-20170128.txt.xz .
> 
> Regarding below patch, it helped avoiding complete memory depletion with
> large write() request. I don't know whether below patch helps avoiding
> complete memory depletion when reading large amount (in other words, I
> don't know whether this check is done for large read() request).

It's not AFAICS. do_generic_file_read doesn't do the
fatal_signal_pending check.

> But
> I believe that __GFP_KILLABLE (despite the limitation that there are
> unkillable waits in the reclaim path) is better solution compared to
> scattering around fatal_signal_pending() in the callers. The reason
> we check SIGKILL here is to avoid allocating memory more than needed.
> If we check SIGKILL in the entry point of __alloc_pages_nodemask() and
> retry: label in __alloc_pages_slowpath(), we waste 0 page. Regardless
> of whether the OOM killer is invoked, whether memory can be allocated
> without direct reclaim operation, not allocating memory unless needed
> (in other words, allow page allocator fail immediately if the caller
> can give up on SIGKILL and SIGKILL is pending) makes sense. It will
> reduce possibility of OOM livelock on CONFIG_MMU=n kernels where the
> OOM reaper is not available.

I am not really convinced this is a good idea. Put aside the fuzzy
semantic of __GFP_KILLABLE, we would have to use this flag in all
potentially allocating places from read/write paths and then it is just
easier to do the explicit checks in the the loops around those
allocations.
 
> > On Wed 25-01-17 14:00:14, Michal Hocko wrote:
> > [...]
> > > From 362da5cac527146a341300c2ca441245c16043e8 Mon Sep 17 00:00:00 2001
> > > From: Michal Hocko <mhocko@suse.com>
> > > Date: Wed, 25 Jan 2017 11:06:37 +0100
> > > Subject: [PATCH] fs: break out of iomap_file_buffered_write on fatal signals
> > > 
> > > Tetsuo has noticed that an OOM stress test which performs large write
> > > requests can cause the full memory reserves depletion. He has tracked
> > > this down to the following path
> > > 	__alloc_pages_nodemask+0x436/0x4d0
> > > 	alloc_pages_current+0x97/0x1b0
> > > 	__page_cache_alloc+0x15d/0x1a0          mm/filemap.c:728
> > > 	pagecache_get_page+0x5a/0x2b0           mm/filemap.c:1331
> > > 	grab_cache_page_write_begin+0x23/0x40   mm/filemap.c:2773
> > > 	iomap_write_begin+0x50/0xd0             fs/iomap.c:118
> > > 	iomap_write_actor+0xb5/0x1a0            fs/iomap.c:190
> > > 	? iomap_write_end+0x80/0x80             fs/iomap.c:150
> > > 	iomap_apply+0xb3/0x130                  fs/iomap.c:79
> > > 	iomap_file_buffered_write+0x68/0xa0     fs/iomap.c:243
> > > 	? iomap_write_end+0x80/0x80
> > > 	xfs_file_buffered_aio_write+0x132/0x390 [xfs]
> > > 	? remove_wait_queue+0x59/0x60
> > > 	xfs_file_write_iter+0x90/0x130 [xfs]
> > > 	__vfs_write+0xe5/0x140
> > > 	vfs_write+0xc7/0x1f0
> > > 	? syscall_trace_enter+0x1d0/0x380
> > > 	SyS_write+0x58/0xc0
> > > 	do_syscall_64+0x6c/0x200
> > > 	entry_SYSCALL64_slow_path+0x25/0x25
> > > 
> > > the oom victim has access to all memory reserves to make a forward
> > > progress to exit easier. But iomap_file_buffered_write and other callers
> > > of iomap_apply loop to complete the full request. We need to check for
> > > fatal signals and back off with a short write instead. As the
> > > iomap_apply delegates all the work down to the actor we have to hook
> > > into those. All callers that work with the page cache are calling
> > > iomap_write_begin so we will check for signals there. dax_iomap_actor
> > > has to handle the situation explicitly because it copies data to the
> > > userspace directly. Other callers like iomap_page_mkwrite work on a
> > > single page or iomap_fiemap_actor do not allocate memory based on the
> > > given len.
> > > 
> > > Fixes: 68a9f5e7007c ("xfs: implement iomap based buffered write path")
> > > Cc: stable # 4.8+
> > > Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > > ---
> > >  fs/dax.c   | 5 +++++
> > >  fs/iomap.c | 3 +++
> > >  2 files changed, 8 insertions(+)
> > > 
> > > diff --git a/fs/dax.c b/fs/dax.c
> > > index 413a91db9351..0e263dacf9cf 100644
> > > --- a/fs/dax.c
> > > +++ b/fs/dax.c
> > > @@ -1033,6 +1033,11 @@ dax_iomap_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
> > >  		struct blk_dax_ctl dax = { 0 };
> > >  		ssize_t map_len;
> > >  
> > > +		if (fatal_signal_pending(current)) {
> > > +			ret = -EINTR;
> > > +			break;
> > > +		}
> > > +
> > >  		dax.sector = dax_iomap_sector(iomap, pos);
> > >  		dax.size = (length + offset + PAGE_SIZE - 1) & PAGE_MASK;
> > >  		map_len = dax_map_atomic(iomap->bdev, &dax);
> > > diff --git a/fs/iomap.c b/fs/iomap.c
> > > index e57b90b5ff37..691eada58b06 100644
> > > --- a/fs/iomap.c
> > > +++ b/fs/iomap.c
> > > @@ -114,6 +114,9 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
> > >  
> > >  	BUG_ON(pos + len > iomap->offset + iomap->length);
> > >  
> > > +	if (fatal_signal_pending(current))
> > > +		return -EINTR;
> > > +
> > >  	page = grab_cache_page_write_begin(inode->i_mapping, index, flags);
> > >  	if (!page)
> > >  		return -ENOMEM;
> > > -- 
> > > 2.11.0
> 
> Regarding [1], it helped avoiding the too_many_isolated() issue. I can't
> tell whether it has any negative effect, but I got on the first trial that
> all allocating threads are blocked on wait_for_completion() from flush_work()
> in drain_all_pages() introduced by "mm, page_alloc: drain per-cpu pages from
> workqueue context". There was no warn_alloc() stall warning message afterwords.

That patch is buggy and there is a follow up [1] which is not sitting in the
mmotm (and thus linux-next) yet. I didn't get to review it properly and
I cannot say I would be too happy about using WQ from the page
allocator. I believe even the follow up needs to have WQ_RECLAIM WQ.

[1] http://lkml.kernel.org/r/20170125083038.rzb5f43nptmk7aed@techsingularity.net

Thanks for your testing!
-- 
Michal Hocko
SUSE Labs

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

#1566463 — Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pagesper zone

Michal Hocko wrote:
> I think we are missing a check for fatal_signal_pending in
> iomap_file_buffered_write. This means that an oom victim can consume the
> full memory reserves. What do you think about the following? I haven't
> tested this but it mimics generic_perform_write so I guess it should
> work.

Looks OK to me. I worried

#define AOP_FLAG_UNINTERRUPTIBLE        0x0001 /* will not do a short write */

which forbids (!?) aborting the loop. But it seems that this flag is
no longer checked (i.e. set but not used). So, everybody should be ready
for short write, although I don't know whether exofs / hfs / hfsplus are
doing appropriate error handling.

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

#1566560 — Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pagesper zone

On Wed 25-01-17 19:33:59, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > I think we are missing a check for fatal_signal_pending in
> > iomap_file_buffered_write. This means that an oom victim can consume the
> > full memory reserves. What do you think about the following? I haven't
> > tested this but it mimics generic_perform_write so I guess it should
> > work.
> 
> Looks OK to me. I worried
> 
> #define AOP_FLAG_UNINTERRUPTIBLE        0x0001 /* will not do a short write */
> 
> which forbids (!?) aborting the loop. But it seems that this flag is
> no longer checked (i.e. set but not used). So, everybody should be ready
> for short write, although I don't know whether exofs / hfs / hfsplus are
> doing appropriate error handling.

Those were using generic implementation before and that handles this
case AFAICS.

-- 
Michal Hocko
SUSE Labs

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

#1566585 — Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages per zone

Michal Hocko wrote:
> On Wed 25-01-17 19:33:59, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > I think we are missing a check for fatal_signal_pending in
> > > iomap_file_buffered_write. This means that an oom victim can consume the
> > > full memory reserves. What do you think about the following? I haven't
> > > tested this but it mimics generic_perform_write so I guess it should
> > > work.
> > 
> > Looks OK to me. I worried
> > 
> > #define AOP_FLAG_UNINTERRUPTIBLE        0x0001 /* will not do a short write */
> > 
> > which forbids (!?) aborting the loop. But it seems that this flag is
> > no longer checked (i.e. set but not used). So, everybody should be ready
> > for short write, although I don't know whether exofs / hfs / hfsplus are
> > doing appropriate error handling.
> 
> Those were using generic implementation before and that handles this
> case AFAICS.

What I wanted to say is: "We can remove AOP_FLAG_UNINTERRUPTIBLE completely
because grep does not find that flag used in condition check, can't we?".

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

#1566434 — Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages per zone

On Fri 20-01-17 22:27:27, Tetsuo Handa wrote:
> Mel Gorman wrote:
> > On Thu, Jan 19, 2017 at 12:23:36PM +0100, Michal Hocko wrote:
> > > So what do you think about the following? Tetsuo, would you be willing
> > > to run this patch through your torture testing please?
> > 
> > I'm fine with treating this as a starting point.
> 
> OK. So I tried to test this patch but I failed at preparation step.
> There are too many pending mm patches and I'm not sure which patch on
> which linux-next snapshot I should try.

The current linux-next should be good to test. It contains all patches
sitting in the mmotm tree. If you want a more stable base then you can
use mmotm git tree
(git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git #since-4.9
or its #auto-latest alias)

> Also as another question,
> too_many_isolated() loop exists in both mm/vmscan.c and mm/compaction.c
> but why this patch does not touch the loop in mm/compaction.c part?

I am not yet convinced the compaction suffers from the same problem.
Compaction backs off much sooner so that path shouldn't get into
pathological situation AFAICS. I might be wrong here but I think we
should start with the reclaim path first.
-- 
Michal Hocko
SUSE Labs

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

#1563321 — Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages per zone

On Thursday, January 19, 2017 6:08 PM Mel Gorman wrote: 
> 
> If it's definitely required and is proven to fix the
> infinite-loop-without-oom workload then I'll back off and withdraw my
> objections. However, I'd at least like the following untested patch to
> be considered as an alternative. It has some weaknesses and would be
> slower to OOM than your patch but it avoids reintroducing zone counters
> 
> ---8<---
> mm, vmscan: Wait on a waitqueue when too many pages are isolated
> 
> When too many pages are isolated, direct reclaim waits on congestion to clear
> for up to a tenth of a second. There is no reason to believe that too many
> pages are isolated due to dirty pages, reclaim efficiency or congestion.
> It may simply be because an extremely large number of processes have entered
> direct reclaim at the same time. However, it is possible for the situation
> to persist forever and never reach OOM.
> 
> This patch queues processes a waitqueue when too many pages are isolated.
> When parallel reclaimers finish shrink_page_list, they wake the waiters
> to recheck whether too many pages are isolated.
> 
> The wait on the queue has a timeout as not all sites that isolate pages
> will do the wakeup. Depending on every isolation of LRU pages to be perfect
> forever is potentially fragile. The specific wakeups occur for page reclaim
> and compaction. If too many pages are isolated due to memory failure,
> hotplug or directly calling migration from a syscall then the waiting
> processes may wait the full timeout.
> 
> Note that the timeout allows the use of waitqueue_active() on the basis
> that a race will cause the full timeout to be reached due to a missed
> wakeup. This is relatively harmless and still a massive improvement over
> unconditionally calling congestion_wait.
> 
> Direct reclaimers that cannot isolate pages within the timeout will consider
> return to the caller. This is somewhat clunky as it won't return immediately
> and make go through the other priorities and slab shrinking. Eventually,
> it'll go through a few iterations of should_reclaim_retry and reach the
> MAX_RECLAIM_RETRIES limit and consider going OOM.
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 91f69aa0d581..3dd617d0c8c4 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -628,6 +628,7 @@ typedef struct pglist_data {
>  	int node_id;
>  	wait_queue_head_t kswapd_wait;
>  	wait_queue_head_t pfmemalloc_wait;
> +	wait_queue_head_t isolated_wait;
>  	struct task_struct *kswapd;	/* Protected by
>  					   mem_hotplug_begin/end() */
>  	int kswapd_order;
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 43a6cf1dc202..1b1ff6da7401 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1634,6 +1634,10 @@ static enum compact_result compact_zone(struct zone *zone, struct compact_contro
>  	count_compact_events(COMPACTMIGRATE_SCANNED, cc->total_migrate_scanned);
>  	count_compact_events(COMPACTFREE_SCANNED, cc->total_free_scanned);
> 
> +	/* Page reclaim could have stalled due to isolated pages */
> +	if (waitqueue_active(&zone->zone_pgdat->isolated_wait))
> +		wake_up(&zone->zone_pgdat->isolated_wait);
> +
>  	trace_mm_compaction_end(start_pfn, cc->migrate_pfn,
>  				cc->free_pfn, end_pfn, sync, ret);
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 8ff25883c172..d848c9f31bff 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5823,6 +5823,7 @@ static void __paginginit free_area_init_core(struct pglist_data *pgdat)
>  #endif
>  	init_waitqueue_head(&pgdat->kswapd_wait);
>  	init_waitqueue_head(&pgdat->pfmemalloc_wait);
> +	init_waitqueue_head(&pgdat->isolated_wait);
>  #ifdef CONFIG_COMPACTION
>  	init_waitqueue_head(&pgdat->kcompactd_wait);
>  #endif
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 2281ad310d06..c93f299fbad7 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1603,16 +1603,16 @@ int isolate_lru_page(struct page *page)
>   * the LRU list will go small and be scanned faster than necessary, leading to
>   * unnecessary swapping, thrashing and OOM.
>   */
> -static int too_many_isolated(struct pglist_data *pgdat, int file,
> +static bool safe_to_isolate(struct pglist_data *pgdat, int file,
>  		struct scan_control *sc)

I prefer the current function name.

>  {
>  	unsigned long inactive, isolated;
> 
>  	if (current_is_kswapd())
> -		return 0;
> +		return true;
> 
> -	if (!sane_reclaim(sc))
> -		return 0;
> +	if (sane_reclaim(sc))
> +		return true;

We only need a one-line change.
> 
>  	if (file) {
>  		inactive = node_page_state(pgdat, NR_INACTIVE_FILE);
> @@ -1630,7 +1630,7 @@ static int too_many_isolated(struct pglist_data *pgdat, int file,
>  	if ((sc->gfp_mask & (__GFP_IO | __GFP_FS)) == (__GFP_IO | __GFP_FS))
>  		inactive >>= 3;
> 
> -	return isolated > inactive;
> +	return isolated < inactive;
>  }
> 
>  static noinline_for_stack void
> @@ -1719,12 +1719,28 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
>  	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>  	struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat;
> 
> -	while (unlikely(too_many_isolated(pgdat, file, sc))) {
> -		congestion_wait(BLK_RW_ASYNC, HZ/10);
> +	while (!safe_to_isolate(pgdat, file, sc)) {
> +		long ret;
> +
> +		ret = wait_event_interruptible_timeout(pgdat->isolated_wait,
> +			safe_to_isolate(pgdat, file, sc), HZ/10);
> 
>  		/* We are about to die and free our memory. Return now. */
> -		if (fatal_signal_pending(current))
> -			return SWAP_CLUSTER_MAX;
> +		if (fatal_signal_pending(current)) {
> +			nr_reclaimed = SWAP_CLUSTER_MAX;
> +			goto out;
> +		}
> +
> +		/*
> +		 * If we reached the timeout, this is direct reclaim, and
> +		 * pages cannot be isolated then return. If the situation

Please add something that we would rather shrink slab than go
another round of nap.

> +		 * persists for a long time then it'll eventually reach
> +		 * the no_progress limit in should_reclaim_retry and consider
> +		 * going OOM. In this case, do not wake the isolated_wait
> +		 * queue as the wakee will still not be able to make progress.
> +		 */
> +		if (!ret && !current_is_kswapd() && !safe_to_isolate(pgdat, file, sc))
> +			return 0;
>  	}
> 
>  	lru_add_drain();
> @@ -1839,6 +1855,10 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
>  			stat.nr_activate, stat.nr_ref_keep,
>  			stat.nr_unmap_fail,
>  			sc->priority, file);
> +
> +out:
> +	if (waitqueue_active(&pgdat->isolated_wait))
> +		wake_up(&pgdat->isolated_wait);
>  	return nr_reclaimed;
>  }
> 
Is it also needed to check isolated_wait active before kswapd 
takes nap?

thanks
Hillf

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

#1563420 — Re: [RFC PATCH 1/2] mm, vmscan: account the number of isolated pages per zone

On Fri, Jan 20, 2017 at 02:42:24PM +0800, Hillf Danton wrote:
> > @@ -1603,16 +1603,16 @@ int isolate_lru_page(struct page *page)
> >   * the LRU list will go small and be scanned faster than necessary, leading to
> >   * unnecessary swapping, thrashing and OOM.
> >   */
> > -static int too_many_isolated(struct pglist_data *pgdat, int file,
> > +static bool safe_to_isolate(struct pglist_data *pgdat, int file,
> >  		struct scan_control *sc)
> 
> I prefer the current function name.
> 

The restructure is to work with the workqueue api.

> >  {
> >  	unsigned long inactive, isolated;
> > 
> >  	if (current_is_kswapd())
> > -		return 0;
> > +		return true;
> > 
> > -	if (!sane_reclaim(sc))
> > -		return 0;
> > +	if (sane_reclaim(sc))
> > +		return true;
> 
> We only need a one-line change.

It's bool so the conversion is made to bool while it's being changed
anyway.

> > 
> >  	if (file) {
> >  		inactive = node_page_state(pgdat, NR_INACTIVE_FILE);
> > @@ -1630,7 +1630,7 @@ static int too_many_isolated(struct pglist_data *pgdat, int file,
> >  	if ((sc->gfp_mask & (__GFP_IO | __GFP_FS)) == (__GFP_IO | __GFP_FS))
> >  		inactive >>= 3;
> > 
> > -	return isolated > inactive;
> > +	return isolated < inactive;
> >  }
> > 
> >  static noinline_for_stack void
> > @@ -1719,12 +1719,28 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
> >  	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
> >  	struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat;
> > 
> > -	while (unlikely(too_many_isolated(pgdat, file, sc))) {
> > -		congestion_wait(BLK_RW_ASYNC, HZ/10);
> > +	while (!safe_to_isolate(pgdat, file, sc)) {
> > +		long ret;
> > +
> > +		ret = wait_event_interruptible_timeout(pgdat->isolated_wait,
> > +			safe_to_isolate(pgdat, file, sc), HZ/10);
> > 
> >  		/* We are about to die and free our memory. Return now. */
> > -		if (fatal_signal_pending(current))
> > -			return SWAP_CLUSTER_MAX;
> > +		if (fatal_signal_pending(current)) {
> > +			nr_reclaimed = SWAP_CLUSTER_MAX;
> > +			goto out;
> > +		}
> > +
> > +		/*
> > +		 * If we reached the timeout, this is direct reclaim, and
> > +		 * pages cannot be isolated then return. If the situation
> 
> Please add something that we would rather shrink slab than go
> another round of nap.
> 

That's not necessarily true or even a good idea. It could result in
excessive slab shrinking that is no longer in proportion to LRU scanning
and increased contention within shrinkers.

> > +		 * persists for a long time then it'll eventually reach
> > +		 * the no_progress limit in should_reclaim_retry and consider
> > +		 * going OOM. In this case, do not wake the isolated_wait
> > +		 * queue as the wakee will still not be able to make progress.
> > +		 */
> > +		if (!ret && !current_is_kswapd() && !safe_to_isolate(pgdat, file, sc))
> > +			return 0;
> >  	}
> > 
> >  	lru_add_drain();
> > @@ -1839,6 +1855,10 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
> >  			stat.nr_activate, stat.nr_ref_keep,
> >  			stat.nr_unmap_fail,
> >  			sc->priority, file);
> > +
> > +out:
> > +	if (waitqueue_active(&pgdat->isolated_wait))
> > +		wake_up(&pgdat->isolated_wait);
> >  	return nr_reclaimed;
> >  }
> > 
> Is it also needed to check isolated_wait active before kswapd 
> takes nap?
> 

No because this is where pages were isolated and there is no putback
event that would justify waking the queue. There is a race between
waitqueue_active() and going to sleep that we rely on the timeout to
recover from.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web