On Fri, Jan 22, 2016 at 03:31:27PM +0100, Andrea Arcangeli wrote: > 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;
Ughh. Shrinker interface is confusing. >From ed85b527b2ea5c0b8ed93d9d212f9f0d25cae3ab Mon Sep 17 00:00:00 2001 From: "Kirill A. Shutemov" <[email protected]> Date: Fri, 22 Jan 2016 18:10:59 +0300 Subject: [PATCH] thp: deferred_split_scan(): stop shrinker if the queue is empty If pages on queue were freed under us, deferred_split_scan() would return zero. It makes caller keep calling deferred_split_scan() without any result. Let's return SHRINK_STOP in this situation. Signed-off-by: Kirill A. Shutemov <[email protected]> Suggested-by: Andrea Arcangeli <[email protected]> --- mm/huge_memory.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 298dbc001b07..2ea5e26ce069 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -3508,6 +3508,12 @@ static unsigned long deferred_split_scan(struct shrinker *shrink, list_splice_tail(&list, &pgdata->split_queue); spin_unlock_irqrestore(&pgdata->split_queue_lock, flags); + /* + * Stop shrinker, if we didn't split any page, but the queue is empty. + * This can happen if pages were freed under us. + */ + if (!split && list_empty(&pgdata->split_queue)) + return SHRINK_STOP; return split; } -- Kirill A. Shutemov
