Re: [PATCH 1/2] Page migration via Swap V2: Page Eviction

Magnus Damm
2005-10-19T10:04:08Z

On 10/19/05, Christoph Lameter <clameter@engr.sgi.com> wrote:
> On Tue, 18 Oct 2005, Magnus Damm wrote:
>
> > This function is very similar to isolate_lru_pages(), except that it
> > operates on one page at a time and drains the lru if needed. Maybe
> > isolate_lru_pages() could use this function (inline) if the spinlock
> > and drain code was moved out?
>
> isolate_lru_pages operates on batches of pages from the same zone and is
> very efficient by only taking a single lock. It also does not drain other
> processors LRUs.

Ah, I see. You have a mix of pages from different zones on your list.
Maybe it is possible to use the same kind of zone locking style as
release_pages() to avoid duplicating code...

> > I'm also curios why you choose to always use list_del() and move back
> > the page if freed elsewhere, instead of using
> > del_page_from_[in]active_list(). I guess because of performance. But
> > if that is the case, wouldn't it make sense to do as little as
> > possible with the spinlock held, ie move list_add() (when rc == 1) out
> > of the function?
>
> I tried to follow isolate_lru_pages as closely as possible. list_add() is
> a simple operation and so I left it inside following some earlier code
> from the hotplug project.

Yep, it probably won't matter.

I'm trying to figure out if this code works in all cases:

+ spin_lock_irq(&zone->lru_lock);
+ list_del(&page->lru);
+ if (!TestSetPageLRU(page)) {
+ if (PageActive(page))
+ add_page_to_active_list(zone, page);
+ else
+ add_page_to_inactive_list(zone, page);
+ count++;
+ }
+ spin_unlock_irq(&zone->lru_lock);

Why not use if (TestSetPageLRU(page)) BUG()?

Or is it possible that someone sets the LRU bit while we are keeping
the pages on our non-lru list? If so, who is stealing and will your
put_page() patch work correctly if the page is stolen from us?

Thanks,

/ magnus