Messages in this thread | | | Subject | Re: [PATCH 5/7] mm: free user PTE page table pages | From | Mika Penttilä <> | Date | Mon, 19 Jul 2021 17:17:44 +0300 |
| |
On 19.7.2021 17.12, Qi Zheng wrote: > On 7/19/21 9:55 PM, Mika Penttilä wrote: >> >> >> On 19.7.2021 15.56, Qi Zheng wrote: >>> On 7/18/21 2:19 PM, Mika Penttilä wrote: >>> >>>>> + >>>>> +/* >>>>> + * returns true if the pmd has been populated with PTE page table, >>>>> + * or false for all other cases. >>>>> + */ >>>>> +bool pte_install_try_get(struct mm_struct *mm, pmd_t *pmd, >>>>> pgtable_t *pte) >>>>> +{ >>>>> + spinlock_t *ptl; >>>>> + bool retval = true; >>>>> + >>>>> +retry: >>>>> + ptl = pmd_lock(mm, pmd); >>>>> + if (likely(pmd_none(*pmd))) { >>>>> + __pte_install(mm, pmd, pte); >>>>> + } else if (pmd_leaf(*pmd) || !pmd_present(*pmd)) { >>>>> + retval = false; >>>>> + } else if (!pte_get_unless_zero(pmd)) { >>>>> + spin_unlock(ptl); >>>>> + goto retry; >>>>> + } >>>>> + spin_unlock(ptl); >>>>> + return retval; >>>>> +} >>>>> + >>>> >>>> Can pte_get_unless_zero() return true above? Can the pmd have been >>>> by populated by others? In that case the ref count is wrongly >>>> incremented. >>>> >>> >>> Here we only have mmap_read_lock(mm), so the pmd can be populated with >>> other PTE page table page after a page fault in a different thread B >>> of this mm. In this case, thread B already hold a pte_refcount of >>> the PTE page table page populated in the pmd, so >>> pte_get_unless_zero() can >>> return true above. >>> >> >> Yes but if thread B populates the page table page and pte, then we >> also increase the refcount with pte_get_unless_zero() , but dont >> decrease it when notice !pte_none(). >> And in the pte_none() case, the refcount is increased again, so >> double accounting. see finish_fault(). > > The semantics of this function is to hold the pte_refcount count when it > returns 1, and its caller is responsible for decrease the pte_refcount > by calling pte_put(), like the following pattern: > > do_anonymous_page() > > --> pte_alloc_try_get() > > do something about pte > > pte_put() > > Similarly in finish_fault(). > > Thanks, > > Qi
Ok I didn't notice the added pte_put()'s at the end of these funcs.
Thanks, Mika
> >> >>> Similarly, if THP is enabled, the pmd also can be populated with a >>> THP page, we can see more detail in comment in handle_pte_fault(). The >>> pmd_leaf() above is to detect this situation. >>
| |