LinuxLists.cc - [patch 05/19] split LRU lists into anon & file sets

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Tue, 8 Jan 2008 14:22:38 -0800 (PST)
Christoph Lameter <[email protected]> wrote:

> It may be good to coordinate this with Andrea Arcangeli's OOM fixes.

Probably. With the split LRU lists (and the noreclaim LRUs), we can
simplify the OOM test a lot:

If free + file_active + file_inactive <= zone->pages_high and swap
space is full, the system is doomed. No need for guesswork.

> Also would it be possible to create generic functions that can move pages
> in pagevecs to an arbitrary lru list?

What would you use those functions for?

Or am I simply misunderstanding your idea?

--
All rights reversed.

2008-01-08 22:42:19

by Christoph Lameter

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Tue, 8 Jan 2008, Rik van Riel wrote:

> > Also would it be possible to create generic functions that can move pages
> > in pagevecs to an arbitrary lru list?
>
> What would you use those functions for?

We keep on duplicating the pagevec lru operation functions in mm/swap.c.
Some generic stuff would reduce the code size.

2008-01-09 02:46:13

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Tue, 8 Jan 2008 14:42:03 -0800 (PST)
Christoph Lameter <[email protected]> wrote:
> On Tue, 8 Jan 2008, Rik van Riel wrote:
>
> > > Also would it be possible to create generic functions that can move pages
> > > in pagevecs to an arbitrary lru list?
> >
> > What would you use those functions for?
>
> We keep on duplicating the pagevec lru operation functions in mm/swap.c.
> Some generic stuff would reduce the code size.

Good idea. Added to my TODO list :)

--
All rights reversed.

2008-01-09 04:40:29

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

I like this patch set thank you.

On Tue, 08 Jan 2008 15:59:44 -0500
Rik van Riel <[email protected]> wrote:
> Index: linux-2.6.24-rc6-mm1/mm/memcontrol.c
> ===================================================================
> --- linux-2.6.24-rc6-mm1.orig/mm/memcontrol.c 2008-01-07 11:55:09.000000000 -0500
> +++ linux-2.6.24-rc6-mm1/mm/memcontrol.c 2008-01-07 17:32:53.000000000 -0500
<snip>

> -enum mem_cgroup_zstat_index {
> - MEM_CGROUP_ZSTAT_ACTIVE,
> - MEM_CGROUP_ZSTAT_INACTIVE,
> -
> - NR_MEM_CGROUP_ZSTAT,
> -};
> -
> struct mem_cgroup_per_zone {
> /*
> * spin_lock to protect the per cgroup LRU
> */
> spinlock_t lru_lock;
> - struct list_head active_list;
> - struct list_head inactive_list;
> - unsigned long count[NR_MEM_CGROUP_ZSTAT];
> + struct list_head lists[NR_LRU_LISTS];
> + unsigned long count[NR_LRU_LISTS];
> };
> /* Macro for accessing counter */
> #define MEM_CGROUP_ZSTAT(mz, idx) ((mz)->count[(idx)])
> @@ -160,6 +152,7 @@ struct page_cgroup {
> };
> #define PAGE_CGROUP_FLAG_CACHE (0x1) /* charged as cache */
> #define PAGE_CGROUP_FLAG_ACTIVE (0x2) /* page is active in this cgroup */
> +#define PAGE_CGROUP_FLAG_FILE (0x4) /* page is file system backed */
>

Now, we don't have control_type and a feature for accounting only CACHE.
Balbir-san, do you have some new plan ?

BTW, is it better to use PageSwapBacked(pc->page) rather than adding a new flag
PAGE_CGROUP_FLAG_FILE ?

PAGE_CGROUP_FLAG_ACTIVE is used because global reclaim can change
ACTIVE/INACTIVE attribute without accessing memory cgroup.
(Then, we cannot trust PageActive(pc->page))

ANON <-> FILE attribute can be changed dinamically (after added to LRU) ?

If no, using page_file_cache(pc->page) will be easy.

Thanks,
-Kame

2008-01-10 02:22:03

by Balbir Singh

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

* KAMEZAWA Hiroyuki <[email protected]> [2008-01-09 13:41:32]:

> I like this patch set thank you.
>
> On Tue, 08 Jan 2008 15:59:44 -0500
> Rik van Riel <[email protected]> wrote:
> > Index: linux-2.6.24-rc6-mm1/mm/memcontrol.c
> > ===================================================================
> > --- linux-2.6.24-rc6-mm1.orig/mm/memcontrol.c 2008-01-07 11:55:09.000000000 -0500
> > +++ linux-2.6.24-rc6-mm1/mm/memcontrol.c 2008-01-07 17:32:53.000000000 -0500
> <snip>
>
> > -enum mem_cgroup_zstat_index {
> > - MEM_CGROUP_ZSTAT_ACTIVE,
> > - MEM_CGROUP_ZSTAT_INACTIVE,
> > -
> > - NR_MEM_CGROUP_ZSTAT,
> > -};
> > -
> > struct mem_cgroup_per_zone {
> > /*
> > * spin_lock to protect the per cgroup LRU
> > */
> > spinlock_t lru_lock;
> > - struct list_head active_list;
> > - struct list_head inactive_list;
> > - unsigned long count[NR_MEM_CGROUP_ZSTAT];
> > + struct list_head lists[NR_LRU_LISTS];
> > + unsigned long count[NR_LRU_LISTS];
> > };
> > /* Macro for accessing counter */
> > #define MEM_CGROUP_ZSTAT(mz, idx) ((mz)->count[(idx)])
> > @@ -160,6 +152,7 @@ struct page_cgroup {
> > };
> > #define PAGE_CGROUP_FLAG_CACHE (0x1) /* charged as cache */
> > #define PAGE_CGROUP_FLAG_ACTIVE (0x2) /* page is active in this cgroup */
> > +#define PAGE_CGROUP_FLAG_FILE (0x4) /* page is file system backed */
> >
>
> Now, we don't have control_type and a feature for accounting only CACHE.
> Balbir-san, do you have some new plan ?
>

