linux-kernel - Re: [PATCH 4/4] mm: Generalize putback scan functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0add6f90-371d-c95b-7032-49f323f96b02@virtuozzo.com>
Date:   Thu, 14 Feb 2019 13:29:01 +0300
From:   Kirill Tkhai <ktkhai@...tuozzo.com>
To:     Daniel Jordan <daniel.m.jordan@...cle.com>
Cc:     akpm@...ux-foundation.org, mhocko@...e.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] mm: Generalize putback scan functions

On 13.02.2019 22:20, Daniel Jordan wrote:
> On Tue, Feb 12, 2019 at 06:14:16PM +0300, Kirill Tkhai wrote:
>> +static unsigned noinline_for_stack move_pages_to_lru(struct lruvec *lruvec,
>> +						     struct list_head *list)
>>  {
>>  	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>> +	int nr_pages, nr_moved = 0;
>>  	LIST_HEAD(pages_to_free);
>> +	struct page *page;
>> +	enum lru_list lru;
>>  
>> -	/*
>> -	 * Put back any unfreeable pages.
>> -	 */
>> -	while (!list_empty(page_list)) {
>> -		struct page *page = lru_to_page(page_list);
>> -		int lru;
>> -
>> -		VM_BUG_ON_PAGE(PageLRU(page), page);
>> -		list_del(&page->lru);
>> +	while (!list_empty(list)) {
>> +		page = lru_to_page(list);
>>  		if (unlikely(!page_evictable(page))) {
>> +			list_del_init(&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);
>>  
>> +		VM_BUG_ON_PAGE(PageLRU(page), page);
> 
> Nit, but moving the BUG down here weakens it a little bit since we miss
> checking it if the page is unevictable.

Yeah, this is bad.
 
> Maybe worth pointing out in the changelog that the main difference from
> combining these two functions is that we're now checking for !page_evictable
> coming from shrink_active_list, which shouldn't change any behavior since that
> path works with evictable pages only.

Sounds good.