On Tue, Jan 12, 2021 at 7:12 PM Michal Hocko [off-list ref] wrote:

On Tue 12-01-21 18:49:17, Muchun Song wrote:

quoted

On Tue, Jan 12, 2021 at 6:06 PM Michal Hocko [off-list ref] wrote:

quoted

On Tue 12-01-21 17:51:05, Muchun Song wrote:

quoted

On Tue, Jan 12, 2021 at 4:33 PM Michal Hocko [off-list ref] wrote:

quoted

On Mon 11-01-21 17:20:51, Mike Kravetz wrote:

quoted

On 1/10/21 4:40 AM, Muchun Song wrote:

quoted

There is a race between dissolve_free_huge_page() and put_page(),
and the race window is quite small. Theoretically, we should return
-EBUSY when we encounter this race. In fact, we have a chance to
successfully dissolve the page if we do a retry. Because the race
window is quite small. If we seize this opportunity, it is an
optimization for increasing the success rate of dissolving page.

If we free a HugeTLB page from a non-task context, it is deferred
through a workqueue. In this case, we need to flush the work.

The dissolve_free_huge_page() can be called from memory hotplug,
the caller aims to free the HugeTLB page to the buddy allocator
so that the caller can unplug the page successfully.

Signed-off-by: Muchun Song <redacted>
---
 mm/hugetlb.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

I am unsure about the need for this patch.  The code is OK, there are no
issues with the code.

As mentioned in the commit message, this is an optimization and could
potentially cause a memory offline operation to succeed instead of fail.
However, we are very unlikely to ever exercise this code.  Adding an
optimization that is unlikely to be exercised is certainly questionable.

Memory offline is the only code that could benefit from this optimization.
As someone with more memory offline user experience, what is your opinion
Michal?

I am not a great fun of optimizations without any data to back them up.
I do not see any sign this code has been actually tested and the
condition triggered.

This race is quite small. I only trigger this only once on my server.
And then the kernel panic. So I sent this patch series to fix some
bugs.

Memory hotplug shouldn't panic when this race happens. Are you sure you
have seen a race that is directly related to this patch?

I mean the panic is fixed by:

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

OK, so the answer is that this is not really triggered by any real life
problem. Can you actually trigger it intentionally?

quoted

quoted

quoted

quoted

Besides that I have requested to have an explanation of why blocking on
the WQ is safe and that hasn't happened.

I have seen all the caller of dissolve_free_huge_page, some caller is under
page lock (via lock_page). Others are also under a sleep context.

So I think that blocking on the WQ is safe. Right?

I have requested to explicitly write your thinking why this is safe so
that we can double check it. Dependency on a work queue progress is much
more complex than any other locks because there is no guarantee that WQ
will make forward progress (all workers might be stuck, new workers not
able to be created etc.).

OK. I know about your concern. How about setting the page as temporary
when hitting this race?

 int dissolve_free_huge_page(struct page *page)
 {

@@ -1793,8 +1794,10 @@ int dissolve_free_huge_page(struct page *page)
                 * We should make sure that the page is already on the free list
                 * when it is dissolved.
                 */
-               if (unlikely(!PageHugeFreed(head)))
+               if (unlikely(!PageHugeFreed(head))) {
+                       SetPageHugeTemporary(page)
                        goto out;
+               }

Setting the page as temporary and just return -EBUSY (do not flush
the work). __free_huge_page() will free it to the buddy allocator later.

Can we stop these subtle hacks please? Temporary page is meant to
represent unaccounted temporary page for migration. This has nothing to
do with it.

Sure. Can drop this patch.

--
Michal Hocko
SUSE Labs