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

Peter Zijlstra
2006-02-08T20:00:10Z

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?

Kind regards,

Peter Zijlstra

> I wonder if this divergence is intended or not, but the attached patch
> gave a major improvement.
> Also, I think there's no reason to call __select_list_hand in those
> functions.
>
> n unpatched patched
> ========================
> 100 392283 595229
> 200 393087 584397
> 300 309595 521259
> 400 283340 400243
> 500 132298 303189
>
> --- linux-2.6.16-rc1-git3-clockpro/mm/clockpro.c.save 2006-02-08 17:20:40.000000000 +0900
> +++ linux-2.6.16-rc1-git3-clockpro/mm/clockpro.c 2006-02-08 17:21:16.000000000 +0900
> @@ -446,7 +446,10 @@ void page_replace_reinsert(struct list_h
> 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)
> @@ -520,7 +523,10 @@ void page_replace_reinsert_zone(struct z
> ++dct;
> }
>
> - 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);
> }
> __cold_target_inc(zone, dct);