Hi, KAMEZAWA-San,

The control_type feature is gone. We still have cached page
accounting, but we do not allow control of only RSS pages anymore. We
need to control both RSS+cached pages. I do not understand your
question about new plan? Is it about adding back control_type?

> BTW, is it better to use PageSwapBacked(pc->page) rather than adding a new flag
> PAGE_CGROUP_FLAG_FILE ?
>
>
> PAGE_CGROUP_FLAG_ACTIVE is used because global reclaim can change
> ACTIVE/INACTIVE attribute without accessing memory cgroup.
> (Then, we cannot trust PageActive(pc->page))
>

Yes, correct. A page active on the node's zone LRU need not be active
in the memory cgroup.

> ANON <-> FILE attribute can be changed dinamically (after added to LRU) ?
>
> If no, using page_file_cache(pc->page) will be easy.
>
> Thanks,
> -Kame
>

--
Warm Regards,
Balbir Singh
Linux Technology Center
IBM, ISTL

2008-01-10 02:25:58

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Tue, 08 Jan 2008 15:59:44 -0500
Rik van Riel <[email protected]> wrote:

> + rotate_sum = zone->recent_rotated_file + zone->recent_rotated_anon;
> +
> + /* Keep a floating average of RECENT references. */
> + if (unlikely(rotate_sum > min(anon, file))) {
> + spin_lock_irq(&zone->lru_lock);
> + zone->recent_rotated_file /= 2;
> + zone->recent_rotated_anon /= 2;
> + spin_unlock_irq(&zone->lru_lock);
> + rotate_sum /= 2;
> + }
> +
> + /*
> + * With swappiness at 100, anonymous and file have the same priority.
> + * This scanning priority is essentially the inverse of IO cost.
> + */
> + anon_prio = sc->swappiness;
> + file_prio = 200 - sc->swappiness;
> +
> + /*
> + * anon recent_rotated_anon
> + * %anon = 100 * ----------- / ------------------- * IO cost
> + * anon + file rotate_sum
> + */
> + ap = (anon_prio * anon) / (anon + file + 1);
> + ap *= rotate_sum / (zone->recent_rotated_anon + 1);
> + if (ap == 0)
> + ap = 1;
> + else if (ap > 100)
> + ap = 100;
> + percent[0] = ap;
> +

Hmm, it seems..

When a program copies large amount of files, recent_rotated_file increases
rapidly and

rotate_sum
----------
recent_rotated_anon

will be very big.

And %ap will be big regardless of vm_swappiness if it's not 0.

I think # of recent_successful_pageout(anon/file) should be took into account...

I'm sorry if I miss something.

Thanks,
-Kame

2008-01-10 02:36:17

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Thu, 10 Jan 2008 07:51:33 +0530
Balbir Singh <[email protected]> wrote:

> > > #define PAGE_CGROUP_FLAG_CACHE (0x1) /* charged as cache */
> > > #define PAGE_CGROUP_FLAG_ACTIVE (0x2) /* page is active in this cgroup */
> > > +#define PAGE_CGROUP_FLAG_FILE (0x4) /* page is file system backed */
> > >
> >
> > Now, we don't have control_type and a feature for accounting only CACHE.
> > Balbir-san, do you have some new plan ?
> >
>
> Hi, KAMEZAWA-San,
>
> The control_type feature is gone. We still have cached page
> accounting, but we do not allow control of only RSS pages anymore. We
> need to control both RSS+cached pages. I do not understand your
> question about new plan? Is it about adding back control_type?
>
Ah, just wanted to confirm that we can drop PAGE_CGROUP_FLAG_CACHE
if page_file_cache() function and split-LRU is introduced.

Thanks you.

-Kame

2008-01-10 02:37:42

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Thu, 10 Jan 2008 11:28:49 +0900
KAMEZAWA Hiroyuki <[email protected]> wrote:

> Hmm, it seems..
>
> When a program copies large amount of files, recent_rotated_file increases
> rapidly and
>
> rotate_sum
> ----------
> recent_rotated_anon
>
> will be very big.
>
> And %ap will be big regardless of vm_swappiness if it's not 0.
>
> I think # of recent_successful_pageout(anon/file) should be took into account...
>
> I'm sorry if I miss something.

