Hi, Daniel,
Daniel Jordan <daniel.m.jordan@xxxxxxxxxx> writes:
> Hi Ying,
>
> On Tue, Nov 20, 2018 at 04:54:36PM +0800, Huang Ying wrote:
>> diff --git a/mm/swap_state.c b/mm/swap_state.c
>> index 97831166994a..1eedbc0aede2 100644
>> --- a/mm/swap_state.c
>> +++ b/mm/swap_state.c
>> @@ -387,14 +389,42 @@ struct page *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
>> * as SWAP_HAS_CACHE. That's done in later part of code or
>> * else swap_off will be aborted if we return NULL.
>> */
>> - if (!__swp_swapcount(entry) && swap_slot_cache_enabled)
>> + if (!__swp_swapcount(entry, &entry_size) &&
>> + swap_slot_cache_enabled)
>> break;
>>
>> /*
>> * Get a new page to read into from swap.
>> */
>> - if (!new_page) {
>> - new_page = alloc_page_vma(gfp_mask, vma, addr);
>> + if (!new_page ||
>> + (IS_ENABLED(CONFIG_THP_SWAP) &&
>> + hpage_nr_pages(new_page) != entry_size)) {
>> + if (new_page)
>> + put_page(new_page);
>> + if (IS_ENABLED(CONFIG_THP_SWAP) &&
>> + entry_size == HPAGE_PMD_NR) {
>> + gfp_t gfp;
>> +
>> + gfp = alloc_hugepage_direct_gfpmask(vma, addr);
>
> vma is NULL when we get here from try_to_unuse, so the kernel will die on
> vma->flags inside alloc_hugepage_direct_gfpmask.
Good catch! Thanks a lot for your help to pinpoint this bug!
> try_to_unuse swaps in before it finds vma's, but even if those were reversed,
> it seems try_to_unuse wouldn't always have a single vma to pass into this path
> since it's walking the swap_map and multiple processes mapping the same huge
> page can have different huge page advice (and maybe mempolicies?), affecting
> the result of alloc_hugepage_direct_gfpmask. And yet
> alloc_hugepage_direct_gfpmask needs a vma to do its job. So, I'm not sure how
> to fix this.
>
> If the entry's usage count were 1, we could find the vma in that common case to
> give read_swap_cache_async, and otherwise allocate small pages. We'd have THPs
> some of the time and be exactly following alloc_hugepage_direct_gfpmask, but
> would also be conservative when it's uncertain.
>
> Or, if the system-wide THP settings allow it then go for it, but otherwise
> ignore vma hints and always fall back to small pages. This requires another
> way of controlling THP allocations besides alloc_hugepage_direct_gfpmask.
>
> Or maybe try_to_unuse shouldn't allocate hugepages at all, but then no perf
> improvement for try_to_unuse.
>
> What do you think?
I think that swapoff() which is the main user of try_to_unuse() isn't a
common operation in practical. So it's not necessary to make it more
complex for this.
In alloc_hugepage_direct_gfpmask(), the only information provided by vma
is: vma->flags & VM_HUGEPAGE. Because we have no vma available, I think
it is OK to just assume that the flag is cleared. That is, rely on
system-wide THP settings only.
What do you think about this proposal?
Best Regards,
Huang, Ying
