linux-kernel - Re: [PATCH 2/3] thp: change deferred_split_count() to return number of THP in queue

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]

Date:	Fri, 22 Jan 2016 15:31:27 +0100
From:	Andrea Arcangeli <aarcange@...hat.com>
To:	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Hugh Dickins <hughd@...gle.com>,
	Dave Hansen <dave.hansen@...el.com>,
	Mel Gorman <mgorman@...e.de>, Rik van Riel <riel@...hat.com>,
	Vlastimil Babka <vbabka@...e.cz>,
	Christoph Lameter <cl@...two.org>,
	Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
	Steve Capper <steve.capper@...aro.org>,
	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Michal Hocko <mhocko@...e.cz>,
	Jerome Marchand <jmarchan@...hat.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH 2/3] thp: change deferred_split_count() to return number
 of THP in queue

On Thu, Jan 21, 2016 at 03:09:22PM +0300, Kirill A. Shutemov wrote:
> @@ -3511,7 +3506,7 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
>  	list_splice_tail(&list, &pgdata->split_queue);
>  	spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
>  
> -	return split * HPAGE_PMD_NR / 2;
> +	return split;
>  }

Looking further at how the caller processes this "split" retval, if
the list has been fully shrunk by the page freeing, between the
split_count and split_scan, the caller seems to ignore a 0 value
returned above and it'll keep calling even if sc->nr_to_scan isn't
decreasing. The caller won't even check sc->nr_to_scan to notice that
it isn't decreasing anymore, it's write-only as far as the caller is
concerned.

It's also weird we can't return the number of freed pages and break
the loop with just one invocation of the split_scan, but that's a
slight inefficiency in the caller interface. The caller also seems to
forget to set total_scan to 0 if SHRINK_STOP was returned but perhaps
that's on purpose, however for our purpose it'd be better off if it
did.

The split_queue.next is going to be hot in the CPU cache anyway, so
unless we change the caller, it should be worth it to add a list_empty
check and return SHRINK_STOP if it was empty. Doing it at the start or
end doesn't make much difference, at the end lockless it'll deal with
the split failures too if any.

	return split ? : list_empty(&pgdat->split_queue) ? SPLIT_STOP : 0;

Thanks,
Andrea