All of lore.kernel.org
 help / color / mirror / Atom feed
From: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
To: Hugh Dickins <hughd@google.com>
Cc: 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 18:09:32 -0500	[thread overview]
Message-ID: <4F0B73AC.7000504@gmail.com> (raw)
In-Reply-To: <alpine.LSU.2.00.1201091342300.1272@eggly.anvils>

(1/9/12 5:25 PM), Hugh Dickins wrote:
> On Mon, 9 Jan 2012, KOSAKI Motohiro wrote:
>> 2012/1/6 Hugh Dickins<hughd@google.com>:
>
> [ check_move_unevictable_page(s) ]
>
>>>
>>> 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,
>
> Thank you for giving it serious thought:
> such races are hard work to think about.
>
>> PG_mlocked operation is not protected by LRU lock. So, I think we
>
> Right.  But I don't see that I've made a significant change there.
>
> I may be being lazy, and rushing back to answer you, without giving
> constructive thought to what the precise race is that you see, and
> how we might fix it.  If the case you have in mind is easy for you
> to describe in detail, please do so; but don't hesitate to tell me
> to my own work for myself!

Bah! I was moron. I now think your code is right.

spin_lock(lru_lock)
if (page_evictable(page))
	blah blah blah
spin_unlock(lru_lock)

is always safe. Counter part should have following code and
waiting spin_lock(lru_lock) in isolate_lru_page().

                 if (!isolate_lru_page(page))
                         putback_lru_page(page);

then, even if check_move_unevictable_pages() observed wrong page status,
putback_lru_page() should put back the page into right lru.

I'm very sorry for annoying you.

	Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>



Probably, page_evictable() might be needed some additional comments. But
I have no idea what comment clearly explain this complex rule.....
  

--
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: 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 18:09:32 -0500	[thread overview]
Message-ID: <4F0B73AC.7000504@gmail.com> (raw)
In-Reply-To: <alpine.LSU.2.00.1201091342300.1272@eggly.anvils>

(1/9/12 5:25 PM), Hugh Dickins wrote:
> On Mon, 9 Jan 2012, KOSAKI Motohiro wrote:
>> 2012/1/6 Hugh Dickins<hughd@google.com>:
>
> [ check_move_unevictable_page(s) ]
>
>>>
>>> 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,
>
> Thank you for giving it serious thought:
> such races are hard work to think about.
>
>> PG_mlocked operation is not protected by LRU lock. So, I think we
>
> Right.  But I don't see that I've made a significant change there.
>
> I may be being lazy, and rushing back to answer you, without giving
> constructive thought to what the precise race is that you see, and
> how we might fix it.  If the case you have in mind is easy for you
> to describe in detail, please do so; but don't hesitate to tell me
> to my own work for myself!

Bah! I was moron. I now think your code is right.

spin_lock(lru_lock)
if (page_evictable(page))
	blah blah blah
spin_unlock(lru_lock)

is always safe. Counter part should have following code and
waiting spin_lock(lru_lock) in isolate_lru_page().

                 if (!isolate_lru_page(page))
                         putback_lru_page(page);

then, even if check_move_unevictable_pages() observed wrong page status,
putback_lru_page() should put back the page into right lru.

I'm very sorry for annoying you.

	Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>



Probably, page_evictable() might be needed some additional comments. But
I have no idea what comment clearly explain this complex rule.....
  

  reply	other threads:[~2012-01-09 23:09 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
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 [this message]
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=4F0B73AC.7000504@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.