refill_inactive_zone question

refill_inactive_zone question

[Marcelo Tosatti]

Hi MM gurus, 

Reading refill_inactive_zone(), while looping on pages grabbed from the inactive list (l_hold), 
refill_inactive_zone() it does:

                                                                                                                                                                                  
        while (!list_empty(&l_hold)) {
                page = lru_to_page(&l_hold);
                list_del(&page->lru);
                if (page_mapped(page)) {
                        if (!reclaim_mapped) {
                                list_add(&page->lru, &l_active);
                                continue;
                        }
                        page_map_lock(page);
                        if (page_referenced(page)) {
                                page_map_unlock(page);
                                list_add(&page->lru, &l_active);
                                continue;
                        }
                        page_map_unlock(page);
                }
                /*
                 * FIXME: need to consider page_count(page) here if/when we
                 * reap orphaned pages via the LRU (Daniel's locking stuff)
                 */
                if (total_swap_pages == 0 && PageAnon(page)) { 
                        list_add(&page->lru, &l_active);
                        continue;
                }
                list_add(&page->lru, &l_inactive);
        }


Is it possible to have AnonPages without a mapping to them? I dont think so.

Can't the check "if (total_swap_pages == 0 && PageAnon(page))" be moved
inside "if (page_mapped(page))" ? 

TIA!
--
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:"aart@kvack.org"> aart@kvack.org </a>

Re: refill_inactive_zone question

[Marcelo Tosatti]

On Fri, Aug 27, 2004 at 10:17:35PM +0100, Hugh Dickins wrote:
> On Fri, 27 Aug 2004, Marcelo Tosatti wrote:
> > 
> > Is it possible to have AnonPages without a mapping to them? I dont think so.
> 
> It was impossible, but my "remove page_map_lock" patches had to change that.
> 
> > Can't the check "if (total_swap_pages == 0 && PageAnon(page))" be moved
> > inside "if (page_mapped(page))" ? 
> 
> Yes: it's like that in -mm, and I believe now in Linus' bk tree too.
> 
> Hugh

Hi Hugh, 

Oh thanks! I see that. So you just dropped the bit spinlocked and changed
mapcount to an atomic variable, right?  Cool. Do you have any numbers on 
big SMP systems for that change? 

Talking about refill_inactive_zone(), the next stage of the algorithm:

        while (!list_empty(&l_active)) {
                page = lru_to_page(&l_active);
                prefetchw_prev_lru_page(page, &l_active, flags);
                if (TestSetPageLRU(page))
                        BUG();
                BUG_ON(!PageActive(page));
                list_move(&page->lru, &zone->active_list);
                pgmoved++;
                if (!pagevec_add(&pvec, page)) {
                        zone->nr_active += pgmoved;
                        pgmoved = 0;
                        spin_unlock_irq(&zone->lru_lock);
                        __pagevec_release(&pvec);
                        spin_lock_irq(&zone->lru_lock);
                }
        }

Several things:

1) __pagevec_release does lru_add_drain(), which moves pages in the deferred lru 
queues (active & inactive) to the actual lists. But at that point in refill_inactive()
thats not a direct benefit (we already moved scanned the inactive list at the beginning
of the algo). So, we could remove that lru_add_drain from refill_inactive_zone->__pagevec_release.

The bad part of doing unecessary lru_add_drain's is that we minimize the chance from 
the queue growing big. And the queues growing big means we batch the list moving, better
cache locality. 

Is there any good reason for doing that lru_add_drain from the refill_inactive_zone()
callchain?


2) before calling __pagevec_release we drop the zone lock, to lock it again at 
__pagevec_release->release_pages. Acquiring locks is usually more expensive 
then it seems (thanks Paul McKenney!), release_pages handles pagevec's containing pages
 from different zones, but we know all pages on this pagevec are on the same zone.
Couldnt it all be under the zone lock?

3) What happens if that __pagevec_release frees one or more pages page (and deletes it/them 
from the LRU list)?  We will still count those pages in "pgmoved" which will then be 
accounted in zone->nr_active. Whoops.

Hope I'm full of shit.
--
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:"aart@kvack.org"> aart@kvack.org </a>

Re: refill_inactive_zone question

[Marcelo Tosatti]

On Fri, Aug 27, 2004 at 05:16:41PM -0300, Marcelo Tosatti wrote:
> On Fri, Aug 27, 2004 at 10:17:35PM +0100, Hugh Dickins wrote:
> > On Fri, 27 Aug 2004, Marcelo Tosatti wrote:
> > > 
> > > Is it possible to have AnonPages without a mapping to them? I dont think so.
> > 
> > It was impossible, but my "remove page_map_lock" patches had to change that.
> > 
> > > Can't the check "if (total_swap_pages == 0 && PageAnon(page))" be moved
> > > inside "if (page_mapped(page))" ? 
> > 
> > Yes: it's like that in -mm, and I believe now in Linus' bk tree too.
> > 
> > Hugh
> 
> Hi Hugh, 
> 
> Oh thanks! I see that. So you just dropped the bit spinlocked and changed
> mapcount to an atomic variable, right?  Cool. Do you have any numbers on 
> big SMP systems for that change? 
> 
> Talking about refill_inactive_zone(), the next stage of the algorithm:
> 
>         while (!list_empty(&l_active)) {
>                 page = lru_to_page(&l_active);
>                 prefetchw_prev_lru_page(page, &l_active, flags);
>                 if (TestSetPageLRU(page))
>                         BUG();
>                 BUG_ON(!PageActive(page));
>                 list_move(&page->lru, &zone->active_list);
>                 pgmoved++;
>                 if (!pagevec_add(&pvec, page)) {
>                         zone->nr_active += pgmoved;
>                         pgmoved = 0;
>                         spin_unlock_irq(&zone->lru_lock);
>                         __pagevec_release(&pvec);
>                         spin_lock_irq(&zone->lru_lock);
>                 }
>         }
> 
> Several things:
> 
> 1) __pagevec_release does lru_add_drain(), which moves pages in the deferred lru 
> queues (active & inactive) to the actual lists. But at that point in refill_inactive()
> thats not a direct benefit (we already moved scanned the inactive list at the beginning
> of the algo). So, we could remove that lru_add_drain from refill_inactive_zone->__pagevec_release.
> 
> The bad part of doing unecessary lru_add_drain's is that we minimize the chance from 
> the queue growing big. And the queues growing big means we batch the list moving, better
> cache locality. 
> 
> Is there any good reason for doing that lru_add_drain from the refill_inactive_zone()
> callchain?
> 
> 
> 2) before calling __pagevec_release we drop the zone lock, to lock it again at 
> __pagevec_release->release_pages. Acquiring locks is usually more expensive 
> then it seems (thanks Paul McKenney!), release_pages handles pagevec's containing pages
>  from different zones, but we know all pages on this pagevec are on the same zone.
> Couldnt it all be under the zone lock?
> 
> 3) What happens if that __pagevec_release frees one or more pages page (and deletes it/them 
> from the LRU list)?  We will still count those pages in "pgmoved" which will then be 
> accounted in zone->nr_active. Whoops.

Note: All of this is also valid for the next step on the algorithm which handles the l_inactive list.

> Hope I'm full of shit. 

!!
--
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:"aart@kvack.org"> aart@kvack.org </a>

Re: refill_inactive_zone question

[Hugh Dickins]

On Fri, 27 Aug 2004, Marcelo Tosatti wrote:
> 
> Is it possible to have AnonPages without a mapping to them? I dont think so.

It was impossible, but my "remove page_map_lock" patches had to change that.

> Can't the check "if (total_swap_pages == 0 && PageAnon(page))" be moved
> inside "if (page_mapped(page))" ? 

Yes: it's like that in -mm, and I believe now in Linus' bk tree too.

Hugh

--
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:"aart@kvack.org"> aart@kvack.org </a>

Re: refill_inactive_zone question

[Hugh Dickins]

On Fri, 27 Aug 2004, Marcelo Tosatti wrote:
> 
> Oh thanks! I see that. So you just dropped the bit spinlocked and changed
> mapcount to an atomic variable, right?

That's it, yes.  It needed a little more rework than that
(see ChangeLog) but that's the main thrust.

> Cool. Do you have any numbers on big SMP systems for that change? 

Sorry, no.  When I sent out the patches a second time to Andrew (didn't
copy LKML since nothing really changed from the first time in July),
I did CC Martin in the hope that he might feel the urge to run up
some numbers (or at least let him be aware of that change lest he
misinterpret numbers), but I think he was busy with other stuff.

All I had on it was the 10% off lmbench fork numbers on my 2*HT*P4.

> Talking about refill_inactive_zone(), the next stage of the algorithm:

Sorry, I'm going to have to leave you to sort this out with someone else
(I think Nick has replied), I'm not familiar with it, and I'm falling
way behind with the things I need to attend to.

> Hope I'm full of shit.

Yeah!
Hugh

--
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:"aart@kvack.org"> aart@kvack.org </a>

Re: refill_inactive_zone question

[Martin J. Bligh]

--Hugh Dickins <hugh@veritas.com> wrote (on Saturday, August 28, 2004 17:08:23 +0100):

> On Fri, 27 Aug 2004, Marcelo Tosatti wrote:
>> 
>> Oh thanks! I see that. So you just dropped the bit spinlocked and changed
>> mapcount to an atomic variable, right?
> 
> That's it, yes.  It needed a little more rework than that
> (see ChangeLog) but that's the main thrust.
> 
>> Cool. Do you have any numbers on big SMP systems for that change? 
> 
> Sorry, no.  When I sent out the patches a second time to Andrew (didn't
> copy LKML since nothing really changed from the first time in July),
> I did CC Martin in the hope that he might feel the urge to run up
> some numbers (or at least let him be aware of that change lest he
> misinterpret numbers), but I think he was busy with other stuff.

The lab was down for rewiring, and then I forgot about it - sorry. 
Can you resend me the patch again, and I'll get off my ass and do 
something about it? ;-)

M.

--
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:"aart@kvack.org"> aart@kvack.org </a>