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

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 10/16/26 3:00 AM, David Hildenbrand (Arm) wrote:
> 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.

I'm afraid that we can't fit all the memory barriers into 
sync_with_folio_pmd_zap(). As the current patch does, we need memory 
barriers on both the writer and reader sides.

>> David, does the following diff look good to you?
> 
> I think we should extend the sync_with_folio_pmd_zap() doc as suggested.

Ack.

>> 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?

Good point. But I don't want to add page_vma_mapped_walk() changes into 
this fix patch. I need to analyze all callers of page_vma_mapped_walk() 
and the potential races. I'll look into whether it should be changed in 
a follow-up patch.

Thanks for your comments.