Re: [PATCH v2] mm: memory: fix truncation removing mapped folios

["David Hildenbrand (Arm)" <david@kernel.org>]

On 10/15/26 08:41, Baolin Wang wrote:
> 
> 
> On 10/14/26 9:25 PM, David Hildenbrand (Arm) wrote:
>> On 10/13/26 11:36, Baolin Wang wrote:
>>>
>>>
>>>
>>> I had considered this approach before, but it is not feasible. IIUC, the
>>> sync_with_folio_pmd_zap() is used to address the race with zap_huge_pmd(), and
>>> both the PMD clear and the folio mapcount decrement are performed under the PMD
>>> lock in zap_huge_pmd().
>>
>> Just to be clear, I am thinking about something like the following on the
>> zap_pmd_range() side:
>>
>> diff --git a/mm/internal.h b/mm/internal.h
>> index 38b1165212c94..a8820ad7e2f21 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -584,14 +584,15 @@ void free_pgtables(struct mmu_gather *tlb, struct
>> unmap_desc *desc);
>>   void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte);
>>
>>   /**
>> - * sync_with_folio_pmd_zap - sync with concurrent zapping of a folio PMD
>> + * sync_with_folio_pmd_zap - sync with concurrent zapping of folios
>>    * @mm: The mm_struct.
>>    * @pmdp: Pointer to the pmd that was found to be pmd_none().
>>    *
>>    * When we find a pmd_none() while unmapping a folio without holding the PTL,
>> - * zap_huge_pmd() may have cleared the PMD but not yet modified the folio to
>> - * indicate that it's unmapped. Skipping the PMD without synchronization could
>> - * make folio unmapping code assume that unmapping failed.
>> + * zap_huge_pmd() or zap_empty_pte_table() may have cleared the PMD but not
>> + * yet modified the mapped folios to indicate they are unmapped. Skipping the
>> + * PMD without synchronization could make folio unmapping code assume that
>> + * unmapping failed.
>>    *
>>    * Wait for concurrent zapping to complete by grabbing the PTL.
>>    */
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 8b0c2c735d3de..c585f5eb255c9 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -2139,9 +2139,7 @@ static inline unsigned long zap_pmd_range(struct
>> mmu_gather *tlb,
>>                                  continue;
>>                          }
>>                          /* fall through */
>> -               } else if (details && details->single_folio &&
>> -                          folio_test_pmd_mappable(details->single_folio) &&
>> -                          next - addr == HPAGE_PMD_SIZE && pmd_none(*pmd)) {
>> +               } else if (details && details->single_folio && pmd_none(*pmd)) {
>>                          sync_with_folio_pmd_zap(tlb->mm, pmd);
>>                  }
>>                  if (pmd_none(*pmd)) {
>>
> 
> Yes, I've considered that. However, implicit memory barriers give me a headache,
> so I ended up going with explicit memory barriers (suggested by Pedro).
> 
> If you don't like memory barriers, let me re-check whether the implicit memory
> barriers here can ensure race safety with sync_with_folio_pmd_zap(). A
> simplified logic is as follows:
> 
> CPU 0 (truncation)                        CPU 1 (zapping)
> down_read() ->Acquire A1.                pte spin_lock() ->Acquire A4
>                                          clear mapcount
> 
>                                          pmd spin_lock() ->Acquire A5
>                                          clear pmd
>                                          pmd spin_unlock() ->Release R5
> 
> if (pmd_none()) { //read pmd
>                                          pte spin_unlock() ->Release R4
>     pmd spin_lock() ->Acquire A2
>     pmd spin_unlock() ->Release R2
> }
> up_read() -> Release R1
> 
> spin_lock(y) ->Acquire A3
> folio_mapped() // read mapcount
> spin_unlock(y) ->Release R3
> 
> CPU 0 constraints:
> 1. R2 is a release, so "read pmd" completes before R2.
> 2. A2 completes before A3 (ACQUIRE vs ACQUIRE).
> 3. "read mapcount" completes after A3, hence after A2.
> 
> CPU 1 constraints:
> 1. R5 is a release. Both "clear mapcount" and "clear pmd" complete before R5.
> 
> According to above constraints, if CPU0 sees pmd_none() caused by CPU1's clear
> pmd, and CPU0 then acquires the same pmd lock released by CPU1 at R5, then
> CPU1's clear mapcount happens before CPU0's later folio_mapped().
> 
> Therefore folio_mapped() is guaranteed not to read the old mapcount in this
> case. So I think sync_with_folio_pmd_zap() can work, but needs a small change to
> avoid the double pmd_none() check.


I'd be okay with some (follow-up?) memory barrier handling inside
sync_with_folio_pmd_zap(), for example, based on the folio size and with proper
documentation why we need the PT lock in one case and why the memory barrier is
ok in the other case.

> 
> David, does the following diff look good to you?

I think we should extend the sync_with_folio_pmd_zap() doc as suggested.

> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 6a8e7772b8d6..c0aadab1b59e 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2036,6 +2036,15 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
>                 }
>         } while (pte += nr, addr += PAGE_SIZE * nr, addr != end);
> 
> +       add_mm_rss_vec(mm, rss);
> +       lazy_mmu_mode_disable();
> +
> +       /* Do the actual TLB flush before dropping ptl */
> +       if (force_flush) {
> +               tlb_flush_mmu_tlbonly(tlb);
> +               tlb_flush_rmaps(tlb, vma);
> +       }
> +
>         /*
>          * Fast path: try to hold the pmd lock and unmap the PTE page.
>          *
> @@ -2044,16 +2053,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
>          * to ensure they are still none, thereby preventing the pte entries
>          * from being repopulated by another thread.
>          */
> -       if (can_reclaim_pt && direct_reclaim && addr == end)
> +       if (can_reclaim_pt && direct_reclaim && addr == end) {
>                 direct_reclaim = zap_empty_pte_table(mm, pmd, ptl, &pmdval);
> -
> -       add_mm_rss_vec(mm, rss);
> -       lazy_mmu_mode_disable();
> -
> -       /* Do the actual TLB flush before dropping ptl */
> -       if (force_flush) {
> -               tlb_flush_mmu_tlbonly(tlb);
> -               tlb_flush_rmaps(tlb, vma);
>         }
>         pte_unmap_unlock(start_pte, ptl);
> 
> @@ -2102,12 +2103,11 @@ static inline unsigned long zap_pmd_range(struct
> mmu_gather *tlb,
>                                 continue;
>                         }
>                         /* fall through */
> -               } else if (details && details->single_folio &&
> -                          folio_test_pmd_mappable(details->single_folio) &&
> -                          next - addr == HPAGE_PMD_SIZE && pmd_none(*pmd)) {
> -                       sync_with_folio_pmd_zap(tlb->mm, pmd);
>                 }
> +
>                 if (pmd_none(*pmd)) {
> +                       if (details && details->single_folio)
> +                               sync_with_folio_pmd_zap(tlb->mm, pmd);
>                         addr = next;
>                         continue;
>                 }

We do have another sync_with_folio_pmd_zap() user in page_vma_mapped_walk(),
does that one need care as well?

Not sure if removing the thp_vma_suitable_order + pvmw->nr_pages negatively
affect performance, I suspect not but we better double check.

-- 
Cheers,

David