Re: [PATCH v7 01/10] mm/vmscan: remove unnecessary lruvec adding

[Alex Shi]

å 2020/1/10 äå4:39, Konstantin Khlebnikov åé:
> On 25/12/2019 12.04, Alex Shi wrote:
>> We don't have to add a freeable page into lru and then remove from it.
>> This change saves a couple of actions and makes the moving more clear.
>>
>> The SetPageLRU needs to be kept here for list intergrity. Otherwise:
>>
>> ÂÂÂÂ #0 mave_pages_to_lruÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ #1 release_pages
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (put_page_testzero())
>> ÂÂÂÂ if (put_page_testzero())
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ !PageLRU //skip lru_lock
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ list_add(&page->lru,);
>> ÂÂÂÂ else
>> ÂÂÂÂÂÂÂÂ list_add(&page->lru,) //corrupt
>>
>> Signed-off-by: Alex Shi <alex.shi@xxxxxxxxxxxxxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
>> Cc: Hugh Dickins <hughd@xxxxxxxxxx>
>> Cc: yun.wang@xxxxxxxxxxxxxxxxx
>> Cc: linux-mm@xxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> ---
>> Â mm/vmscan.c | 16 +++++++---------
>> Â 1 file changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 572fb17c6273..8719361b47a0 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -1852,26 +1852,18 @@ static unsigned noinline_for_stack move_pages_to_lru(struct lruvec *lruvec,
> 
> Here is another cleanup: pass only pgdat as argument.
> 
> This function reavaluates lruvec for each page under lru lock.
> Probably this is redundant for now but could be used in the future (or your patchset already use that).

Thanks a lot for comments, Konstantin!

yes, we could use pgdat here, but since to remove the pgdat later, maybe better to save this change? :)

> 
>> ÂÂÂÂÂ while (!list_empty(list)) {
>> ÂÂÂÂÂÂÂÂÂ page = lru_to_page(list);
>> ÂÂÂÂÂÂÂÂÂ VM_BUG_ON_PAGE(PageLRU(page), page);
>> +ÂÂÂÂÂÂÂ list_del(&page->lru);
>> ÂÂÂÂÂÂÂÂÂ if (unlikely(!page_evictable(page))) {
>> -ÂÂÂÂÂÂÂÂÂÂÂ list_del(&page->lru);
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ spin_unlock_irq(&pgdat->lru_lock);
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ putback_lru_page(page);
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ spin_lock_irq(&pgdat->lru_lock);
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ continue;
>> ÂÂÂÂÂÂÂÂÂ }
>> -ÂÂÂÂÂÂÂ lruvec = mem_cgroup_page_lruvec(page, pgdat);
>> -
> 
> Please leave a comment that we must set PageLRU before dropping our page reference.

Right, I will try to give a comments here.
 
> 
>> ÂÂÂÂÂÂÂÂÂ SetPageLRU(page);
>> -ÂÂÂÂÂÂÂ lru = page_lru(page);
>> -
>> -ÂÂÂÂÂÂÂ nr_pages = hpage_nr_pages(page);
>> -ÂÂÂÂÂÂÂ update_lru_size(lruvec, lru, page_zonenum(page), nr_pages);
>> -ÂÂÂÂÂÂÂ list_move(&page->lru, &lruvec->lists[lru]);
>> Â ÂÂÂÂÂÂÂÂÂ if (put_page_testzero(page)) {
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ __ClearPageLRU(page);
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ __ClearPageActive(page);
>> -ÂÂÂÂÂÂÂÂÂÂÂ del_page_from_lru_list(page, lruvec, lru);
>> Â ÂÂÂÂÂÂÂÂÂÂÂÂÂ if (unlikely(PageCompound(page))) {
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ spin_unlock_irq(&pgdat->lru_lock);
>> @@ -1880,6 +1872,12 @@ static unsigned noinline_for_stack move_pages_to_lru(struct lruvec *lruvec,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ } else
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ list_add(&page->lru, &pages_to_free);
>> ÂÂÂÂÂÂÂÂÂ } else {
>> +ÂÂÂÂÂÂÂÂÂÂÂ lruvec = mem_cgroup_page_lruvec(page, pgdat);
>> +ÂÂÂÂÂÂÂÂÂÂÂ lru = page_lru(page);
>> +ÂÂÂÂÂÂÂÂÂÂÂ nr_pages = hpage_nr_pages(page);
>> +
>> +ÂÂÂÂÂÂÂÂÂÂÂ update_lru_size(lruvec, lru, page_zonenum(page), nr_pages);
>> +ÂÂÂÂÂÂÂÂÂÂÂ list_add(&page->lru, &lruvec->lists[lru]);
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ nr_moved += nr_pages;
>> ÂÂÂÂÂÂÂÂÂ }
> 
> IMHO It looks better to in this way:> 
> SetPageLRU()
> 
> if (unlikely(put_page_testzero())) {
> Â<free>
> Âcontinue;
> }
> 
> <add>

Yes, this looks better!

Thanks
Alex

> 
>> ÂÂÂÂÂ }
>>