On Wed, Aug 23, 2023 at 8:26 AM David Hildenbrand [off-list ref] wrote:

On 23.08.23 17:21, Yosry Ahmed wrote:

quoted

On Wed, Aug 23, 2023 at 8:17 AM David Hildenbrand [off-list ref] wrote:

quoted

On 23.08.23 17:12, Yosry Ahmed wrote:

quoted

On Mon, Aug 21, 2023 at 9:09 AM David Hildenbrand [off-list ref] wrote:

quoted

Let's stop using page->private on tail pages, making it possible to
just unconditionally reuse that field in the tail pages of large folios.

The remaining usage of the private field for THP_SWAP is in the THP
splitting code (mm/huge_memory.c), that we'll handle separately later.

Update the THP_SWAP documentation and sanity checks in mm_types.h and
__split_huge_page_tail().

Signed-off-by: David Hildenbrand <redacted>

The mm part looks good to me (with the added fixup):

Reviewed-by: Yosry Ahmed <redacted>

Thanks!

quoted

quoted

   /**

diff --git a/include/linux/swap.h b/include/linux/swap.h
index bb5adc604144..84fe0e94f5cd 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h

@@ -339,6 +339,15 @@ static inline swp_entry_t folio_swap_entry(struct folio *folio)
          return entry;
   }

+static inline swp_entry_t page_swap_entry(struct page *page)
+{
+       struct folio *folio = page_folio(page);
+       swp_entry_t entry = folio_swap_entry(folio);
+
+       entry.val += page - &folio->page;
+       return entry;
+}
+
   static inline void folio_set_swap_entry(struct folio *folio, swp_entry_t entry)
   {
          folio->private = (void *)entry.val;

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index cc2f65f8cc62..c04702ae71d2 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c

@@ -2446,18 +2446,15 @@ static void __split_huge_page_tail(struct page *head, int tail,
          page_tail->index = head->index + tail;

          /*
-        * page->private should not be set in tail pages with the exception
-        * of swap cache pages that store the swp_entry_t in tail pages.
-        * Fix up and warn once if private is unexpectedly set.
-        *
-        * What of 32-bit systems, on which folio->_pincount overlays
-        * head[1].private?  No problem: THP_SWAP is not enabled on 32-bit, and
-        * pincount must be 0 for folio_ref_freeze() to have succeeded.
+        * page->private should not be set in tail pages. Fix up and warn once
+        * if private is unexpectedly set.
           */
-       if (!folio_test_swapcache(page_folio(head))) {
-               VM_WARN_ON_ONCE_PAGE(page_tail->private != 0, page_tail);
+       if (unlikely(page_tail->private)) {
+               VM_WARN_ON_ONCE_PAGE(true, page_tail);
                  page_tail->private = 0;
          }

Could probably save a couple of lines here:

if (VM_WARN_ON_ONCE_PAGE(page_tail->private != 0, page_tail))

         page_tail->private = 0;

That would mean that we eventually compile out the runtime check

#define VM_WARN_ON_ONCE_PAGE(cond, page)  BUILD_BUG_ON_INVALID(cond)

I thought the warning would be compiled out but not the check, my bad.

I even remembered that VM_WARN_ON_ONCE and friends could/should not be
used in conditionals.

But we do seem to have two users now:

  $ git grep "if (VM_WARN_ON"
mm/mmap.c:              if (VM_WARN_ON_ONCE_MM(vma->vm_end != vmi_end, mm))
mm/mmap.c:              if (VM_WARN_ON_ONCE_MM(vma->vm_start != vmi_start, mm))

But they only do warning-related action, to dump the stack, the vma, ...

So if the warnings get compiled out, also all the other stuff gets compiled out as well,
which makes sense here.

Funny enough, I did the same grep and immediately thought that since
we have users of that, then it's okay (i.e the check wouldn't be
compiled out). I wasn't thorough enough to actually check what they
are doing :)

--
Cheers,

David / dhildenb

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel