* [PATCH] mm: remove worrying dead code from find_get_pages()
@ 2011-02-24 5:31 ` Hugh Dickins
0 siblings, 0 replies; 4+ messages in thread
From: Hugh Dickins @ 2011-02-24 5:31 UTC (permalink / raw)
To: Nick Piggin
Cc: Andrew Morton, Peter Zijlstra, Wu Fengguang, Salman Qazi,
linux-kernel, linux-mm
The radix_tree_deref_retry() case in find_get_pages() has a strange
little excrescence, not seen in the other gang lookups: it looks like
the start of an abandoned attempt to guarantee forward progress in a
case that cannot arise.
ret should always be 0 here: if it isn't, then going back to restart
will leak references to pages already gotten. There used to be a
comment saying nr_found is necessarily 1 here: that's not quite true,
but the radix_tree_deref_retry() case is peculiar to the entry at index
0, when we race with it being moved out of the radix_tree root or back.
Remove the worrisome two lines, add a brief comment here and in
find_get_pages_contig() and find_get_pages_tag(), and a WARN_ON
in find_get_pages() should it ever be seen elsewhere than at 0.
Signed-off-by: Hugh Dickins <hughd@google.com>
---
mm/filemap.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
--- 2.6.38-rc6/mm/filemap.c 2011-01-18 22:04:56.000000000 -0800
+++ linux/mm/filemap.c 2011-02-23 15:45:47.000000000 -0800
@@ -782,9 +782,13 @@ repeat:
page = radix_tree_deref_slot((void **)pages[i]);
if (unlikely(!page))
continue;
+
+ /*
+ * This can only trigger when the entry at index 0 moves out
+ * of or back to the root: none yet gotten, safe to restart.
+ */
if (radix_tree_deref_retry(page)) {
- if (ret)
- start = pages[ret-1]->index;
+ WARN_ON(start | i);
goto restart;
}
@@ -834,6 +838,11 @@ repeat:
page = radix_tree_deref_slot((void **)pages[i]);
if (unlikely(!page))
continue;
+
+ /*
+ * This can only trigger when the entry at index 0 moves out
+ * of or back to the root: none yet gotten, safe to restart.
+ */
if (radix_tree_deref_retry(page))
goto restart;
@@ -894,6 +903,11 @@ repeat:
page = radix_tree_deref_slot((void **)pages[i]);
if (unlikely(!page))
continue;
+
+ /*
+ * This can only trigger when the entry at index 0 moves out
+ * of or back to the root: none yet gotten, safe to restart.
+ */
if (radix_tree_deref_retry(page))
goto restart;
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] mm: remove worrying dead code from find_get_pages()
@ 2011-02-24 5:31 ` Hugh Dickins
0 siblings, 0 replies; 4+ messages in thread
From: Hugh Dickins @ 2011-02-24 5:31 UTC (permalink / raw)
To: Nick Piggin
Cc: Andrew Morton, Peter Zijlstra, Wu Fengguang, Salman Qazi,
linux-kernel, linux-mm
The radix_tree_deref_retry() case in find_get_pages() has a strange
little excrescence, not seen in the other gang lookups: it looks like
the start of an abandoned attempt to guarantee forward progress in a
case that cannot arise.
ret should always be 0 here: if it isn't, then going back to restart
will leak references to pages already gotten. There used to be a
comment saying nr_found is necessarily 1 here: that's not quite true,
but the radix_tree_deref_retry() case is peculiar to the entry at index
0, when we race with it being moved out of the radix_tree root or back.
Remove the worrisome two lines, add a brief comment here and in
find_get_pages_contig() and find_get_pages_tag(), and a WARN_ON
in find_get_pages() should it ever be seen elsewhere than at 0.
Signed-off-by: Hugh Dickins <hughd@google.com>
---
mm/filemap.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
--- 2.6.38-rc6/mm/filemap.c 2011-01-18 22:04:56.000000000 -0800
+++ linux/mm/filemap.c 2011-02-23 15:45:47.000000000 -0800
@@ -782,9 +782,13 @@ repeat:
page = radix_tree_deref_slot((void **)pages[i]);
if (unlikely(!page))
continue;
+
+ /*
+ * This can only trigger when the entry at index 0 moves out
+ * of or back to the root: none yet gotten, safe to restart.
+ */
if (radix_tree_deref_retry(page)) {
- if (ret)
- start = pages[ret-1]->index;
+ WARN_ON(start | i);
goto restart;
}
@@ -834,6 +838,11 @@ repeat:
page = radix_tree_deref_slot((void **)pages[i]);
if (unlikely(!page))
continue;
+
+ /*
+ * This can only trigger when the entry at index 0 moves out
+ * of or back to the root: none yet gotten, safe to restart.
+ */
if (radix_tree_deref_retry(page))
goto restart;
@@ -894,6 +903,11 @@ repeat:
page = radix_tree_deref_slot((void **)pages[i]);
if (unlikely(!page))
continue;
+
+ /*
+ * This can only trigger when the entry at index 0 moves out
+ * of or back to the root: none yet gotten, safe to restart.
+ */
if (radix_tree_deref_retry(page))
goto restart;
--
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mm: remove worrying dead code from find_get_pages()
2011-02-24 5:31 ` Hugh Dickins
@ 2011-02-24 12:38 ` Peter Zijlstra
-1 siblings, 0 replies; 4+ messages in thread
From: Peter Zijlstra @ 2011-02-24 12:38 UTC (permalink / raw)
To: Hugh Dickins
Cc: Nick Piggin, Andrew Morton, Wu Fengguang, Salman Qazi,
linux-kernel, linux-mm
On Wed, 2011-02-23 at 21:31 -0800, Hugh Dickins wrote:
> The radix_tree_deref_retry() case in find_get_pages() has a strange
> little excrescence, not seen in the other gang lookups: it looks like
> the start of an abandoned attempt to guarantee forward progress in a
> case that cannot arise.
>
> ret should always be 0 here: if it isn't, then going back to restart
> will leak references to pages already gotten. There used to be a
> comment saying nr_found is necessarily 1 here: that's not quite true,
> but the radix_tree_deref_retry() case is peculiar to the entry at index
> 0, when we race with it being moved out of the radix_tree root or back.
>
> Remove the worrisome two lines, add a brief comment here and in
> find_get_pages_contig() and find_get_pages_tag(), and a WARN_ON
> in find_get_pages() should it ever be seen elsewhere than at 0.
>
> Signed-off-by: Hugh Dickins <hughd@google.com>
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mm: remove worrying dead code from find_get_pages()
@ 2011-02-24 12:38 ` Peter Zijlstra
0 siblings, 0 replies; 4+ messages in thread
From: Peter Zijlstra @ 2011-02-24 12:38 UTC (permalink / raw)
To: Hugh Dickins
Cc: Nick Piggin, Andrew Morton, Wu Fengguang, Salman Qazi,
linux-kernel, linux-mm
On Wed, 2011-02-23 at 21:31 -0800, Hugh Dickins wrote:
> The radix_tree_deref_retry() case in find_get_pages() has a strange
> little excrescence, not seen in the other gang lookups: it looks like
> the start of an abandoned attempt to guarantee forward progress in a
> case that cannot arise.
>
> ret should always be 0 here: if it isn't, then going back to restart
> will leak references to pages already gotten. There used to be a
> comment saying nr_found is necessarily 1 here: that's not quite true,
> but the radix_tree_deref_retry() case is peculiar to the entry at index
> 0, when we race with it being moved out of the radix_tree root or back.
>
> Remove the worrisome two lines, add a brief comment here and in
> find_get_pages_contig() and find_get_pages_tag(), and a WARN_ON
> in find_get_pages() should it ever be seen elsewhere than at 0.
>
> Signed-off-by: Hugh Dickins <hughd@google.com>
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
--
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-02-24 12:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-24 5:31 [PATCH] mm: remove worrying dead code from find_get_pages() Hugh Dickins
2011-02-24 5:31 ` Hugh Dickins
2011-02-24 12:38 ` Peter Zijlstra
2011-02-24 12:38 ` Peter Zijlstra
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.