On Wed 13-01-21 18:14:55, Muchun Song wrote:

On Wed, Jan 13, 2021 at 5:33 PM Michal Hocko [off-list ref] wrote:

quoted

On Wed 13-01-21 13:22:07, Muchun Song wrote:

quoted

There is a race between dissolve_free_huge_page() and put_page().
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.

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

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 4a9011e12175..898e4ea43e13 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c

@@ -1772,6 +1772,7 @@ int dissolve_free_huge_page(struct page *page)
 {
      int rc = -EBUSY;

+retry:
      /* Not to disrupt normal path by vainly holding hugetlb_lock */
      if (!PageHuge(page))
              return 0;

@@ -1793,8 +1794,23 @@ 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)))
-                     goto out;
+             if (unlikely(!PageHugeFreed(head))) {
+                     spin_unlock(&hugetlb_lock);
+
+                     /*
+                      * 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.
+                      */
+                     while (PageHeadHuge(head) && !PageHugeFreed(head)) {
+                             cond_resched();
+                             cpu_relax();
+                     }
+                     goto retry;

OK, so you have done the retry here. Please fold it into the previous
patch. Also do we need cpu_relax on top of cond_resched as well?

Because the previous patch is a bugfix and should be backprt to the other
stable tree, right?

Yes, it is a bugfix but it arguably opens another issue so the follow up
patch should better be applied along with it.

I just want the fix patch to be small enough.
So I do the retry in this patch. If you do not agree with this. I
will fold this into the previous patch.

Do you mean this?

cpu_relax();
cond_resched();
cpu_relax();

No, I am questiong the use of cpu_relax. What is the point?

-- 
Michal Hocko
SUSE Labs