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

[Alex Shi <alex.shi@linux.alibaba.com>]

在 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@linux.alibaba.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>> Cc: Hugh Dickins <hughd@google.com>
>> Cc: yun.wang@linux.alibaba.com
>> Cc: linux-mm@kvack.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   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

> 
>>       }
>>