Re: [External] Re: [PATCH v2 3/6] mm: hugetlb: fix a race between freeing and dissolving the page

On Fri 08-01-21 19:52:54, Muchun Song wrote:

On Fri, Jan 8, 2021 at 7:44 PM Michal Hocko [off-list ref] wrote:

quoted

On Fri 08-01-21 18:08:57, Muchun Song wrote:

quoted

On Fri, Jan 8, 2021 at 5:31 PM Michal Hocko [off-list ref] wrote:

quoted

On Fri 08-01-21 17:01:03, Muchun Song wrote:

quoted

On Fri, Jan 8, 2021 at 4:43 PM Michal Hocko [off-list ref] wrote:

quoted

On Thu 07-01-21 23:11:22, Muchun Song wrote:

[..]

quoted

quoted

quoted

But I find a tricky problem to solve. See free_huge_page().
If we are in non-task context, we should schedule a work
to free the page. We reuse the page->mapping. If the page
is already freed by the dissolve path. We should not touch
the page->mapping. So we need to check PageHuge().
The check and llist_add() should be protected by
hugetlb_lock. But we cannot do that. Right? If dissolve
happens after it is linked to the list. We also should
remove it from the list (hpage_freelist). It seems to make
the thing more complex.

I am not sure I follow you here but yes PageHuge under hugetlb_lock
should be the reliable way to check for the race. I am not sure why we
really need to care about mapping or other state.

CPU0:                               CPU1:
free_huge_page(page)
  if (PageHuge(page))
                                    dissolve_free_huge_page(page)
                                      spin_lock(&hugetlb_lock)
                                      update_and_free_page(page)
                                      spin_unlock(&hugetlb_lock)
    llist_add(page->mapping)
    // the mapping is corrupted

The PageHuge(page) and llist_add() should be protected by
hugetlb_lock. Right? If so, we cannot hold hugetlb_lock
in free_huge_page() path.

OK, I see. I completely forgot about this snowflake. I thought that
free_huge_page was a typo missing initial __. Anyway you are right that
this path needs a check as well. But I don't see why we couldn't use the
lock here. The lock can be held only inside the !in_task branch.

Because we hold the hugetlb_lock without disable irq. So if an interrupt
occurs after we hold the lock. And we also free a HugeTLB page. Then
it leads to deadlock.

There is nothing really to prevent making hugetlb_lock irq safe, isn't
it?

Yeah. We can make the hugetlb_lock irq safe. But why have we not
done this? Maybe the commit changelog can provide more information.

See https://github.com/torvalds/linux/commit/c77c0a8ac4c522638a8242fcb9de9496e3cdbb2d

Dang! Maybe it is the time to finally stack one workaround on top of the
other and put this code into the shape. The amount of hackery and subtle
details has just grown beyond healthy!
-- 
Michal Hocko
SUSE Labs