On Thu, Feb 18, 2021 at 01:52:38PM +0100, Michal Hocko wrote:

quoted

Ok, makes sense.
__GFP_THISNODE will not allow fallback to other node's zones.
Since we only allow the nid the page belongs to, nodemask should be
NULL, right?

I would have to double check because hugetlb has a slightly different
expectations from nodemask than the page allocator. The later translates
that to all possible nodes but hugetlb API tries to dereference nodes.
Maybe THIS node special cases it somewhere.

Uhm, I do not quite follow here.
AFAICS, alloc_fresh_huge_page->alloc_buddy_huge_page does nothing
with the nodemask, bur rather with nodes_retry mask. That is done
to not retry on a node we failed to allocate a page.

Now, alloc_buddy_huge_page calls __alloc_pages_nodemask directly.
If my understanding is correct, it is ok to have a null nodemask
as __next_zones_zonelist() will go through our own zonelist,
since __GFP_THISNODE made us take ZONELIST_NOFALLBACK.

Actually, I do not see how passing a non-null nodemask migth have
helped there, unless we allow to specify more nodes.

quoted

I did. The 'put_page' call should be placed above, right after getting
the page. Otherwise, refcount == 1 and we will fail to dissolve the
new page if we need to (in case old page fails to be dissolved).
I already fixed that locally.

I am not sure I follow. newly allocated pages is unreferenced
unconditionally and the old page is not referenced by this path.

Current code is:

 allocate_a_new_page (new_page's refcount = 1)
 dissolve_old_page
  : if fail
     dissolve_new_page (we cannot dissolve it refcount != 0)
 put_page(new_page);

It should be:

 allocate_a_new_page (new_page's refcount = 1)
 put_page(new_page); (new_page's refcount = 0)
 dissolve_old_page
  : if fail
     dissolve_new_page (we can dissolve it as refcount == 0)

I hope this clarifies it .

-- 
Oscar Salvador
SUSE L3