You are right. I wonder if this, again, is a case of myself or
Lee forward porting old code. I remember having (had) a very
different version of get_scan_ratio() at some point in the past,
but I cannot remember if we discarded this version for that other
version, or the other way around :(

Lee, would you by any chance still have some alternative versions
of get_scan_ratio() around? I'm searching through my systems, but
have not found it yet...

--
All rights reversed.

2008-01-10 03:27:03

by Balbir Singh

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

* KAMEZAWA Hiroyuki <[email protected]> [2008-01-10 11:36:18]:

> On Thu, 10 Jan 2008 07:51:33 +0530
> Balbir Singh <[email protected]> wrote:
>
> > > > #define PAGE_CGROUP_FLAG_CACHE (0x1) /* charged as cache */
> > > > #define PAGE_CGROUP_FLAG_ACTIVE (0x2) /* page is active in this cgroup */
> > > > +#define PAGE_CGROUP_FLAG_FILE (0x4) /* page is file system backed */
> > > >
> > >
> > > Now, we don't have control_type and a feature for accounting only CACHE.
> > > Balbir-san, do you have some new plan ?
> > >
> >
> > Hi, KAMEZAWA-San,
> >
> > The control_type feature is gone. We still have cached page
> > accounting, but we do not allow control of only RSS pages anymore. We
> > need to control both RSS+cached pages. I do not understand your
> > question about new plan? Is it about adding back control_type?
> >
> Ah, just wanted to confirm that we can drop PAGE_CGROUP_FLAG_CACHE
> if page_file_cache() function and split-LRU is introduced.
>

Earlier we would have had a problem, since we even accounted for swap
cache with PAGE_CGROUP_FLAG_CACHE and I think page_file_cache() does
not account swap cache pages with page_file_cache(). Our accounting
is based on mapped vs unmapped whereas the new code from Rik accounts
file vs anonymous. I suspect we could live a little while longer
with PAGE_CGROUP_FLAG_CACHE and then if we do not need it at all,
we can mark it down for removal. What do you think?

--
Warm Regards,
Balbir Singh
Linux Technology Center
IBM, ISTL

2008-01-10 04:20:55

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Thu, 10 Jan 2008 08:56:31 +0530
Balbir Singh <[email protected]> wrote:

> > > The control_type feature is gone. We still have cached page
> > > accounting, but we do not allow control of only RSS pages anymore. We
> > > need to control both RSS+cached pages. I do not understand your
> > > question about new plan? Is it about adding back control_type?
> > >
> > Ah, just wanted to confirm that we can drop PAGE_CGROUP_FLAG_CACHE
> > if page_file_cache() function and split-LRU is introduced.
> >
>
> Earlier we would have had a problem, since we even accounted for swap
> cache with PAGE_CGROUP_FLAG_CACHE and I think page_file_cache() does
> not account swap cache pages with page_file_cache(). Our accounting
> is based on mapped vs unmapped whereas the new code from Rik accounts
> file vs anonymous. I suspect we could live a little while longer
> with PAGE_CGROUP_FLAG_CACHE and then if we do not need it at all,
> we can mark it down for removal. What do you think?

Okay, I have no objection.

Thanks,
-Kame

2008-01-11 03:59:55

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

Hi Rik

> -static inline long mem_cgroup_calc_reclaim_inactive(struct mem_cgroup *mem,
> - struct zone *zone, int priority)
> +static inline long mem_cgroup_calc_reclaim(struct mem_cgroup *mem,
> + struct zone *zone, int priority,
> + int active, int file)
> {
> return 0;
> }

it can't compile if memcgroup turn off.

because current mem_cgroup_calc_reclaim type is below.

long mem_cgroup_calc_reclaim(struct mem_cgroup *mem, struct zone *zone,
int priority, enum lru_list lru)

after patched below, it can compile.
I hope you don't think unpleasant by a trivial point out.

regard.

- kosaki

Index: linux-2.6.24-rc6-mm1-rvr/include/linux/memcontrol.h
===================================================================
--- linux-2.6.24-rc6-mm1-rvr.orig/include/linux/memcontrol.h 2008-01-11 11:10:16.000000000 +0900
+++ linux-2.6.24-rc6-mm1-rvr/include/linux/memcontrol.h 2008-01-11 12:08:29.000000000 +0900
@@ -168,9 +168,8 @@
{
}

-static inline long mem_cgroup_calc_reclaim(struct mem_cgroup *mem,
- struct zone *zone, int priority,
- int active, int file)
+static inline long mem_cgroup_calc_reclaim(struct mem_cgroup *mem, struct zone *zone,
+ int priority, enum lru_list lru)
{
return 0;
}

2008-01-11 06:26:00

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

Hi Rik

> +static inline int is_file_lru(enum lru_list l)
> +{
> + BUILD_BUG_ON(LRU_INACTIVE_FILE != 2 || LRU_ACTIVE_FILE != 3);
> + return (l/2 == 1);
> +}

below patch is a bit cleanup proposal.
i think LRU_FILE is more clarify than "/2".

What do you think it?

Index: linux-2.6.24-rc6-mm1-rvr/include/linux/mmzone.h
===================================================================
--- linux-2.6.24-rc6-mm1-rvr.orig/include/linux/mmzone.h 2008-01-11 11:10:30.000000000 +0900
+++ linux-2.6.24-rc6-mm1-rvr/include/linux/mmzone.h 2008-01-11 14:40:31.000000000 +0900
@@ -147,7 +147,7 @@
static inline int is_file_lru(enum lru_list l)
{
BUILD_BUG_ON(LRU_INACTIVE_FILE != 2 || LRU_ACTIVE_FILE != 3);
- return (l/2 == 1);
+ return !!(l & LRU_FILE);
}

struct per_cpu_pages {

2008-01-11 07:36:17

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

Hi Rik

> @@ -1128,64 +1026,65 @@ static void shrink_active_list(unsigned
(snip)

> + /*
> + * For sorting active vs inactive pages, we'll use the 'anon'
> + * elements of the local list[] array and sort out the file vs
> + * anon pages below.
> + */
> while (!list_empty(&l_hold)) {
> + lru = LRU_INACTIVE_ANON;
> cond_resched();
> page = lru_to_page(&l_hold);
> list_del(&page->lru);
> - if (page_mapped(page)) {
> - if (!reclaim_mapped ||
> - (total_swap_pages == 0 && PageAnon(page)) ||
> - page_referenced(page, 0, sc->mem_cgroup)) {
> - list_add(&page->lru, &list[LRU_ACTIVE]);
> - continue;
> - }
> - } else if (TestClearPageReferenced(page)) {
> - list_add(&page->lru, &list[LRU_ACTIVE]);
> - continue;
> - }
> - list_add(&page->lru, &list[LRU_INACTIVE]);
> + if (page_referenced(page, 0, sc->mem_cgroup))
> + lru = LRU_ACTIVE_ANON;
> + list_add(&page->lru, &list[lru]);
> }

Why drop (total_swap_pages == 0 && PageAnon(page)) condition?
in embedded sysmtem,
CONFIG_NORECLAIM is OFF (because almost embedded cpu is 32bit) and
that anon move to inactive list is meaningless because it doesn't have swap.

below code is more good, may be.
but I don't understand yet why ignore page_referenced() result at anon page ;-)

- kosaki

---
mm/vmscan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.24-rc6-mm1-rvr/mm/vmscan.c
===================================================================
--- linux-2.6.24-rc6-mm1-rvr.orig/mm/vmscan.c 2008-01-11 13:59:12.000000000 +0900
+++ linux-2.6.24-rc6-mm1-rvr/mm/vmscan.c 2008-01-11 16:16:44.000000000 +0900
@@ -1147,7 +1147,7 @@ static void shrink_active_list(unsigned
}

if (page_referenced(page, 0, sc->mem_cgroup)) {
- if (file)
+ if (file || (total_swap_pages == 0))
/* Referenced file pages stay active. */
lru = LRU_ACTIVE_ANON;
else

2008-01-11 15:37:45

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Fri, 11 Jan 2008 12:59:31 +0900
KOSAKI Motohiro <[email protected]> wrote:

> Hi Rik
>
> > -static inline long mem_cgroup_calc_reclaim_inactive(struct mem_cgroup *mem,
> > - struct zone *zone, int priority)
> > +static inline long mem_cgroup_calc_reclaim(struct mem_cgroup *mem,
> > + struct zone *zone, int priority,
> > + int active, int file)
> > {
> > return 0;
> > }
>
> it can't compile if memcgroup turn off.

Doh! Good point.

Thank you for pointing out this error. I applied your fix to my tree,
it will be in the next version.

--
All rights reversed.

2008-01-11 15:43:23

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Fri, 11 Jan 2008 15:24:34 +0900
KOSAKI Motohiro <[email protected]> wrote:

> below patch is a bit cleanup proposal.
> i think LRU_FILE is more clarify than "/2".
>
> What do you think it?

Thank you for the cleanup, your version looks a lot nicer.
I have applied your patch to my series.

--
All rights reversed.

2008-01-11 15:47:17

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Fri, 11 Jan 2008 16:35:24 +0900
KOSAKI Motohiro <[email protected]> wrote:

> Why drop (total_swap_pages == 0 && PageAnon(page)) condition?
> in embedded sysmtem,
> CONFIG_NORECLAIM is OFF (because almost embedded cpu is 32bit) and
> that anon move to inactive list is meaningless because it doesn't have swap.

That was a mistake, kind of. Since all swap backed pages are on their
own LRU lists, we should not scan those lists at all any more if we are
out of swap space.

The patch that fixes get_scan_ratio() adds that test.

Having said that, with the nr_swap_pages==0 test in get_scan_ratio(),
we no longer need to test for that condition in shrink_active_list().

> below code is more good, may be.
> but I don't understand yet why ignore page_referenced() result at anon page ;-)

On modern systems, swapping out anonymous pages is a relatively rare
event. All anonymous pages start out as active and referenced, so
testing for that condition does (1) not add any information and (2)
mean we need to scan ALL of the anonymous pages, in order to find one
candidate to swap out (since they are all referenced).

Simply deactivating a few pages and checking whether they were referenced
again while on the (smaller) inactive_anon_list means we can find candidates
to page out with a lot less CPU time used.

--
All rights reversed.

2008-01-11 15:49:26

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Fri, 2008-01-11 at 15:24 +0900, KOSAKI Motohiro wrote:
> Hi Rik
>
> > +static inline int is_file_lru(enum lru_list l)
> > +{
> > + BUILD_BUG_ON(LRU_INACTIVE_FILE != 2 || LRU_ACTIVE_FILE != 3);
> > + return (l/2 == 1);
> > +}
>
> below patch is a bit cleanup proposal.
> i think LRU_FILE is more clarify than "/2".
>
> What do you think it?
>
>
>
> Index: linux-2.6.24-rc6-mm1-rvr/include/linux/mmzone.h
> ===================================================================
> --- linux-2.6.24-rc6-mm1-rvr.orig/include/linux/mmzone.h 2008-01-11 11:10:30.000000000 +0900
> +++ linux-2.6.24-rc6-mm1-rvr/include/linux/mmzone.h 2008-01-11 14:40:31.000000000 +0900
> @@ -147,7 +147,7 @@
> static inline int is_file_lru(enum lru_list l)
> {
> BUILD_BUG_ON(LRU_INACTIVE_FILE != 2 || LRU_ACTIVE_FILE != 3);
> - return (l/2 == 1);
> + return !!(l & LRU_FILE);
> }
>
> struct per_cpu_pages {
>

Kosaki-san:

Again, my doing. I agree that the calculation is a bit strange, but I
wanted to "future-proof" this function in case we ever get to a value of
'6' for the lru_list enum. In that case, the AND will evaluate to
non-zero for what may not be a file LRU. Between the build time
assertion and the division [which could just be a 'l >> 1', I suppose]
we should be safe.

Thanks,
Lee

2008-01-11 15:58:34

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Fri, 2008-01-11 at 10:42 -0500, Rik van Riel wrote:
> On Fri, 11 Jan 2008 15:24:34 +0900
> KOSAKI Motohiro <[email protected]> wrote:
>
> > below patch is a bit cleanup proposal.
> > i think LRU_FILE is more clarify than "/2".
> >
> > What do you think it?
>
> Thank you for the cleanup, your version looks a lot nicer.
> I have applied your patch to my series.
>

Rik:

I think we also want to do something like:

- BUILD_BUG_ON(LRU_INACTIVE_FILE != 2 || LRU_ACTIVE_FILE != 3);
+ BUILD_BUG_ON(LRU_INACTIVE_FILE != 2 || LRU_ACTIVE_FILE != 3 ||
+ NR_LRU_LISTS > 6);

Then we'll be warned if future change might break our implicit
assumption that any lru_list value with '0x2' set is a file lru.

Lee

2008-01-11 16:06:47

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Fri, 11 Jan 2008 10:50:09 -0500
Lee Schermerhorn <[email protected]> wrote:

> Again, my doing. I agree that the calculation is a bit strange, but I
> wanted to "future-proof" this function in case we ever get to a value of
> '6' for the lru_list enum. In that case, the AND will evaluate to
> non-zero for what may not be a file LRU. Between the build time
> assertion and the division [which could just be a 'l >> 1', I suppose]
> we should be safe.

Good point. I did not guess that.

I'll restore the code to your original test.

--
All rights reversed.

2008-01-11 16:16:01

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Fri, 11 Jan 2008 10:59:18 -0500
Lee Schermerhorn <[email protected]> wrote:

> On Fri, 2008-01-11 at 10:42 -0500, Rik van Riel wrote:
> > On Fri, 11 Jan 2008 15:24:34 +0900
> > KOSAKI Motohiro <[email protected]> wrote:
> >
> > > below patch is a bit cleanup proposal.
> > > i think LRU_FILE is more clarify than "/2".
> > >
> > > What do you think it?
> >
> > Thank you for the cleanup, your version looks a lot nicer.
> > I have applied your patch to my series.
> >
>
> Rik:
>
> I think we also want to do something like:
>
> - BUILD_BUG_ON(LRU_INACTIVE_FILE != 2 || LRU_ACTIVE_FILE != 3);
> + BUILD_BUG_ON(LRU_INACTIVE_FILE != 2 || LRU_ACTIVE_FILE != 3 ||
> + NR_LRU_LISTS > 6);
>
> Then we'll be warned if future change might break our implicit
> assumption that any lru_list value with '0x2' set is a file lru.

Restoring the code to your original version makes things work again.

OTOH, I almost wonder if we should not simply define it to

return (l == LRU_INACTIVE_FILE || l == LRU_ACTIVE_FILE)

and just deal with it.

Your version of the code is correct and probably faster, but not as
easy to read and probably not in a hot path :)

--
All rights reversed.

2008-01-11 19:51:17

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Fri, 2008-01-11 at 11:15 -0500, Rik van Riel wrote:
> On Fri, 11 Jan 2008 10:59:18 -0500
> Lee Schermerhorn <[email protected]> wrote:
>
> > On Fri, 2008-01-11 at 10:42 -0500, Rik van Riel wrote:
> > > On Fri, 11 Jan 2008 15:24:34 +0900
> > > KOSAKI Motohiro <[email protected]> wrote:
> > >
> > > > below patch is a bit cleanup proposal.
> > > > i think LRU_FILE is more clarify than "/2".
> > > >
> > > > What do you think it?
> > >
> > > Thank you for the cleanup, your version looks a lot nicer.
> > > I have applied your patch to my series.
> > >
> >
> > Rik:
> >
> > I think we also want to do something like:
> >
> > - BUILD_BUG_ON(LRU_INACTIVE_FILE != 2 || LRU_ACTIVE_FILE != 3);
> > + BUILD_BUG_ON(LRU_INACTIVE_FILE != 2 || LRU_ACTIVE_FILE != 3 ||
> > + NR_LRU_LISTS > 6);
> >
> > Then we'll be warned if future change might break our implicit
> > assumption that any lru_list value with '0x2' set is a file lru.
>
> Restoring the code to your original version makes things work again.
>
> OTOH, I almost wonder if we should not simply define it to
>
> return (l == LRU_INACTIVE_FILE || l == LRU_ACTIVE_FILE)
>
> and just deal with it.
>
> Your version of the code is correct and probably faster, but not as
> easy to read and probably not in a hot path :)

Sure. Whatever you think will fly...

Lee
>

2008-01-14 23:57:40

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

Hi

> > Why drop (total_swap_pages == 0 && PageAnon(page)) condition?
> > in embedded sysmtem,
> > CONFIG_NORECLAIM is OFF (because almost embedded cpu is 32bit) and
> > that anon move to inactive list is meaningless because it doesn't have swap.
>
> That was a mistake, kind of. Since all swap backed pages are on their
> own LRU lists, we should not scan those lists at all any more if we are
> out of swap space.
>
> The patch that fixes get_scan_ratio() adds that test.
>
> Having said that, with the nr_swap_pages==0 test in get_scan_ratio(),
> we no longer need to test for that condition in shrink_active_list().

Oh I see!
thank you for your kindful lecture.

your implementation is very cute.

> > below code is more good, may be.
> > but I don't understand yet why ignore page_referenced() result at anon page ;-)
>
> On modern systems, swapping out anonymous pages is a relatively rare
> event. All anonymous pages start out as active and referenced, so
> testing for that condition does (1) not add any information and (2)
> mean we need to scan ALL of the anonymous pages, in order to find one
> candidate to swap out (since they are all referenced).
>
> Simply deactivating a few pages and checking whether they were referenced
> again while on the (smaller) inactive_anon_list means we can find candidates
> to page out with a lot less CPU time used.

thanks, I understand, may be.

- kosaki

2008-01-30 03:29:04

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

Hi Rik, Lee

I tested new hackbench on rvr split LRU patch.
http://people.redhat.com/mingo/cfs-scheduler/tools/hackbench.c

method of test

(1) $ ./hackbench 150 process 1000
(2) # sync; echo 3 > /proc/sys/vm/drop_caches
$ dd if=tmp10G of=/dev/null
$ ./hackbench 150 process 1000

test machine
CPU: Itanium2 x4 (logical 8cpu)
MEM: 8GB

A. vanilla 2.6.24-rc8-mm1
(1) 127.540
(2) 727.548

B. 2.6.24-rc8-mm1 + split-lru-patch-series
(1) 92.730
(2) 758.369

comment:
(1) active/inactive anon ratio improve performance significant.
(2) incorrect page activation reduce performance.

I investigate reason and found reason is [05/19] change.
I tested a bit porton reverted split-lru-patch-series again.

C. 2.6.24-rc8-mm1 + split-lru-patch-series + my-revert-patch
(1) 83.014
(2) 717.009

Of course, We need reintroduce this portion after new page LRU
(aka LRU for used only page).
but now is too early.

I hope this patch series merge to -mm ASAP.
therefore, I hope remove any corner case regression.

Thanks!

- kosaki

Signed-off-by: KOSAKI Motohiro <[email protected]>

---
mm/vmscan.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)

Index: b/mm/vmscan.c
===================================================================
--- a/mm/vmscan.c 2008-01-29 15:59:17.000000000 +0900
+++ b/mm/vmscan.c 2008-01-30 11:53:42.000000000 +0900
@@ -247,6 +247,27 @@
return ret;
}

+/* Called without lock on whether page is mapped, so answer is unstable */
+static inline int page_mapping_inuse(struct page *page)
+{
+ struct address_space *mapping;
+
+ /* Page is in somebody's page tables. */
+ if (page_mapped(page))
+ return 1;
+
+ /* Be more reluctant to reclaim swapcache than pagecache */
+ if (PageSwapCache(page))
+ return 1;
+
+ mapping = page_mapping(page);
+ if (!mapping)
+ return 0;
+
+ /* File is mmap'd by somebody? */
+ return mapping_mapped(mapping);
+}
+
static inline int is_page_cache_freeable(struct page *page)
{
return page_count(page) - !!PagePrivate(page) == 2;
@@ -515,7 +536,8 @@

referenced = page_referenced(page, 1, sc->mem_cgroup);
/* In active use or really unfreeable? Activate it. */
- if (sc->order <= PAGE_ALLOC_COSTLY_ORDER && referenced)
+ if (sc->order <= PAGE_ALLOC_COSTLY_ORDER &&
+ referenced && page_mapping_inuse(page))
goto activate_locked;

#ifdef CONFIG_SWAP
@@ -550,6 +572,8 @@
}

if (PageDirty(page)) {
+ if (sc->order <= PAGE_ALLOC_COSTLY_ORDER && referenced)
+ goto keep_locked;
if (!may_enter_fs) {
sc->nr_io_pages++;
goto keep_locked;

2008-01-30 08:58:28

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

Hi Rik, Lee

I found number of scan pages calculation bug.

1. wrong calculation order

ap *= rotate_sum / (zone->recent_rotated_anon + 1);

when recent_rotated_anon = 100 and recent_rotated_file = 0,

rotate_sum / (zone->recent_rotated_anon + 1)
= 100 / 101
= 0

at that time, ap become 0.

2. wrong fraction omission

nr[l] = zone->nr_scan[l] * percent[file] / 100;

when percent is very small,
nr[l] become 0.

Test Result:
(1) $ ./hackbench 150 process 1000
(2) # sync; echo 3 > /proc/sys/vm/drop_caches
$ dd if=tmp10G of=/dev/null
$ ./hackbench 150 process 1000

rvr-split-lru + revert patch of previous mail
(1) 83.014
(2) 717.009

rvr-split-lru + revert patch of previous mail + below patch
(1) 61.965
(2) 85.444 !!

Now, We got 1000% performance improvement against 2.6.24-rc8-mm1 :)

- kosaki

Signed-off-by: KOSAKI Motohiro <[email protected]>

---
mm/vmscan.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)

Index: b/mm/vmscan.c
===================================================================
--- a/mm/vmscan.c 2008-01-30 15:22:10.000000000 +0900
+++ b/mm/vmscan.c 2008-01-30 16:03:28.000000000 +0900
@@ -1355,7 +1355,7 @@ static void get_scan_ratio(struct zone *
* anon + file rotate_sum
*/
ap = (anon_prio * anon) / (anon + file + 1);
- ap *= rotate_sum / (zone->recent_rotated_anon + 1);
+ ap = (ap * rotate_sum) / (zone->recent_rotated_anon + 1);
if (ap == 0)
ap = 1;
else if (ap > 100)
@@ -1363,7 +1363,7 @@ static void get_scan_ratio(struct zone *
percent[0] = ap;

fp = (file_prio * file) / (anon + file + 1);
- fp *= rotate_sum / (zone->recent_rotated_file + 1);
+ fp = (fp * rotate_sum) / (zone->recent_rotated_file + 1);
if (fp == 0)
fp = 1;
else if (fp > 100)
@@ -1402,6 +1402,7 @@ static unsigned long shrink_zone(int pri

for_each_reclaimable_lru(l) {
if (scan_global_lru(sc)) {
+ unsigned long nr_max_scan;
int file = is_file_lru(l);
/*
* Add one to nr_to_scan just to make sure that the
@@ -1409,7 +1410,11 @@ static unsigned long shrink_zone(int pri
*/
zone->nr_scan[l] += (zone_page_state(zone,
NR_INACTIVE_ANON + l) >> priority) + 1;
- nr[l] = zone->nr_scan[l] * percent[file] / 100;
+ nr[l] = (zone->nr_scan[l] * percent[file] / 100) + 1;
+ nr_max_scan = zone_page_state(zone, NR_INACTIVE_ANON+l);
+ if (nr[l] > nr_max_scan)
+ nr[l] = nr_max_scan;
+
if (nr[l] >= sc->swap_cluster_max)
zone->nr_scan[l] = 0;
else

2008-01-30 14:43:18

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Wed, 2008-01-30 at 17:57 +0900, KOSAKI Motohiro wrote:
> Hi Rik, Lee
>
> I found number of scan pages calculation bug.
>
> 1. wrong calculation order
>
> ap *= rotate_sum / (zone->recent_rotated_anon + 1);
>
> when recent_rotated_anon = 100 and recent_rotated_file = 0,
>
> rotate_sum / (zone->recent_rotated_anon + 1)
> = 100 / 101
> = 0
>
> at that time, ap become 0.
>
> 2. wrong fraction omission
>
> nr[l] = zone->nr_scan[l] * percent[file] / 100;
>
> when percent is very small,
> nr[l] become 0.
>
> Test Result:
> (1) $ ./hackbench 150 process 1000
> (2) # sync; echo 3 > /proc/sys/vm/drop_caches
> $ dd if=tmp10G of=/dev/null
> $ ./hackbench 150 process 1000
>
> rvr-split-lru + revert patch of previous mail
> (1) 83.014
> (2) 717.009
>
> rvr-split-lru + revert patch of previous mail + below patch
> (1) 61.965
> (2) 85.444 !!
>
>
> Now, We got 1000% performance improvement against 2.6.24-rc8-mm1 :)
>
>
>
> - kosaki
>
>
> Signed-off-by: KOSAKI Motohiro <[email protected]>

<snip>

Kosaki-san:

Rik is currently out on holiday and I've been traveling. Just getting
back to rebasing to 24-rc8-mm1. Thank you for your efforts in testing
and tracking down the regressions. I will add your fixes into my tree
and try them out and let you know. Rik mentioned to me that he has a
fix for the "get_scan_ratio()" calculation that is causing us to OOM
kill prematurely--i.e., when we still have lots of swap space to evict
swappable anon. I don't know if it's similar to what you have posted.
Have to wait and see what he says. Meantime, we'll try your patches.

Again, thank you.

Regards,
Lee

2008-01-31 01:18:23

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

Hi Lee-san

> Rik is currently out on holiday and I've been traveling. Just getting
> back to rebasing to 24-rc8-mm1. Thank you for your efforts in testing
> and tracking down the regressions. I will add your fixes into my tree
> and try them out and let you know. Rik mentioned to me that he has a
> fix for the "get_scan_ratio()" calculation that is causing us to OOM
> kill prematurely--i.e., when we still have lots of swap space to evict
> swappable anon. I don't know if it's similar to what you have posted.
> Have to wait and see what he says. Meantime, we'll try your patches.

thank you for your quick response.

on my test environment, my patch solve incorrect OOM.
because, too small reclaim cause OOM.

Please confirm.

- kosaki

2008-01-31 10:48:44

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Thu, 31 Jan 2008 10:17:48 +0900
KOSAKI Motohiro <[email protected]> wrote:

> on my test environment, my patch solve incorrect OOM.
> because, too small reclaim cause OOM.

That makes sense.

The version you two are looking at can return
"percentages" way larger than 100 in get_scan_ratio.

A fixed version of get_scan_ratio, where the
percentages always add up to 100%, makes the
system go OOM before it seriously starts
swapping.

I will integrate your fixes with my code when I
get back from holidays. Then things should work :)

Thank you for your analysis of the problem.

2008-01-31 11:00:05

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

> I will integrate your fixes with my code when I
> get back from holidays. Then things should work :)
>
> Thank you for your analysis of the problem.

Thank you.
enjoy good vacation :)

-
kosaki

2008-02-07 00:47:10

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

On Wed, 30 Jan 2008 17:57:54 +0900
KOSAKI Motohiro <[email protected]> wrote:

> I found number of scan pages calculation bug.

My latest version of get_scan_ratio() works differently, with the
percentages always adding up to 100. However, your patch gave me
the inspiration to (hopefully) find the bug in my version of the
code.

> 1. wrong calculation order

I do not believe my new code has this problem. Of course, this
is purely due to luck :)

> 2. wrong fraction omission
>
> nr[l] = zone->nr_scan[l] * percent[file] / 100;
>
> when percent is very small,
> nr[l] become 0.

This is probably where the problem is. Kind of.

I believe that the problem is that we scale nr[l] by the percentage,
instead of scaling the amount we add to zone->nr_scan[l] by the
percentage!

> @@ -1409,7 +1410,11 @@ static unsigned long shrink_zone(int pri
> */
> zone->nr_scan[l] += (zone_page_state(zone,
> NR_INACTIVE_ANON + l) >> priority) + 1;
> - nr[l] = zone->nr_scan[l] * percent[file] / 100;
> + nr[l] = (zone->nr_scan[l] * percent[file] / 100) + 1;
> + nr_max_scan = zone_page_state(zone, NR_INACTIVE_ANON+l);
> + if (nr[l] > nr_max_scan)
> + nr[l] = nr_max_scan;
> +
> if (nr[l] >= sc->swap_cluster_max)
> zone->nr_scan[l] = 0;
> else

With the fix below (against my latest tree), we always add at least one
to zone->nr_scan[l] and always make that increment count later on!

I am still recovering from my trip home (thanks to the airline companies
I spent 25 hours travelling, from door to door), so I may not get around
to actually testing this today:

Index: linux-2.6.24-rc6-mm1/mm/vmscan.c
===================================================================
--- linux-2.6.24-rc6-mm1.orig/mm/vmscan.c 2008-02-06 19:23:16.000000000 -0500
+++ linux-2.6.24-rc6-mm1/mm/vmscan.c 2008-02-06 19:22:55.000000000 -0500
@@ -1275,13 +1275,17 @@ static unsigned long shrink_zone(int pri
for_each_lru(l) {
if (scan_global_lru(sc)) {
int file = is_file_lru(l);
+ int scan;
/*
* Add one to nr_to_scan just to make sure that the
- * kernel will slowly sift through the active list.
+ * kernel will slowly sift through each list.
*/
- zone->nr_scan[l] += (zone_page_state(zone,
- NR_INACTIVE_ANON + l) >> priority) + 1;
- nr[l] = zone->nr_scan[l] * percent[file] / 100;
+ scan = zone_page_state(zone, NR_INACTIVE_ANON + l);
+ scan >>= priority;
+ scan = (scan * percent[file]) / 100;
+
+ zone->nr_scan[l] += scan + 1;
+ nr[l] = zone->nr_scan[l];
if (nr[l] >= sc->swap_cluster_max)
zone->nr_scan[l] = 0;
else

2008-02-07 01:21:18

[permalink] [raw]

Subject: Re: [patch 05/19] split LRU lists into anon & file sets

Hi Rik

Welcome back :)

> > I found number of scan pages calculation bug.
>
> My latest version of get_scan_ratio() works differently, with the
> percentages always adding up to 100. However, your patch gave me
> the inspiration to (hopefully) find the bug in my version of the
> code.

OK.

> > 2. wrong fraction omission
> >
> > nr[l] = zone->nr_scan[l] * percent[file] / 100;
> >
> > when percent is very small,
> > nr[l] become 0.
>
> This is probably where the problem is. Kind of.
>
> I believe that the problem is that we scale nr[l] by the percentage,
> instead of scaling the amount we add to zone->nr_scan[l] by the
> percentage!

Aahh,
you are right.

> Index: linux-2.6.24-rc6-mm1/mm/vmscan.c
> ===================================================================
> --- linux-2.6.24-rc6-mm1.orig/mm/vmscan.c 2008-02-06 19:23:16.000000000 -0500
> +++ linux-2.6.24-rc6-mm1/mm/vmscan.c 2008-02-06 19:22:55.000000000 -0500
> @@ -1275,13 +1275,17 @@ static unsigned long shrink_zone(int pri
> for_each_lru(l) {
> if (scan_global_lru(sc)) {
> int file = is_file_lru(l);
> + int scan;
> /*
> * Add one to nr_to_scan just to make sure that the
> - * kernel will slowly sift through the active list.
> + * kernel will slowly sift through each list.
> */
> - zone->nr_scan[l] += (zone_page_state(zone,
> - NR_INACTIVE_ANON + l) >> priority) + 1;
> - nr[l] = zone->nr_scan[l] * percent[file] / 100;
> + scan = zone_page_state(zone, NR_INACTIVE_ANON + l);
> + scan >>= priority;
> + scan = (scan * percent[file]) / 100;
> +
> + zone->nr_scan[l] += scan + 1;
> + nr[l] = zone->nr_scan[l];
> if (nr[l] >= sc->swap_cluster_max)
> zone->nr_scan[l] = 0;
> else

looks good.
thank you clean up code.

- kosaki

2008-02-07 01:36:54