[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6edb1aa3-ea72-49fd-9aaf-189ad6b61ee5@bytedance.com>
Date: Mon, 29 Jul 2024 16:52:44 +0800
From: Qi Zheng <zhengqi.arch@...edance.com>
To: David Hildenbrand <david@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
Andrew Morton <akpm@...ux-foundation.org>,
Muchun Song <muchun.song@...ux.dev>, Peter Xu <peterx@...hat.com>,
Oscar Salvador <osalvador@...e.de>
Subject: Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer
On 2024/7/29 16:46, David Hildenbrand wrote:
> On 29.07.24 09:48, Qi Zheng wrote:
>>
>>
>> On 2024/7/26 02:39, David Hildenbrand wrote:
>>> pte_lockptr() is the only *_lockptr() function that doesn't consume
>>> what would be expected: it consumes a pmd_t pointer instead of a pte_t
>>> pointer.
>>>
>>> Let's change that. The two callers in pgtable-generic.c are easily
>>> adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
>>> pte_offset_map_nolock() to obtain the lock, even though we won't
>>> actually
>>> be traversing the page table.
>>>
>>> This makes the code more similar to the other variants and avoids other
>>> hacks to make the new pte_lockptr() version happy. pte_lockptr() users
>>> reside now only in pgtable-generic.c.
>>>
>>> Maybe, using pte_offset_map_nolock() is the right thing to do because
>>> the PTE table could have been removed in the meantime? At least it
>>> sounds
>>> more future proof if we ever have other means of page table reclaim.
>>
>> Agree, this helps us recheck the pmd entry.
>>
>>>
>>> It's not quite clear if holding the PTE table lock is really required:
>>> what if someone else obtains the lock just after we unlock it? But we'll
>>> leave that as is for now, maybe there are good reasons.
>>>
>>> This is a preparation for adapting hugetlb page table locking logic to
>>> take the same locks as core-mm page table walkers would.
>>>
>>> Signed-off-by: David Hildenbrand <david@...hat.com>
>>> ---
>>> include/linux/mm.h | 7 ++++---
>>> mm/khugepaged.c | 21 +++++++++++++++------
>>> mm/pgtable-generic.c | 4 ++--
>>> 3 files changed, 21 insertions(+), 11 deletions(-)
>>
>> Since pte_lockptr() no longer has a pmd parameter, it is best to modify
>> the comments above __pte_offset_map_lock() as well:
>>
>> ```
>> This helps the caller to avoid a later pte_lockptr(mm, *pmd), which
>> might by that time act on a changed *pmd ...
>> ```
>
> Right, thanks a lot for the review!
>
> The following on top;
>
>
> From a46b16aa9bfa02ffb425d364d7f00129a8e803ad Mon Sep 17 00:00:00 2001
> From: David Hildenbrand <david@...hat.com>
> Date: Mon, 29 Jul 2024 10:43:34 +0200
> Subject: [PATCH] fixup: mm: let pte_lockptr() consume a pte_t pointer
>
> Let's adjust the comment, passing a pte to pte_lockptr() and dropping
> a detail about changed *pmd, which no longer applies.
>
> Signed-off-by: David Hildenbrand <david@...hat.com>
> ---
> mm/pgtable-generic.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
> index 13a7705df3f87..f17465b43d344 100644
> --- a/mm/pgtable-generic.c
> +++ b/mm/pgtable-generic.c
> @@ -350,11 +350,11 @@ pte_t *pte_offset_map_nolock(struct mm_struct *mm,
> pmd_t *pmd,
> * pte_offset_map_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_nolock() provides the correct
> spinlock
> - * pointer for the page table that it returns. In principle, the
> caller should
> - * 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.
> + * the caller to avoid a later pte_lockptr(mm, pte):
> pte_offset_map_nolock()
> + * provides the correct spinlock pointer for the page table that it
> returns.
> + * In principle, the caller should 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.
> *
> * 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
LGTM. Thanks!
Powered by blists - more mailing lists