Re: [PATCH 6/9] clockpro-clockpro.patch

IWAMOTO Toshihiro
2006-02-09T10:07:56Z

At Thu, 09 Feb 2006 16:22:40 +0900,
IWAMOTO Toshihiro wrote:
>
> At Wed, 08 Feb 2006 21:00:10 +0100,
> Peter Zijlstra wrote:
> >
> > On Wed, 2006-02-08 at 19:05 +0900, IWAMOTO Toshihiro wrote:
> > > I've noticed the way page_replace_reinsert puts pages to a list is
> > > different from what the paper (TR-05-3.pdf) says.
> >
> > I reread the paper, and you seem to have found a detail I had not
> > noticed before. Thanks!
> >
> > It concerns section 4.3 paragraph 2 and the use of list head as defined
> > in section 4.2 paragraph 2.
> >
> > The way I read it is that pages that have their reference bit set are
> > moved to the list head, which according to 4.2.2 is the page right in
> > front of hand hot (which would correspond to the tail of the HAND_HOT
> > list).
> >
> > So I agree the current code is wrong. However I read it differently.
> >
> > In your patch you move all Hot pages to the head of the hot list, which
> > effectively leaves them in place. The other pages are moved to the head
> > of the cold list, which is right behind hand hot.
> >
> > So I don't see your condition nor the placement. Could you explain your
> > reasoning?
>
> My patch should be okay for pages which are already hot when Hcold
> passes, but I found that the paper says pages which are turned hot by
> Hcold should be placed at the list head, which requires a page flag to
> implement. I think the PG_test bit could be abused for this purpose.

How about this patch? I got a similar benchmark result as before (but
probably with statistically significant differences for some n).

I found the usage of page_replace_reinsert is quite different and that
part of the patch might need to be dropped (but left as is for now).
And it seems that __select_list_hand in __page_replace_add need to be
removed, too.

--- a/mm/clockpro.c.orig 2006-02-06 14:41:40.000000000 +0900
+++ a/mmclockpro.c 2006-02-09 18:52:36.000000000 +0900
@@ -444,9 +444,11 @@ void page_replace_reinsert(struct list_h
spin_unlock_irq(&zone->lru_lock);
zone = pagezone;
spin_lock_irq(&zone->lru_lock);
- __select_list_hand(zone, &zone->policy.list_hand[HAND_HOT]);
}
- list_move(&page->lru, &zone->policy.list_hand[HAND_HOT]);
+ if (!PageHot(page))
+ list_move(&page->lru, &zone->policy.list_hand[HAND_COLD]);
+ else
+ list_move(&page->lru, &zone->policy.list_hand[HAND_HOT]);
__page_release(zone, page, &pvec);
}
if (zone)
@@ -507,20 +509,25 @@ void page_replace_reinsert_zone(struct z
{
struct pagevec pvec;
unsigned long dct = 0;
+ int newhot;

pagevec_init(&pvec, 1);
spin_lock_irq(&zone->lru_lock);
- __select_list_hand(zone, &zone->policy.list_hand[HAND_HOT]);
while (!list_empty(page_list)) {
struct page *page = lru_to_page(page_list);
prefetchw_prev_lru_page(page, page_list, flags);

if (PageHot(page) && PageTest(page)) {
ClearPageTest(page);
+ newhot = 1;
++dct;
- }
+ } else
+ newhot = 0;

- list_move(&page->lru, &zone->policy.list_hand[HAND_HOT]);
+ if (!PageHot(page) || newhot)
+ list_move(&page->lru, &zone->policy.list_hand[HAND_COLD]);
+ else
+ list_move(&page->lru, &zone->policy.list_hand[HAND_HOT]);
__page_release(zone, page, &pvec);
}
__cold_target_inc(zone, dct);