| Message ID | 20240904084022.32728-2-zhengqi.arch@bytedance.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | introduce pte_offset_map_{ro|rw}_nolock() | expand |
On 2024/9/4 16:40, Qi Zheng wrote: > Currently, the usage of pte_offset_map_nolock() can be divided into the > following two cases: > > 1) After acquiring PTL, only read-only operations are performed on the PTE > page. In this case, the RCU lock in pte_offset_map_nolock() will ensure > that the PTE page will not be freed, and there is no need to worry > about whether the pmd entry is modified. > > 2) After acquiring PTL, the pte or pmd entries may be modified. At this > time, we need to ensure that the pmd entry has not been modified > concurrently. > > To more clearing distinguish between these two cases, this commit > introduces two new helper functions to replace pte_offset_map_nolock(). > For 1), just rename it to pte_offset_map_ro_nolock(). For 2), in addition > to changing the name to pte_offset_map_rw_nolock(), it also outputs the > pmdval when successful. It is applicable for may-write cases where any > modification operations to the page table may happen after the > corresponding spinlock is held afterwards. But the users should make sure > the page table is stable like checking pte_same() or checking pmd_same() > by using the output pmdval before performing the write operations. > > Note: "RO" / "RW" expresses the intended semantics, not that the *kmap* > will be read-only/read-write protected. > > Subsequent commits will convert pte_offset_map_nolock() into the above > two functions one by one, and finally completely delete it. > > Signed-off-by: Qi Zheng<zhengqi.arch@bytedance.com> > --- > Documentation/mm/split_page_table_lock.rst | 7 +++ > include/linux/mm.h | 5 +++ > mm/pgtable-generic.c | 50 ++++++++++++++++++++++ > 3 files changed, 62 insertions(+) > > diff --git a/Documentation/mm/split_page_table_lock.rst b/Documentation/mm/split_page_table_lock.rst > index e4f6972eb6c04..08d0e706a32db 100644 > --- a/Documentation/mm/split_page_table_lock.rst > +++ b/Documentation/mm/split_page_table_lock.rst > @@ -19,6 +19,13 @@ There are helpers to lock/unlock a table and other accessor functions: > - pte_offset_map_nolock() > maps PTE, returns pointer to PTE with pointer to its PTE table > lock (not taken), or returns NULL if no PTE table; > + - pte_offset_map_ro_nolock() > + maps PTE, returns pointer to PTE with pointer to its PTE table > + lock (not taken), or returns NULL if no PTE table; > + - pte_offset_map_rw_nolock() > + maps PTE, returns pointer to PTE with pointer to its PTE table > + lock (not taken) and the value of its pmd entry, or returns NULL > + if no PTE table; > - pte_offset_map() > maps PTE, returns pointer to PTE, or returns NULL if no PTE table; > - pte_unmap() > diff --git a/include/linux/mm.h b/include/linux/mm.h > index a7c74a840249a..1fde9242231c9 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -3006,6 +3006,11 @@ static inline pte_t *pte_offset_map_lock(struct mm_struct *mm, pmd_t *pmd, > > pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd, > unsigned long addr, spinlock_t **ptlp); > +pte_t *pte_offset_map_ro_nolock(struct mm_struct *mm, pmd_t *pmd, > + unsigned long addr, spinlock_t **ptlp); > +pte_t *pte_offset_map_rw_nolock(struct mm_struct *mm, pmd_t *pmd, > + unsigned long addr, pmd_t *pmdvalp, > + spinlock_t **ptlp); > > #define pte_unmap_unlock(pte, ptl) do { \ > spin_unlock(ptl); \ > diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c > index a78a4adf711ac..262b7065a5a2e 100644 > --- a/mm/pgtable-generic.c > +++ b/mm/pgtable-generic.c > @@ -317,6 +317,33 @@ pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd, > return pte; > } > > +pte_t *pte_offset_map_ro_nolock(struct mm_struct *mm, pmd_t *pmd, > + unsigned long addr, spinlock_t **ptlp) > +{ > + pmd_t pmdval; > + pte_t *pte; > + > + pte = __pte_offset_map(pmd, addr, &pmdval); > + if (likely(pte)) > + *ptlp = pte_lockptr(mm, &pmdval); > + return pte; > +} > + > +pte_t *pte_offset_map_rw_nolock(struct mm_struct *mm, pmd_t *pmd, > + unsigned long addr, pmd_t *pmdvalp, > + spinlock_t **ptlp) > +{ > + pmd_t pmdval; > + pte_t *pte; > + > + VM_WARN_ON_ONCE(!pmdvalp); > + pte = __pte_offset_map(pmd, addr, &pmdval); > + if (likely(pte)) > + *ptlp = pte_lockptr(mm, &pmdval); > + *pmdvalp = pmdval; > + return pte; > +} > + > /* > * pte_offset_map_lock(mm, pmd, addr, ptlp), and its internal implementation > * __pte_offset_map_lock() below, is usually called with the pmd pointer for > @@ -356,6 +383,29 @@ pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd, > * recheck *pmd once the lock is taken; in practice, no callsite needs that - > * either the mmap_lock for write, or pte_same() check on contents, is enough. > * > + * pte_offset_map_ro_nolock(mm, pmd, addr, ptlp), above, is like pte_offset_map(); > + * but when successful, it also outputs a pointer to the spinlock in ptlp - as > + * pte_offset_map_lock() does, but in this case without locking it. This helps > + * the caller to avoid a later pte_lockptr(mm, *pmd), which might by that time > + * act on a changed *pmd: pte_offset_map_ro_nolock() provides the correct spinlock > + * pointer for the page table that it returns. Even after grabbing the spinlock, > + * we might be looking either at a page table that is still mapped or one that > + * was unmapped and is about to get freed. But for R/O access this is sufficient. > + * So it is only applicable for read-only cases where any modification operations > + * to the page table are not allowed even if the corresponding spinlock is held > + * afterwards. > + * > + * pte_offset_map_rw_nolock(mm, pmd, addr, pmdvalp, ptlp), above, is like > + * pte_offset_map_ro_nolock(); but when successful, it also outputs the pdmval. > + * It is applicable for may-write cases where any modification operations to the > + * page table may happen after the corresponding spinlock is held afterwards. > + * But the users should make sure the page table is stable like checking pte_same() > + * or checking pmd_same() by using the output pmdval before performing the write > + * operations. Now, we have two options to make sure the stability of PTE for users of pte_offset_map_rw_nolock(), in order to ease this operation, how about proposing a new helper (or two, one for pmd_same, another for pte_same) like pte_lock_stability (I am not good at naming, maybe you can) which helps users 1) hold the PTL and 2) check if the PTE is stable and 3) return true if the PTE stable, otherwise return false. Muchun, Thanks. > + * > + * Note: "RO" / "RW" expresses the intended semantics, not that the *kmap* will > + * be read-only/read-write protected. > + * > * Note that free_pgtables(), used after unmapping detached vmas, or when > * exiting the whole mm, does not take page table lock before freeing a page > * table, and may not use RCU at all: "outsiders" like khugepaged should avoid
Hi Muchun, On 2024/9/6 15:20, Muchun Song wrote: > > > On 2024/9/4 16:40, Qi Zheng wrote: >> Currently, the usage of pte_offset_map_nolock() can be divided into the >> following two cases: >> >> 1) After acquiring PTL, only read-only operations are performed on the >> PTE >> page. In this case, the RCU lock in pte_offset_map_nolock() will >> ensure >> that the PTE page will not be freed, and there is no need to worry >> about whether the pmd entry is modified. >> >> 2) After acquiring PTL, the pte or pmd entries may be modified. At this >> time, we need to ensure that the pmd entry has not been modified >> concurrently. >> >> To more clearing distinguish between these two cases, this commit >> introduces two new helper functions to replace pte_offset_map_nolock(). >> For 1), just rename it to pte_offset_map_ro_nolock(). For 2), in addition >> to changing the name to pte_offset_map_rw_nolock(), it also outputs the >> pmdval when successful. It is applicable for may-write cases where any >> modification operations to the page table may happen after the >> corresponding spinlock is held afterwards. But the users should make sure >> the page table is stable like checking pte_same() or checking pmd_same() >> by using the output pmdval before performing the write operations. >> >> Note: "RO" / "RW" expresses the intended semantics, not that the *kmap* >> will be read-only/read-write protected. >> >> Subsequent commits will convert pte_offset_map_nolock() into the above >> two functions one by one, and finally completely delete it. >> >> Signed-off-by: Qi Zheng<zhengqi.arch@bytedance.com> >> --- >> Documentation/mm/split_page_table_lock.rst | 7 +++ >> include/linux/mm.h | 5 +++ >> mm/pgtable-generic.c | 50 ++++++++++++++++++++++ >> 3 files changed, 62 insertions(+) >> >> diff --git a/Documentation/mm/split_page_table_lock.rst >> b/Documentation/mm/split_page_table_lock.rst >> index e4f6972eb6c04..08d0e706a32db 100644 >> --- a/Documentation/mm/split_page_table_lock.rst >> +++ b/Documentation/mm/split_page_table_lock.rst >> @@ -19,6 +19,13 @@ There are helpers to lock/unlock a table and other >> accessor functions: >> - pte_offset_map_nolock() >> maps PTE, returns pointer to PTE with pointer to its PTE table >> lock (not taken), or returns NULL if no PTE table; >> + - pte_offset_map_ro_nolock() >> + maps PTE, returns pointer to PTE with pointer to its PTE table >> + lock (not taken), or returns NULL if no PTE table; >> + - pte_offset_map_rw_nolock() >> + maps PTE, returns pointer to PTE with pointer to its PTE table >> + lock (not taken) and the value of its pmd entry, or returns NULL >> + if no PTE table; >> - pte_offset_map() >> maps PTE, returns pointer to PTE, or returns NULL if no PTE table; >> - pte_unmap() >> diff --git a/include/linux/mm.h b/include/linux/mm.h >> index a7c74a840249a..1fde9242231c9 100644 >> --- a/include/linux/mm.h >> +++ b/include/linux/mm.h >> @@ -3006,6 +3006,11 @@ static inline pte_t *pte_offset_map_lock(struct >> mm_struct *mm, pmd_t *pmd, >> pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd, >> unsigned long addr, spinlock_t **ptlp); >> +pte_t *pte_offset_map_ro_nolock(struct mm_struct *mm, pmd_t *pmd, >> + unsigned long addr, spinlock_t **ptlp); >> +pte_t *pte_offset_map_rw_nolock(struct mm_struct *mm, pmd_t *pmd, >> + unsigned long addr, pmd_t *pmdvalp, >> + spinlock_t **ptlp); >> #define pte_unmap_unlock(pte, ptl) do { \ >> spin_unlock(ptl); \ >> diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c >> index a78a4adf711ac..262b7065a5a2e 100644 >> --- a/mm/pgtable-generic.c >> +++ b/mm/pgtable-generic.c >> @@ -317,6 +317,33 @@ pte_t *pte_offset_map_nolock(struct mm_struct >> *mm, pmd_t *pmd, >> return pte; >> } >> +pte_t *pte_offset_map_ro_nolock(struct mm_struct *mm, pmd_t *pmd, >> + unsigned long addr, spinlock_t **ptlp) >> +{ >> + pmd_t pmdval; >> + pte_t *pte; >> + >> + pte = __pte_offset_map(pmd, addr, &pmdval); >> + if (likely(pte)) >> + *ptlp = pte_lockptr(mm, &pmdval); >> + return pte; >> +} >> + >> +pte_t *pte_offset_map_rw_nolock(struct mm_struct *mm, pmd_t *pmd, >> + unsigned long addr, pmd_t *pmdvalp, >> + spinlock_t **ptlp) >> +{ >> + pmd_t pmdval; >> + pte_t *pte; >> + >> + VM_WARN_ON_ONCE(!pmdvalp); >> + pte = __pte_offset_map(pmd, addr, &pmdval); >> + if (likely(pte)) >> + *ptlp = pte_lockptr(mm, &pmdval); >> + *pmdvalp = pmdval; >> + return pte; >> +} >> + >> /* >> * pte_offset_map_lock(mm, pmd, addr, ptlp), and its internal >> implementation >> * __pte_offset_map_lock() below, is usually called with the pmd >> pointer for >> @@ -356,6 +383,29 @@ pte_t *pte_offset_map_nolock(struct mm_struct >> *mm, pmd_t *pmd, >> * recheck *pmd once the lock is taken; in practice, no callsite >> needs that - >> * either the mmap_lock for write, or pte_same() check on contents, >> is enough. >> * >> + * pte_offset_map_ro_nolock(mm, pmd, addr, ptlp), above, is like >> pte_offset_map(); >> + * but when successful, it also outputs a pointer to the spinlock in >> ptlp - as >> + * pte_offset_map_lock() does, but in this case without locking it. >> This helps >> + * the caller to avoid a later pte_lockptr(mm, *pmd), which might by >> that time >> + * act on a changed *pmd: pte_offset_map_ro_nolock() provides the >> correct spinlock >> + * pointer for the page table that it returns. Even after grabbing >> the spinlock, >> + * we might be looking either at a page table that is still mapped or >> one that >> + * was unmapped and is about to get freed. But for R/O access this is >> sufficient. >> + * So it is only applicable for read-only cases where any >> modification operations >> + * to the page table are not allowed even if the corresponding >> spinlock is held >> + * afterwards. >> + * >> + * pte_offset_map_rw_nolock(mm, pmd, addr, pmdvalp, ptlp), above, is >> like >> + * pte_offset_map_ro_nolock(); but when successful, it also outputs >> the pdmval. >> + * It is applicable for may-write cases where any modification >> operations to the >> + * page table may happen after the corresponding spinlock is held >> afterwards. >> + * But the users should make sure the page table is stable like >> checking pte_same() >> + * or checking pmd_same() by using the output pmdval before >> performing the write >> + * operations. > > Now, we have two options to make sure the stability of PTE for users > of pte_offset_map_rw_nolock(), in order to ease this operation, how > about proposing a new helper (or two, one for pmd_same, another for > pte_same) like pte_lock_stability (I am not good at naming, maybe > you can) which helps users 1) hold the PTL and 2) check if the PTE is > stable and 3) return true if the PTE stable, otherwise return false. I've been trying to do this these days, but I found it was not very convenient. I introduced the following helpers: #define __PTE_STABILITY(lock) \ bool __pte_stability_##lock(pmd_t *pmd, pmd_t *orig_pmd, pte_t *pte, \ pte_t *orig_pte, spinlock_t *ptlp) \ { \ pte_spin_##lock(ptlp); \ if (orig_pte) { \ VM_WARN_ON_ONCE(pte_none(*orig_pte)); \ return pte_same(*orig_pte, ptep_get(pte)); \ } \ if (orig_pmd) { \ VM_WARN_ON_ONCE(pmd_none(*orig_pmd)); \ return pmd_same(*orig_pmd, pmdp_get_lockless(pmd)); \ } \ VM_WARN_ON_ONCE(1); \ return false; \ } __PTE_STABILITY(lock) __PTE_STABILITY(lock_nested) static inline bool pte_stability_lock(pmd_t *pmd, pmd_t *orig_pmd, pte_t *pte, pte_t *orig_pte, spinlock_t *ptlp) __acquires(ptlp) { return __pte_stability_lock(pmd, orig_pmd, pte, orig_pte, ptlp); } #ifdef CONFIG_SPLIT_PTE_PTLOCKS static inline bool pte_stability_lock_nested(pmd_t *pmd, pmd_t *orig_pmd, pte_t *pte, pte_t *orig_pte, spinlock_t *ptlp) __acquires(ptlp) { return __pte_stability_lock_nested(pmd, orig_pmd, pte, orig_pte, ptlp); } static inline void pte_stability_unlock_nested(spinlock_t *ptlp) __releases(ptlp) { spin_unlock(ptlp); } #else static inline bool pte_stability_lock_nested(pmd_t *pmd, pmd_t *orig_pmd, pte_t *pte, pte_t *orig_pte, spinlock_t *ptlp) { return true; } static inline void pte_stability_unlock_nested(spinlock_t *ptlp) { } #endif /* CONFIG_SPLIT_PTE_PTLOCKS */ and try to use them with pte_offset_map_rw_nolock() in the following functions: 1. collapse_pte_mapped_thp 2. handle_pte_fault 3. map_pte 4. move_pages_pte 5. walk_pte_range For 1, 2 and 3, the conversion is relatively simple, but 2 actually already does a pte_same() check, so it does not reduce the amount of code much. For 4, the pte_same() checks have already been done, and it is not easy to convert double_pt_lock() to use pte_stability_lock{_nested}(). For 5, it calls spin_trylock(), we should introduce another pte_stability_trylock() helper for it, but it feels unnecessary. There are not many places where pte_offset_map_rw_nolock() is called, and some places have already done pte_same() checks, so maybe open code is enough and there is no need to introduce more helper function. Thanks, Qi > > Muchun, > Thanks. > >> + * >> + * Note: "RO" / "RW" expresses the intended semantics, not that the >> *kmap* will >> + * be read-only/read-write protected. >> + * >> * Note that free_pgtables(), used after unmapping detached vmas, or >> when >> * exiting the whole mm, does not take page table lock before >> freeing a page >> * table, and may not use RCU at all: "outsiders" like khugepaged >> should avoid >
diff --git a/Documentation/mm/split_page_table_lock.rst b/Documentation/mm/split_page_table_lock.rst index e4f6972eb6c04..08d0e706a32db 100644 --- a/Documentation/mm/split_page_table_lock.rst +++ b/Documentation/mm/split_page_table_lock.rst @@ -19,6 +19,13 @@ There are helpers to lock/unlock a table and other accessor functions: - pte_offset_map_nolock() maps PTE, returns pointer to PTE with pointer to its PTE table lock (not taken), or returns NULL if no PTE table; + - pte_offset_map_ro_nolock() + maps PTE, returns pointer to PTE with pointer to its PTE table + lock (not taken), or returns NULL if no PTE table; + - pte_offset_map_rw_nolock() + maps PTE, returns pointer to PTE with pointer to its PTE table + lock (not taken) and the value of its pmd entry, or returns NULL + if no PTE table; - pte_offset_map() maps PTE, returns pointer to PTE, or returns NULL if no PTE table; - pte_unmap() diff --git a/include/linux/mm.h b/include/linux/mm.h index a7c74a840249a..1fde9242231c9 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -3006,6 +3006,11 @@ static inline pte_t *pte_offset_map_lock(struct mm_struct *mm, pmd_t *pmd, pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd, unsigned long addr, spinlock_t **ptlp); +pte_t *pte_offset_map_ro_nolock(struct mm_struct *mm, pmd_t *pmd, + unsigned long addr, spinlock_t **ptlp); +pte_t *pte_offset_map_rw_nolock(struct mm_struct *mm, pmd_t *pmd, + unsigned long addr, pmd_t *pmdvalp, + spinlock_t **ptlp); #define pte_unmap_unlock(pte, ptl) do { \ spin_unlock(ptl); \ diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c index a78a4adf711ac..262b7065a5a2e 100644 --- a/mm/pgtable-generic.c +++ b/mm/pgtable-generic.c @@ -317,6 +317,33 @@ pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd, return pte; } +pte_t *pte_offset_map_ro_nolock(struct mm_struct *mm, pmd_t *pmd, + unsigned long addr, spinlock_t **ptlp) +{ + pmd_t pmdval; + pte_t *pte; + + pte = __pte_offset_map(pmd, addr, &pmdval); + if (likely(pte)) + *ptlp = pte_lockptr(mm, &pmdval); + return pte; +} + +pte_t *pte_offset_map_rw_nolock(struct mm_struct *mm, pmd_t *pmd, + unsigned long addr, pmd_t *pmdvalp, + spinlock_t **ptlp) +{ + pmd_t pmdval; + pte_t *pte; + + VM_WARN_ON_ONCE(!pmdvalp); + pte = __pte_offset_map(pmd, addr, &pmdval); + if (likely(pte)) + *ptlp = pte_lockptr(mm, &pmdval); + *pmdvalp = pmdval; + return pte; +} + /* * pte_offset_map_lock(mm, pmd, addr, ptlp), and its internal implementation * __pte_offset_map_lock() below, is usually called with the pmd pointer for @@ -356,6 +383,29 @@ pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd, * recheck *pmd once the lock is taken; in practice, no callsite needs that - * either the mmap_lock for write, or pte_same() check on contents, is enough. * + * pte_offset_map_ro_nolock(mm, pmd, addr, ptlp), above, is like pte_offset_map(); + * but when successful, it also outputs a pointer to the spinlock in ptlp - as + * pte_offset_map_lock() does, but in this case without locking it. This helps + * the caller to avoid a later pte_lockptr(mm, *pmd), which might by that time + * act on a changed *pmd: pte_offset_map_ro_nolock() provides the correct spinlock + * pointer for the page table that it returns. Even after grabbing the spinlock, + * we might be looking either at a page table that is still mapped or one that + * was unmapped and is about to get freed. But for R/O access this is sufficient. + * So it is only applicable for read-only cases where any modification operations + * to the page table are not allowed even if the corresponding spinlock is held + * afterwards. + * + * pte_offset_map_rw_nolock(mm, pmd, addr, pmdvalp, ptlp), above, is like + * pte_offset_map_ro_nolock(); but when successful, it also outputs the pdmval. + * It is applicable for may-write cases where any modification operations to the + * page table may happen after the corresponding spinlock is held afterwards. + * But the users should make sure the page table is stable like checking pte_same() + * or checking pmd_same() by using the output pmdval before performing the write + * operations. + * + * Note: "RO" / "RW" expresses the intended semantics, not that the *kmap* will + * be read-only/read-write protected. + * * Note that free_pgtables(), used after unmapping detached vmas, or when * exiting the whole mm, does not take page table lock before freeing a page * table, and may not use RCU at all: "outsiders" like khugepaged should avoid
Currently, the usage of pte_offset_map_nolock() can be divided into the following two cases: 1) After acquiring PTL, only read-only operations are performed on the PTE page. In this case, the RCU lock in pte_offset_map_nolock() will ensure that the PTE page will not be freed, and there is no need to worry about whether the pmd entry is modified. 2) After acquiring PTL, the pte or pmd entries may be modified. At this time, we need to ensure that the pmd entry has not been modified concurrently. To more clearing distinguish between these two cases, this commit introduces two new helper functions to replace pte_offset_map_nolock(). For 1), just rename it to pte_offset_map_ro_nolock(). For 2), in addition to changing the name to pte_offset_map_rw_nolock(), it also outputs the pmdval when successful. It is applicable for may-write cases where any modification operations to the page table may happen after the corresponding spinlock is held afterwards. But the users should make sure the page table is stable like checking pte_same() or checking pmd_same() by using the output pmdval before performing the write operations. Note: "RO" / "RW" expresses the intended semantics, not that the *kmap* will be read-only/read-write protected. Subsequent commits will convert pte_offset_map_nolock() into the above two functions one by one, and finally completely delete it. Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> --- Documentation/mm/split_page_table_lock.rst | 7 +++ include/linux/mm.h | 5 +++ mm/pgtable-generic.c | 50 ++++++++++++++++++++++ 3 files changed, 62 insertions(+)