* [patch 2/4] mm: introduce page_lru_type()
2009-07-21 8:56 [patch 1/4] mm: drop unneeded double negations Johannes Weiner
@ 2009-07-21 8:56 ` Johannes Weiner
2009-07-22 1:52 ` Minchan Kim
2009-07-22 16:01 ` Rik van Riel
2009-07-21 8:56 ` [patch 3/4] mm: return boolean from page_is_file_cache() Johannes Weiner
` (2 subsequent siblings)
3 siblings, 2 replies; 18+ messages in thread
From: Johannes Weiner @ 2009-07-21 8:56 UTC (permalink / raw)
To: Andrew Morton; +Cc: KOSAKI Motohiro, linux-mm
Instead of abusing page_is_file_cache() for LRU list index arithmetic,
add another helper with a more appropriate name and convert the
non-boolean users of page_is_file_cache() accordingly.
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
include/linux/mm_inline.h | 19 +++++++++++++++++--
mm/swap.c | 4 ++--
mm/vmscan.c | 6 +++---
3 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
index 7fbb972..ec975f2 100644
--- a/include/linux/mm_inline.h
+++ b/include/linux/mm_inline.h
@@ -60,6 +60,21 @@ del_page_from_lru(struct zone *zone, struct page *page)
}
/**
+ * page_lru_type - which LRU list type should a page be on?
+ * @page: the page to test
+ *
+ * Used for LRU list index arithmetic.
+ *
+ * Returns the base LRU type - file or anon - @page should be on.
+ */
+static enum lru_list page_lru_type(struct page *page)
+{
+ if (page_is_file_cache(page))
+ return LRU_INACTIVE_FILE;
+ return LRU_INACTIVE_ANON;
+}
+
+/**
* page_lru - which LRU list should a page be on?
* @page: the page to test
*
@@ -68,14 +83,14 @@ del_page_from_lru(struct zone *zone, struct page *page)
*/
static inline enum lru_list page_lru(struct page *page)
{
- enum lru_list lru = LRU_BASE;
+ enum lru_list lru;
if (PageUnevictable(page))
lru = LRU_UNEVICTABLE;
else {
+ lru = page_lru_type(page);
if (PageActive(page))
lru += LRU_ACTIVE;
- lru += page_is_file_cache(page);
}
return lru;
diff --git a/mm/swap.c b/mm/swap.c
index cb29ae5..8f84638 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -118,7 +118,7 @@ static void pagevec_move_tail(struct pagevec *pvec)
spin_lock(&zone->lru_lock);
}
if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
- int lru = page_is_file_cache(page);
+ int lru = page_lru_type(page);
list_move_tail(&page->lru, &zone->lru[lru].list);
pgmoved++;
}
@@ -181,7 +181,7 @@ void activate_page(struct page *page)
spin_lock_irq(&zone->lru_lock);
if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
int file = page_is_file_cache(page);
- int lru = LRU_BASE + file;
+ int lru = page_lru_type(page);
del_page_from_lru_list(zone, page, lru);
SetPageActive(page);
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 46ec6a5..758f628 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -531,7 +531,7 @@ redo:
* unevictable page on [in]active list.
* We know how to handle that.
*/
- lru = active + page_is_file_cache(page);
+ lru = active + page_lru_type(page);
lru_cache_add_lru(page, lru);
} else {
/*
@@ -981,7 +981,7 @@ static unsigned long clear_active_flags(struct list_head *page_list,
struct page *page;
list_for_each_entry(page, page_list, lru) {
- lru = page_is_file_cache(page);
+ lru = page_lru_type(page);
if (PageActive(page)) {
lru += LRU_ACTIVE;
ClearPageActive(page);
@@ -2645,7 +2645,7 @@ static void check_move_unevictable_page(struct page *page, struct zone *zone)
retry:
ClearPageUnevictable(page);
if (page_evictable(page, NULL)) {
- enum lru_list l = LRU_INACTIVE_ANON + page_is_file_cache(page);
+ enum lru_list l = page_lru_type(page);
__dec_zone_state(zone, NR_UNEVICTABLE);
list_move(&page->lru, &zone->lru[l].list);
--
1.6.3
--
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>
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [patch 2/4] mm: introduce page_lru_type()
2009-07-21 8:56 ` [patch 2/4] mm: introduce page_lru_type() Johannes Weiner
@ 2009-07-22 1:52 ` Minchan Kim
2009-07-22 9:07 ` Johannes Weiner
2009-07-22 16:01 ` Rik van Riel
1 sibling, 1 reply; 18+ messages in thread
From: Minchan Kim @ 2009-07-22 1:52 UTC (permalink / raw)
To: Johannes Weiner; +Cc: Andrew Morton, KOSAKI Motohiro, linux-mm
Hi.
On Tue, Jul 21, 2009 at 5:56 PM, Johannes Weiner<hannes@cmpxchg.org> wrote:
> Instead of abusing page_is_file_cache() for LRU list index arithmetic,
> add another helper with a more appropriate name and convert the
> non-boolean users of page_is_file_cache() accordingly.
>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
> include/linux/mm_inline.h | 19 +++++++++++++++++--
> mm/swap.c | 4 ++--
> mm/vmscan.c | 6 +++---
> 3 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
> index 7fbb972..ec975f2 100644
> --- a/include/linux/mm_inline.h
> +++ b/include/linux/mm_inline.h
> @@ -60,6 +60,21 @@ del_page_from_lru(struct zone *zone, struct page *page)
> }
>
> /**
> + * page_lru_type - which LRU list type should a page be on?
> + * @page: the page to test
> + *
> + * Used for LRU list index arithmetic.
> + *
> + * Returns the base LRU type - file or anon - @page should be on.
> + */
> +static enum lru_list page_lru_type(struct page *page)
> +{
> + if (page_is_file_cache(page))
> + return LRU_INACTIVE_FILE;
> + return LRU_INACTIVE_ANON;
> +}
page_lru_type function's semantics is general but this function only
considers INACTIVE case.
So we always have to check PageActive to know exact lru type.
Why do we need double check(ex, page_lru_type and PageActive) to know
exact lru type ?
It wouldn't be better to check it all at once ?
> +
> +/**
> * page_lru - which LRU list should a page be on?
> * @page: the page to test
> *
> @@ -68,14 +83,14 @@ del_page_from_lru(struct zone *zone, struct page *page)
> */
> static inline enum lru_list page_lru(struct page *page)
> {
> - enum lru_list lru = LRU_BASE;
> + enum lru_list lru;
>
> if (PageUnevictable(page))
> lru = LRU_UNEVICTABLE;
> else {
> + lru = page_lru_type(page);
> if (PageActive(page))
> lru += LRU_ACTIVE;
> - lru += page_is_file_cache(page);
> }
>
> return lru;
> diff --git a/mm/swap.c b/mm/swap.c
> index cb29ae5..8f84638 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -118,7 +118,7 @@ static void pagevec_move_tail(struct pagevec *pvec)
> spin_lock(&zone->lru_lock);
> }
> if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
> - int lru = page_is_file_cache(page);
> + int lru = page_lru_type(page);
> list_move_tail(&page->lru, &zone->lru[lru].list);
> pgmoved++;
> }
> @@ -181,7 +181,7 @@ void activate_page(struct page *page)
> spin_lock_irq(&zone->lru_lock);
> if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
> int file = page_is_file_cache(page);
> - int lru = LRU_BASE + file;
> + int lru = page_lru_type(page);
> del_page_from_lru_list(zone, page, lru);
>
> SetPageActive(page);
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 46ec6a5..758f628 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -531,7 +531,7 @@ redo:
> * unevictable page on [in]active list.
> * We know how to handle that.
> */
> - lru = active + page_is_file_cache(page);
> + lru = active + page_lru_type(page);
> lru_cache_add_lru(page, lru);
> } else {
> /*
> @@ -981,7 +981,7 @@ static unsigned long clear_active_flags(struct list_head *page_list,
> struct page *page;
>
> list_for_each_entry(page, page_list, lru) {
> - lru = page_is_file_cache(page);
> + lru = page_lru_type(page);
> if (PageActive(page)) {
> lru += LRU_ACTIVE;
> ClearPageActive(page);
> @@ -2645,7 +2645,7 @@ static void check_move_unevictable_page(struct page *page, struct zone *zone)
> retry:
> ClearPageUnevictable(page);
> if (page_evictable(page, NULL)) {
> - enum lru_list l = LRU_INACTIVE_ANON + page_is_file_cache(page);
> + enum lru_list l = page_lru_type(page);
>
> __dec_zone_state(zone, NR_UNEVICTABLE);
> list_move(&page->lru, &zone->lru[l].list);
> --
> 1.6.3
>
> --
> 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>
>
--
Kind regards,
Minchan Kim
--
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>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [patch 2/4] mm: introduce page_lru_type()
2009-07-22 1:52 ` Minchan Kim
@ 2009-07-22 9:07 ` Johannes Weiner
2009-07-22 12:37 ` Minchan Kim
0 siblings, 1 reply; 18+ messages in thread
From: Johannes Weiner @ 2009-07-22 9:07 UTC (permalink / raw)
To: Minchan Kim; +Cc: Andrew Morton, KOSAKI Motohiro, linux-mm
Hello Minchan,
On Wed, Jul 22, 2009 at 10:52:21AM +0900, Minchan Kim wrote:
> Hi.
>
> On Tue, Jul 21, 2009 at 5:56 PM, Johannes Weiner<hannes@cmpxchg.org> wrote:
> > Instead of abusing page_is_file_cache() for LRU list index arithmetic,
> > add another helper with a more appropriate name and convert the
> > non-boolean users of page_is_file_cache() accordingly.
> >
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> > ---
> > A include/linux/mm_inline.h | A 19 +++++++++++++++++--
> > A mm/swap.c A A A A A A A A | A A 4 ++--
> > A mm/vmscan.c A A A A A A A | A A 6 +++---
> > A 3 files changed, 22 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
> > index 7fbb972..ec975f2 100644
> > --- a/include/linux/mm_inline.h
> > +++ b/include/linux/mm_inline.h
> > @@ -60,6 +60,21 @@ del_page_from_lru(struct zone *zone, struct page *page)
> > A }
> >
> > A /**
> > + * page_lru_type - which LRU list type should a page be on?
> > + * @page: the page to test
> > + *
> > + * Used for LRU list index arithmetic.
> > + *
> > + * Returns the base LRU type - file or anon - @page should be on.
> > + */
> > +static enum lru_list page_lru_type(struct page *page)
> > +{
> > + A A A if (page_is_file_cache(page))
> > + A A A A A A A return LRU_INACTIVE_FILE;
> > + A A A return LRU_INACTIVE_ANON;
> > +}
>
> page_lru_type function's semantics is general but this function only
> considers INACTIVE case.
> So we always have to check PageActive to know exact lru type.
>
> Why do we need double check(ex, page_lru_type and PageActive) to know
> exact lru type ?
>
> It wouldn't be better to check it all at once ?
page_lru() does that for you already.
But look at the users of page_lru_type(), they know the active bit
when they want to find out the base type, see check_move_unevictable
e.g.
Hannes
--
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>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [patch 2/4] mm: introduce page_lru_type()
2009-07-22 9:07 ` Johannes Weiner
@ 2009-07-22 12:37 ` Minchan Kim
0 siblings, 0 replies; 18+ messages in thread
From: Minchan Kim @ 2009-07-22 12:37 UTC (permalink / raw)
To: Johannes Weiner; +Cc: Andrew Morton, KOSAKI Motohiro, linux-mm
On Wed, Jul 22, 2009 at 6:07 PM, Johannes Weiner<hannes@cmpxchg.org> wrote:
> Hello Minchan,
>
> On Wed, Jul 22, 2009 at 10:52:21AM +0900, Minchan Kim wrote:
>> Hi.
>>
>> On Tue, Jul 21, 2009 at 5:56 PM, Johannes Weiner<hannes@cmpxchg.org> wrote:
>> > Instead of abusing page_is_file_cache() for LRU list index arithmetic,
>> > add another helper with a more appropriate name and convert the
>> > non-boolean users of page_is_file_cache() accordingly.
>> >
>> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
>> > ---
>> > include/linux/mm_inline.h | 19 +++++++++++++++++--
>> > mm/swap.c | 4 ++--
>> > mm/vmscan.c | 6 +++---
>> > 3 files changed, 22 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
>> > index 7fbb972..ec975f2 100644
>> > --- a/include/linux/mm_inline.h
>> > +++ b/include/linux/mm_inline.h
>> > @@ -60,6 +60,21 @@ del_page_from_lru(struct zone *zone, struct page *page)
>> > }
>> >
>> > /**
>> > + * page_lru_type - which LRU list type should a page be on?
>> > + * @page: the page to test
>> > + *
>> > + * Used for LRU list index arithmetic.
>> > + *
>> > + * Returns the base LRU type - file or anon - @page should be on.
>> > + */
>> > +static enum lru_list page_lru_type(struct page *page)
>> > +{
>> > + if (page_is_file_cache(page))
>> > + return LRU_INACTIVE_FILE;
>> > + return LRU_INACTIVE_ANON;
>> > +}
>>
>> page_lru_type function's semantics is general but this function only
>> considers INACTIVE case.
>> So we always have to check PageActive to know exact lru type.
>>
>> Why do we need double check(ex, page_lru_type and PageActive) to know
>> exact lru type ?
>>
>> It wouldn't be better to check it all at once ?
>
> page_lru() does that for you already.
>
> But look at the users of page_lru_type(), they know the active bit
> when they want to find out the base type, see check_move_unevictable
> e.g.
Yes. You already mentioned proper function name. :)
How about changing function name from page_lru_type to page_lru_base_type ?
it might be a nitpick. :)
--
Kind regards,
Minchan Kim
--
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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [patch 2/4] mm: introduce page_lru_type()
2009-07-21 8:56 ` [patch 2/4] mm: introduce page_lru_type() Johannes Weiner
2009-07-22 1:52 ` Minchan Kim
@ 2009-07-22 16:01 ` Rik van Riel
1 sibling, 0 replies; 18+ messages in thread
From: Rik van Riel @ 2009-07-22 16:01 UTC (permalink / raw)
To: Johannes Weiner; +Cc: Andrew Morton, KOSAKI Motohiro, linux-mm
Johannes Weiner wrote:
> Instead of abusing page_is_file_cache() for LRU list index arithmetic,
> add another helper with a more appropriate name and convert the
> non-boolean users of page_is_file_cache() accordingly.
>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Acked-by: Rik van Riel <riel@redhat.com>
--
All rights reversed.
--
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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [patch 3/4] mm: return boolean from page_is_file_cache()
2009-07-21 8:56 [patch 1/4] mm: drop unneeded double negations Johannes Weiner
2009-07-21 8:56 ` [patch 2/4] mm: introduce page_lru_type() Johannes Weiner
@ 2009-07-21 8:56 ` Johannes Weiner
2009-07-21 8:56 ` [patch 4/4] mm: return boolean from page_has_private() Johannes Weiner
2009-07-21 9:33 ` [patch 1/4] mm: drop unneeded double negations Mel Gorman
3 siblings, 0 replies; 18+ messages in thread
From: Johannes Weiner @ 2009-07-21 8:56 UTC (permalink / raw)
To: Andrew Morton; +Cc: KOSAKI Motohiro, linux-mm
page_is_file_cache() has been used for both boolean checks and LRU
arithmetic, which was always a bit weird.
Now that page_lru_type() exists for LRU arithmetic, make
page_is_file_cache() a real predicate function and adjust the
boolean-using callsites to drop those pesky double negations.
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
include/linux/mm_inline.h | 8 ++------
mm/migrate.c | 6 +++---
mm/swap.c | 2 +-
mm/vmscan.c | 2 +-
4 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
index ec975f2..54edae1 100644
--- a/include/linux/mm_inline.h
+++ b/include/linux/mm_inline.h
@@ -5,7 +5,7 @@
* page_is_file_cache - should the page be on a file LRU or anon LRU?
* @page: the page to test
*
- * Returns LRU_FILE if @page is page cache page backed by a regular filesystem,
+ * Returns 1 if @page is page cache page backed by a regular filesystem,
* or 0 if @page is anonymous, tmpfs or otherwise ram or swap backed.
* Used by functions that manipulate the LRU lists, to sort a page
* onto the right LRU list.
@@ -16,11 +16,7 @@
*/
static inline int page_is_file_cache(struct page *page)
{
- if (PageSwapBacked(page))
- return 0;
-
- /* The page is page cache backed by a normal filesystem. */
- return LRU_FILE;
+ return !PageSwapBacked(page);
}
static inline void
diff --git a/mm/migrate.c b/mm/migrate.c
index b535a2c..e97e513 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -68,7 +68,7 @@ int putback_lru_pages(struct list_head *l)
list_for_each_entry_safe(page, page2, l, lru) {
list_del(&page->lru);
dec_zone_page_state(page, NR_ISOLATED_ANON +
- !!page_is_file_cache(page));
+ page_is_file_cache(page));
putback_lru_page(page);
count++;
}
@@ -701,7 +701,7 @@ unlock:
*/
list_del(&page->lru);
dec_zone_page_state(page, NR_ISOLATED_ANON +
- !!page_is_file_cache(page));
+ page_is_file_cache(page));
putback_lru_page(page);
}
@@ -751,7 +751,7 @@ int migrate_pages(struct list_head *from,
local_irq_save(flags);
list_for_each_entry(page, from, lru)
__inc_zone_page_state(page, NR_ISOLATED_ANON +
- !!page_is_file_cache(page));
+ page_is_file_cache(page));
local_irq_restore(flags);
if (!swapwrite)
diff --git a/mm/swap.c b/mm/swap.c
index 8f84638..230589c 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -189,7 +189,7 @@ void activate_page(struct page *page)
add_page_to_lru_list(zone, page, lru);
__count_vm_event(PGACTIVATE);
- update_page_reclaim_stat(zone, page, !!file, 1);
+ update_page_reclaim_stat(zone, page, file, 1);
}
spin_unlock_irq(&zone->lru_lock);
}
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 758f628..6b368d3 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -816,7 +816,7 @@ int __isolate_lru_page(struct page *page, int mode, int file)
if (mode != ISOLATE_BOTH && (!PageActive(page) != !mode))
return ret;
- if (mode != ISOLATE_BOTH && (!page_is_file_cache(page) != !file))
+ if (mode != ISOLATE_BOTH && page_is_file_cache(page) != file)
return ret;
/*
--
1.6.3
--
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>
^ permalink raw reply related [flat|nested] 18+ messages in thread* [patch 4/4] mm: return boolean from page_has_private()
2009-07-21 8:56 [patch 1/4] mm: drop unneeded double negations Johannes Weiner
2009-07-21 8:56 ` [patch 2/4] mm: introduce page_lru_type() Johannes Weiner
2009-07-21 8:56 ` [patch 3/4] mm: return boolean from page_is_file_cache() Johannes Weiner
@ 2009-07-21 8:56 ` Johannes Weiner
2009-07-22 16:49 ` Christoph Lameter
2009-07-21 9:33 ` [patch 1/4] mm: drop unneeded double negations Mel Gorman
3 siblings, 1 reply; 18+ messages in thread
From: Johannes Weiner @ 2009-07-21 8:56 UTC (permalink / raw)
To: Andrew Morton; +Cc: KOSAKI Motohiro, linux-mm
Make page_has_private() return a true boolean value and remove the
double negations from the two callsites using it for arithmetic.
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
include/linux/page-flags.h | 11 ++++++-----
mm/migrate.c | 2 +-
mm/vmscan.c | 2 +-
3 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index e2e5ce5..17119bd 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -396,8 +396,6 @@ static inline void __ClearPageTail(struct page *page)
*/
#define PAGE_FLAGS_CHECK_AT_PREP ((1 << NR_PAGEFLAGS) - 1)
-#endif /* !__GENERATING_BOUNDS_H */
-
/**
* page_has_private - Determine if page has private stuff
* @page: The page to be checked
@@ -405,8 +403,11 @@ static inline void __ClearPageTail(struct page *page)
* Determine if a page has private stuff, indicating that release routines
* should be invoked upon it.
*/
-#define page_has_private(page) \
- ((page)->flags & ((1 << PG_private) | \
- (1 << PG_private_2)))
+static inline int page_has_private(struct page *page)
+{
+ return !!(page->flags & ((1 << PG_private) | (1 << PG_private_2)));
+}
+
+#endif /* !__GENERATING_BOUNDS_H */
#endif /* PAGE_FLAGS_H */
diff --git a/mm/migrate.c b/mm/migrate.c
index e97e513..16052e8 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -272,7 +272,7 @@ static int migrate_page_move_mapping(struct address_space *mapping,
pslot = radix_tree_lookup_slot(&mapping->page_tree,
page_index(page));
- expected_count = 2 + !!page_has_private(page);
+ expected_count = 2 + page_has_private(page);
if (page_count(page) != expected_count ||
(struct page *)radix_tree_deref_slot(pslot) != page) {
spin_unlock_irq(&mapping->tree_lock);
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 6b368d3..67e2824 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -286,7 +286,7 @@ static inline int page_mapping_inuse(struct page *page)
static inline int is_page_cache_freeable(struct page *page)
{
- return page_count(page) - !!page_has_private(page) == 2;
+ return page_count(page) - page_has_private(page) == 2;
}
static int may_write_to_queue(struct backing_dev_info *bdi)
--
1.6.3
--
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>
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [patch 4/4] mm: return boolean from page_has_private()
2009-07-21 8:56 ` [patch 4/4] mm: return boolean from page_has_private() Johannes Weiner
@ 2009-07-22 16:49 ` Christoph Lameter
2009-07-22 17:50 ` Johannes Weiner
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Lameter @ 2009-07-22 16:49 UTC (permalink / raw)
To: Johannes Weiner; +Cc: Andrew Morton, KOSAKI Motohiro, linux-mm
On Tue, 21 Jul 2009, Johannes Weiner wrote:
> Make page_has_private() return a true boolean value and remove the
> double negations from the two callsites using it for arithmetic.
page_has_private_data()?
Also note that you are adding unecessary double negation to the other
callers. Does the compiler catch that?
> +static inline int page_has_private(struct page *page)
> +{
> + return !!(page->flags & ((1 << PG_private) | (1 << PG_private_2)));
> +}
Two private bits? How did that happen?
Could we define a PAGE_FLAGS_PRIVATE in page-flags.h?
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 6b368d3..67e2824 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -286,7 +286,7 @@ static inline int page_mapping_inuse(struct page *page)
>
> static inline int is_page_cache_freeable(struct page *page)
> {
> - return page_count(page) - !!page_has_private(page) == 2;
> + return page_count(page) - page_has_private(page) == 2;
That looks funky and in need of comments.
--
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>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [patch 4/4] mm: return boolean from page_has_private()
2009-07-22 16:49 ` Christoph Lameter
@ 2009-07-22 17:50 ` Johannes Weiner
2009-07-22 17:54 ` [patch 5/4] mm: document is_page_cache_freeable() Johannes Weiner
0 siblings, 1 reply; 18+ messages in thread
From: Johannes Weiner @ 2009-07-22 17:50 UTC (permalink / raw)
To: Christoph Lameter; +Cc: Andrew Morton, KOSAKI Motohiro, linux-mm
On Wed, Jul 22, 2009 at 12:49:44PM -0400, Christoph Lameter wrote:
> On Tue, 21 Jul 2009, Johannes Weiner wrote:
>
> > Make page_has_private() return a true boolean value and remove the
> > double negations from the two callsites using it for arithmetic.
>
> page_has_private_data()?
I am not so fond of changing that, because then we should probably
also rename page_private() and that is moot for slightly improved
source code English.
> Also note that you are adding unecessary double negation to the other
> callers. Does the compiler catch that?
Yes, callsites using it in a conditionals do not change here with gcc
4.3.3.
> > +static inline int page_has_private(struct page *page)
> > +{
> > + return !!(page->flags & ((1 << PG_private) | (1 << PG_private_2)));
> > +}
>
> Two private bits? How did that happen?
fscache :)
> Could we define a PAGE_FLAGS_PRIVATE in page-flags.h?
It would certainly look nicer, I will add that.
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 6b368d3..67e2824 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -286,7 +286,7 @@ static inline int page_mapping_inuse(struct page *page)
> >
> > static inline int is_page_cache_freeable(struct page *page)
> > {
> > - return page_count(page) - !!page_has_private(page) == 2;
> > + return page_count(page) - page_has_private(page) == 2;
>
> That looks funky and in need of comments.
Agreed, I will add one in a different patch.
Hannes
---
^ permalink raw reply [flat|nested] 18+ messages in thread* [patch 5/4] mm: document is_page_cache_freeable()
2009-07-22 17:50 ` Johannes Weiner
@ 2009-07-22 17:54 ` Johannes Weiner
2009-07-22 19:02 ` Christoph Lameter
0 siblings, 1 reply; 18+ messages in thread
From: Johannes Weiner @ 2009-07-22 17:54 UTC (permalink / raw)
To: Christoph Lameter; +Cc: Andrew Morton, KOSAKI Motohiro, linux-mm
On Wed, Jul 22, 2009 at 07:50:31PM +0200, Johannes Weiner wrote:
> On Wed, Jul 22, 2009 at 12:49:44PM -0400, Christoph Lameter wrote:
> > > static inline int is_page_cache_freeable(struct page *page)
> > > {
> > > - return page_count(page) - !!page_has_private(page) == 2;
> > > + return page_count(page) - page_has_private(page) == 2;
> >
> > That looks funky and in need of comments.
Enlighten the reader of this code about what reference count makes a
page cache page freeable.
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
mm/vmscan.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 67e2824..d18f46d 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -286,6 +286,11 @@ static inline int page_mapping_inuse(struct page *page)
static inline int is_page_cache_freeable(struct page *page)
{
+ /*
+ * A freeable page cache page is referenced only by the caller
+ * that isolated the page, the page cache itself and
+ * optionally the page's buffers, if any.
+ */
return page_count(page) - page_has_private(page) == 2;
}
--
1.6.3
--
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>
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [patch 5/4] mm: document is_page_cache_freeable()
2009-07-22 17:54 ` [patch 5/4] mm: document is_page_cache_freeable() Johannes Weiner
@ 2009-07-22 19:02 ` Christoph Lameter
2009-07-22 21:55 ` Li, Ming Chun
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Lameter @ 2009-07-22 19:02 UTC (permalink / raw)
To: Johannes Weiner; +Cc: Andrew Morton, KOSAKI Motohiro, linux-mm
> static inline int is_page_cache_freeable(struct page *page)
> {
> + /*
> + * A freeable page cache page is referenced only by the caller
> + * that isolated the page, the page cache itself and
The page cache "itself"? This is the radix tree reference right?
--
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>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [patch 5/4] mm: document is_page_cache_freeable()
2009-07-22 19:02 ` Christoph Lameter
@ 2009-07-22 21:55 ` Li, Ming Chun
2009-07-22 22:10 ` Johannes Weiner
0 siblings, 1 reply; 18+ messages in thread
From: Li, Ming Chun @ 2009-07-22 21:55 UTC (permalink / raw)
To: Christoph Lameter
Cc: Johannes Weiner, Andrew Morton, KOSAKI Motohiro, linux-mm
On Wed, 22 Jul 2009, Christoph Lameter wrote:
>
> > static inline int is_page_cache_freeable(struct page *page)
> > {
> > + /*
> > + * A freeable page cache page is referenced only by the caller
> > + * that isolated the page, the page cache itself and
>
> The page cache "itself"? This is the radix tree reference right?
>
I think you are right. I had trouble understanding this function, So I
looked into it and found out the call path:
add_to_page_cache_locked
-> page_cache_get
-> atomic_inc(&page->_count)
Please correct me if I am wrong.
Vincent Li
Biomedical Research Center
University of British Columbia
--
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>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [patch 5/4] mm: document is_page_cache_freeable()
2009-07-22 21:55 ` Li, Ming Chun
@ 2009-07-22 22:10 ` Johannes Weiner
2009-07-22 23:51 ` Li, Ming Chun
2009-07-22 23:58 ` Christoph Lameter
0 siblings, 2 replies; 18+ messages in thread
From: Johannes Weiner @ 2009-07-22 22:10 UTC (permalink / raw)
To: Li, Ming Chun; +Cc: Christoph Lameter, Andrew Morton, KOSAKI Motohiro, linux-mm
On Wed, Jul 22, 2009 at 02:55:12PM -0700, Li, Ming Chun wrote:
> On Wed, 22 Jul 2009, Christoph Lameter wrote:
>
> >
> > > static inline int is_page_cache_freeable(struct page *page)
> > > {
> > > + /*
> > > + * A freeable page cache page is referenced only by the caller
> > > + * that isolated the page, the page cache itself and
> >
> > The page cache "itself"? This is the radix tree reference right?
> >
>
> I think you are right. I had trouble understanding this function, So I
> looked into it and found out the call path:
>
> add_to_page_cache_locked
> -> page_cache_get
> -> atomic_inc(&page->_count)
>
> Please correct me if I am wrong.
This is correct. But this is the purpose of reference counters - you
increase it when you reference the object so that it doesn't get freed
under you.
That's why everybody holding a reference to the page must have its
usage counter increased. And this includes the page/swap cache, the
LRU lists, the page tables etc.
And I think in that context my comment should be obvious. Do you need
to know that the page cache is actually managed with radix trees at
this point?
You need to know that the page cache is something holding a reference
to the page so you can meet the requirements that are written above
remove_from_page_cache() - which you are about to call.
I added the comment to document that magic `compare with 2' in there.
If more is needed, I am glad to help - but right now I don't really
think I know what the issue is with this patch?
--
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>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [patch 5/4] mm: document is_page_cache_freeable()
2009-07-22 22:10 ` Johannes Weiner
@ 2009-07-22 23:51 ` Li, Ming Chun
2009-07-22 23:58 ` Christoph Lameter
1 sibling, 0 replies; 18+ messages in thread
From: Li, Ming Chun @ 2009-07-22 23:51 UTC (permalink / raw)
To: Johannes Weiner
Cc: Li, Ming Chun, Christoph Lameter, Andrew Morton, KOSAKI Motohiro,
linux-mm
On Thu, 23 Jul 2009, Johannes Weiner wrote:
> On Wed, Jul 22, 2009 at 02:55:12PM -0700, Li, Ming Chun wrote:
> > On Wed, 22 Jul 2009, Christoph Lameter wrote:
> >
> > >
> > > > static inline int is_page_cache_freeable(struct page *page)
> > > > {
> > > > + /*
> > > > + * A freeable page cache page is referenced only by the caller
> > > > + * that isolated the page, the page cache itself and
> > >
> > > The page cache "itself"? This is the radix tree reference right?
> > >
> >
> > I think you are right. I had trouble understanding this function, So I
> > looked into it and found out the call path:
> >
> > add_to_page_cache_locked
> > -> page_cache_get
> > -> atomic_inc(&page->_count)
> >
> > Please correct me if I am wrong.
>
> This is correct. But this is the purpose of reference counters - you
> increase it when you reference the object so that it doesn't get freed
> under you.
>
> That's why everybody holding a reference to the page must have its
> usage counter increased. And this includes the page/swap cache, the
> LRU lists, the page tables etc.
>
> And I think in that context my comment should be obvious. Do you need
> to know that the page cache is actually managed with radix trees at
> this point?
>
> You need to know that the page cache is something holding a reference
> to the page so you can meet the requirements that are written above
> remove_from_page_cache() - which you are about to call.
>
> I added the comment to document that magic `compare with 2' in there.
> If more is needed, I am glad to help - but right now I don't really
> think I know what the issue is with this patch?
>
>
No issue at all for me :), I should clarify that I had problem
understanding it before you post your comment patch, Your comment acutally
helped me to undertand it better and thanks for the comments. I am just learning from you by
lurking in mailing list and reading the code to understand what you comments.
Vincent Li
Biomedical Research Center
University of British Columbia
--
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>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [patch 5/4] mm: document is_page_cache_freeable()
2009-07-22 22:10 ` Johannes Weiner
2009-07-22 23:51 ` Li, Ming Chun
@ 2009-07-22 23:58 ` Christoph Lameter
1 sibling, 0 replies; 18+ messages in thread
From: Christoph Lameter @ 2009-07-22 23:58 UTC (permalink / raw)
To: Johannes Weiner; +Cc: Li, Ming Chun, Andrew Morton, KOSAKI Motohiro, linux-mm
On Thu, 23 Jul 2009, Johannes Weiner wrote:
> And I think in that context my comment should be obvious. Do you need
> to know that the page cache is actually managed with radix trees at
> this point?
Its good to know where the pointer is that accounts for the
refcount. "pagecache" is a nebulous term.
--
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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [patch 1/4] mm: drop unneeded double negations
2009-07-21 8:56 [patch 1/4] mm: drop unneeded double negations Johannes Weiner
` (2 preceding siblings ...)
2009-07-21 8:56 ` [patch 4/4] mm: return boolean from page_has_private() Johannes Weiner
@ 2009-07-21 9:33 ` Mel Gorman
2009-07-21 11:18 ` Johannes Weiner
3 siblings, 1 reply; 18+ messages in thread
From: Mel Gorman @ 2009-07-21 9:33 UTC (permalink / raw)
To: Johannes Weiner; +Cc: Andrew Morton, KOSAKI Motohiro, linux-mm
On Tue, Jul 21, 2009 at 10:56:31AM +0200, Johannes Weiner wrote:
> Remove double negations where the operand is already boolean.
>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
> mm/memcontrol.c | 2 +-
> mm/memory.c | 2 +-
> mm/vmscan.c | 10 +++++-----
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 38ad840..8ad148a 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -655,7 +655,7 @@ unsigned long mem_cgroup_isolate_pages(unsigned long nr_to_scan,
> int nid = z->zone_pgdat->node_id;
> int zid = zone_idx(z);
> struct mem_cgroup_per_zone *mz;
> - int lru = LRU_FILE * !!file + !!active;
> + int lru = LRU_FILE * file + active;
> int ret;
>
Ok, this should be ok as file and active appear to be 1 and 0.
> BUG_ON(!mem_cont);
> diff --git a/mm/memory.c b/mm/memory.c
> index 6521619..dd8eb26 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -596,7 +596,7 @@ copy_one_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
> if (page) {
> get_page(page);
> page_dup_rmap(page, vma, addr);
> - rss[!!PageAnon(page)]++;
> + rss[PageAnon(page)]++;
> }
Similarly, seems ok.
>
> out_set_pte:
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 07fd8aa..46ec6a5 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -516,7 +516,7 @@ int remove_mapping(struct address_space *mapping, struct page *page)
> void putback_lru_page(struct page *page)
> {
> int lru;
> - int active = !!TestClearPageActive(page);
> + int active = TestClearPageActive(page);
> int was_unevictable = PageUnevictable(page);
>
But are you *sure* about this change?
active it used as an array offset later in this function for evictable pages
so it needs to be 1 or 0 but IIRC, the TestClear functions are not guaranteed
to return 0 or 1 on all architectures. They return 0 or non-zero. I'm 99.999%
certain I've been bitten before by test_bit returning the word with the one
bit set instead of 1. Maybe things have changed since or it's my
imagination but can you double check please?
> VM_BUG_ON(PageLRU(page));
> @@ -966,7 +966,7 @@ static unsigned long isolate_pages_global(unsigned long nr,
> if (file)
> lru += LRU_FILE;
> return isolate_lru_pages(nr, &z->lru[lru].list, dst, scanned, order,
> - mode, !!file);
> + mode, file);
> }
>
> /*
> @@ -1204,7 +1204,7 @@ static unsigned long shrink_inactive_list(unsigned long max_scan,
> lru = page_lru(page);
> add_page_to_lru_list(zone, page, lru);
> if (is_active_lru(lru)) {
> - int file = !!is_file_lru(lru);
> + int file = is_file_lru(lru);
> reclaim_stat->recent_rotated[file]++;
> }
> if (!pagevec_add(&pvec, page)) {
> @@ -1310,7 +1310,7 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
> if (scanning_global_lru(sc)) {
> zone->pages_scanned += pgscanned;
> }
> - reclaim_stat->recent_scanned[!!file] += nr_taken;
> + reclaim_stat->recent_scanned[file] += nr_taken;
>
> __count_zone_vm_events(PGREFILL, zone, pgscanned);
> if (file)
> @@ -1364,7 +1364,7 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
> * helps balance scan pressure between file and anonymous pages in
> * get_scan_ratio.
> */
> - reclaim_stat->recent_rotated[!!file] += nr_rotated;
> + reclaim_stat->recent_rotated[file] += nr_rotated;
> __count_vm_events(PGDEACTIVATE, nr_deactivate);
>
> move_active_pages_to_lru(zone, &l_active,
> --
> 1.6.3
>
> --
> 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>
>
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
--
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>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [patch 1/4] mm: drop unneeded double negations
2009-07-21 9:33 ` [patch 1/4] mm: drop unneeded double negations Mel Gorman
@ 2009-07-21 11:18 ` Johannes Weiner
0 siblings, 0 replies; 18+ messages in thread
From: Johannes Weiner @ 2009-07-21 11:18 UTC (permalink / raw)
To: Mel Gorman; +Cc: Andrew Morton, KOSAKI Motohiro, linux-mm
On Tue, Jul 21, 2009 at 10:33:13AM +0100, Mel Gorman wrote:
> On Tue, Jul 21, 2009 at 10:56:31AM +0200, Johannes Weiner wrote:
> > out_set_pte:
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 07fd8aa..46ec6a5 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -516,7 +516,7 @@ int remove_mapping(struct address_space *mapping, struct page *page)
> > void putback_lru_page(struct page *page)
> > {
> > int lru;
> > - int active = !!TestClearPageActive(page);
> > + int active = TestClearPageActive(page);
> > int was_unevictable = PageUnevictable(page);
> >
>
> But are you *sure* about this change?
>
> active it used as an array offset later in this function for evictable pages
> so it needs to be 1 or 0 but IIRC, the TestClear functions are not guaranteed
> to return 0 or 1 on all architectures. They return 0 or non-zero. I'm 99.999%
> certain I've been bitten before by test_bit returning the word with the one
> bit set instead of 1. Maybe things have changed since or it's my
> imagination but can you double check please?
You are correct. I was a bit naive there and relied on the
documentation of the generic versions of test_and_clear_bit().
However,
- arm returns something non-zero for the atomic versions but
uses the true boolean generic unlocked versions
- ia64 seems to returns true boolean for everything but
__test_and_clear_bit
Everyone else returns true booleans, so I think these two should
adjusted.
Andrew, please ignore this patch for now, I will resend it once I
fixed arm and ia64.
Thanks for pointing it out, Mel.
Hannes
--
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>
^ permalink raw reply [flat|nested] 18+ messages in thread