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

[Andrea Arcangeli]

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