All of lore.kernel.org
 help / color / mirror / Atom feed
From: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
To: Hugh Dickins <hughd@google.com>
Cc: kosaki.motohiro@gmail.com,
	Andrew Morton <akpm@linux-foundation.org>,
	Minchan Kim <minchan.kim@gmail.com>,
	Rik van Riel <riel@redhat.com>, Shaohua Li <shaohua.li@intel.com>,
	Eric Dumazet <eric.dumazet@gmail.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Michel Lespinasse <walken@google.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH 2/2] SHM_UNLOCK: fix Unevictable pages stranded after swap
Date: Mon, 09 Jan 2012 15:42:46 -0500	[thread overview]
Message-ID: <4F0B5146.6090200@gmail.com> (raw)
In-Reply-To: <alpine.LSU.2.00.1201061310340.12082@eggly.anvils>



2012/1/6 Hugh Dickins <hughd@google.com>:
> Commit cc39c6a9bbde "mm: account skipped entries to avoid looping in
> find_get_pages" correctly fixed an infinite loop; but left a problem
> that find_get_pages() on shmem would return 0 (appearing to callers
> to mean end of tree) when it meets a run of nr_pages swap entries.
>
> The only uses of find_get_pages() on shmem are via pagevec_lookup(),
> called from invalidate_mapping_pages(), and from shmctl SHM_UNLOCK's
> scan_mapping_unevictable_pages().  The first is already commented,
> and not worth worrying about; but the second can leave pages on the
> Unevictable list after an unusual sequence of swapping and locking.
>
> Fix that by using shmem_find_get_pages_and_swap() (then ignoring
> the swap) instead of pagevec_lookup().
>
> But I don't want to contaminate vmscan.c with shmem internals, nor
> shmem.c with LRU locking.  So move scan_mapping_unevictable_pages()
> into shmem.c, renaming it shmem_unlock_mapping(); and rename
> check_move_unevictable_page() to check_move_unevictable_pages(),
> looping down an array of pages, oftentimes under the same lock.
>
> Leave out the "rotate unevictable list" block: that's a leftover
> from when this was used for /proc/sys/vm/scan_unevictable_pages,
> whose flawed handling involved looking at pages at tail of LRU.
>
> Was there significance to the sequence first ClearPageUnevictable,
> then test page_evictable, then SetPageUnevictable here?  I think
> not, we're under LRU lock, and have no barriers between those.

If I understand correctly, this is not exactly correct. Because of,
PG_mlocked operation is not protected by LRU lock. So, I think we
have three choice.

1) check_move_unevictable_pages() aimed retry logic and put pages back
    into correct lru.
2) check_move_unevictable_pages() unconditionally move the pages into
    evictable lru, and vmacan put them back into correct lru later.
3) To protect PG_mlock operation by lru lock.


other parts looks fine to me.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

WARNING: multiple messages have this Message-ID (diff)
From: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
To: Hugh Dickins <hughd@google.com>
Cc: kosaki.motohiro@gmail.com,
	Andrew Morton <akpm@linux-foundation.org>,
	Minchan Kim <minchan.kim@gmail.com>,
	Rik van Riel <riel@redhat.com>, Shaohua Li <shaohua.li@intel.com>,
	Eric Dumazet <eric.dumazet@gmail.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Michel Lespinasse <walken@google.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH 2/2] SHM_UNLOCK: fix Unevictable pages stranded after swap
Date: Mon, 09 Jan 2012 15:42:46 -0500	[thread overview]
Message-ID: <4F0B5146.6090200@gmail.com> (raw)
In-Reply-To: <alpine.LSU.2.00.1201061310340.12082@eggly.anvils>



2012/1/6 Hugh Dickins <hughd@google.com>:
> Commit cc39c6a9bbde "mm: account skipped entries to avoid looping in
> find_get_pages" correctly fixed an infinite loop; but left a problem
> that find_get_pages() on shmem would return 0 (appearing to callers
> to mean end of tree) when it meets a run of nr_pages swap entries.
>
> The only uses of find_get_pages() on shmem are via pagevec_lookup(),
> called from invalidate_mapping_pages(), and from shmctl SHM_UNLOCK's
> scan_mapping_unevictable_pages().  The first is already commented,
> and not worth worrying about; but the second can leave pages on the
> Unevictable list after an unusual sequence of swapping and locking.
>
> Fix that by using shmem_find_get_pages_and_swap() (then ignoring
> the swap) instead of pagevec_lookup().
>
> But I don't want to contaminate vmscan.c with shmem internals, nor
> shmem.c with LRU locking.  So move scan_mapping_unevictable_pages()
> into shmem.c, renaming it shmem_unlock_mapping(); and rename
> check_move_unevictable_page() to check_move_unevictable_pages(),
> looping down an array of pages, oftentimes under the same lock.
>
> Leave out the "rotate unevictable list" block: that's a leftover
> from when this was used for /proc/sys/vm/scan_unevictable_pages,
> whose flawed handling involved looking at pages at tail of LRU.
>
> Was there significance to the sequence first ClearPageUnevictable,
> then test page_evictable, then SetPageUnevictable here?  I think
> not, we're under LRU lock, and have no barriers between those.

If I understand correctly, this is not exactly correct. Because of,
PG_mlocked operation is not protected by LRU lock. So, I think we
have three choice.

1) check_move_unevictable_pages() aimed retry logic and put pages back
    into correct lru.
2) check_move_unevictable_pages() unconditionally move the pages into
    evictable lru, and vmacan put them back into correct lru later.
3) To protect PG_mlock operation by lru lock.


other parts looks fine to me.


  reply	other threads:[~2012-01-09 20:42 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-01-06 21:10 [PATCH 1/2] SHM_UNLOCK: fix long unpreemptible section Hugh Dickins
2012-01-06 21:10 ` Hugh Dickins
2012-01-06 21:12 ` [PATCH 2/2] SHM_UNLOCK: fix Unevictable pages stranded after swap Hugh Dickins
2012-01-06 21:12   ` Hugh Dickins
2012-01-09 20:42   ` KOSAKI Motohiro [this message]
2012-01-09 20:42     ` KOSAKI Motohiro
2012-01-09 22:25     ` Hugh Dickins
2012-01-09 22:25       ` Hugh Dickins
2012-01-09 23:09       ` KOSAKI Motohiro
2012-01-09 23:09         ` KOSAKI Motohiro
2012-01-09 23:56         ` Hugh Dickins
2012-01-09 23:56           ` Hugh Dickins
2012-01-15  0:20   ` Hugh Dickins
2012-01-15  0:20     ` Hugh Dickins
2012-01-07  8:28 ` [PATCH 1/2] SHM_UNLOCK: fix long unpreemptible section KOSAKI Motohiro
2012-01-07  8:28   ` KOSAKI Motohiro
2012-01-15  0:18 ` Hugh Dickins
2012-01-15  0:18   ` Hugh Dickins
2012-01-18 22:37   ` Andrew Morton
2012-01-18 22:37     ` Andrew Morton
2012-01-18 23:26     ` Hugh Dickins
2012-01-18 23:26       ` Hugh Dickins

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=4F0B5146.6090200@gmail.com \
    --to=kosaki.motohiro@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=eric.dumazet@gmail.com \
    --cc=hannes@cmpxchg.org \
    --cc=hughd@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=minchan.kim@gmail.com \
    --cc=riel@redhat.com \
    --cc=shaohua.li@intel.com \
    --cc=walken@google.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.