diff mbox series

[v1,03/11] mm: simplify hugetlb and file-THP handling in __page_mapcount()

Message ID 20211217113049.23850-4-david@redhat.com (mailing list archive)
State New
Headers show
Series mm: COW fixes part 1: fix the COW security issue for THP and hugetlb | expand

Commit Message

David Hildenbrand Dec. 17, 2021, 11:30 a.m. UTC 
Let's return early for hugetlb, which really only relies on the compound
mapcount so far and does not support PageDoubleMap() yet. Use the chance
to cleanup the file-THP case to make it easier to grasp. While at it, use
head_compound_mapcount().

This is a preparation for further changes.

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 mm/util.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Nadav Amit Dec. 17, 2021, 5:16 p.m. UTC | #1 
> On Dec 17, 2021, at 3:30 AM, David Hildenbrand <david@redhat.com> wrote:
> 
> Let's return early for hugetlb, which really only relies on the compound
> mapcount so far and does not support PageDoubleMap() yet. Use the chance
> to cleanup the file-THP case to make it easier to grasp. While at it, use
> head_compound_mapcount().
> 
> This is a preparation for further changes.

It would be useful to add “no functional change intended” or something.

> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> mm/util.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/util.c b/mm/util.c
> index 741ba32a43ac..3239e75c148d 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -732,15 +732,18 @@ int __page_mapcount(struct page *page)
> {
> 	int ret;
> 
> -	ret = atomic_read(&page->_mapcount) + 1;
> +	if (PageHuge(page))
> +		return compound_mapcount(page);

Before you return, perhaps you can add an assertion like:

	VM_BUG_ON(PageDoubleMap(page));

This would be make the code clearer and would ease debugging in the
future (if support for double-map is expanded).
David Hildenbrand Dec. 17, 2021, 5:30 p.m. UTC | #2 
On 17.12.21 18:16, Nadav Amit wrote:
> 
>> On Dec 17, 2021, at 3:30 AM, David Hildenbrand <david@redhat.com> wrote:
>>
>> Let's return early for hugetlb, which really only relies on the compound
>> mapcount so far and does not support PageDoubleMap() yet. Use the chance
>> to cleanup the file-THP case to make it easier to grasp. While at it, use
>> head_compound_mapcount().
>>
>> This is a preparation for further changes.
> 
> It would be useful to add “no functional change intended” or something.

Absolutely, same applies to other "simplification" patches.

> 
>>
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> mm/util.c | 11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/util.c b/mm/util.c
>> index 741ba32a43ac..3239e75c148d 100644
>> --- a/mm/util.c
>> +++ b/mm/util.c
>> @@ -732,15 +732,18 @@ int __page_mapcount(struct page *page)
>> {
>> 	int ret;
>>
>> -	ret = atomic_read(&page->_mapcount) + 1;
>> +	if (PageHuge(page))
>> +		return compound_mapcount(page);
> 
> Before you return, perhaps you can add an assertion like:
> 
> 	VM_BUG_ON(PageDoubleMap(page));
> 
> This would be make the code clearer and would ease debugging in the
> future (if support for double-map is expanded).
> 

I'd probably have to add this to a couple of places -- and I assume
anybody working on that has to grep the kernel for use of PageDoubleMap
already.

Thanks!
Mike Kravetz Dec. 17, 2021, 6:06 p.m. UTC | #3 
On 12/17/21 03:30, David Hildenbrand wrote:
> Let's return early for hugetlb, which really only relies on the compound
> mapcount so far and does not support PageDoubleMap() yet. Use the chance

It is too early to say if hugetlb double mapping will use PageDoubleMap().
I do not think (hope) it will be necessary.  So, I think you can drop mention
of it here.

> to cleanup the file-THP case to make it easier to grasp. While at it, use
> head_compound_mapcount().
> 
> This is a preparation for further changes.
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: Mike Kravetz <mike.kravetz@oracle.com>
David Hildenbrand Dec. 17, 2021, 6:11 p.m. UTC | #4 
On 17.12.21 19:06, Mike Kravetz wrote:
> On 12/17/21 03:30, David Hildenbrand wrote:
>> Let's return early for hugetlb, which really only relies on the compound
>> mapcount so far and does not support PageDoubleMap() yet. Use the chance
> 
> It is too early to say if hugetlb double mapping will use PageDoubleMap().
> I do not think (hope) it will be necessary.  So, I think you can drop mention
> of it here.

Desires have most certainly been expressed from a couple of parties --
to PTE map huge pages :) Hopefully we'll find a way to avoid
PageDoubleMap, I agree.

Dropping the comment!

> 
>> to cleanup the file-THP case to make it easier to grasp. While at it, use
>> head_compound_mapcount().
>>
>> This is a preparation for further changes.
>>
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Acked-by: Mike Kravetz <mike.kravetz@oracle.com>
> 

Thanks!
Yang Shi Dec. 17, 2021, 7:07 p.m. UTC | #5 
On Fri, Dec 17, 2021 at 3:33 AM David Hildenbrand <david@redhat.com> wrote:
>
> Let's return early for hugetlb, which really only relies on the compound
> mapcount so far and does not support PageDoubleMap() yet. Use the chance
> to cleanup the file-THP case to make it easier to grasp. While at it, use
> head_compound_mapcount().
>
> This is a preparation for further changes.
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Yang Shi <shy828301@gmail.com>

> ---
>  mm/util.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/mm/util.c b/mm/util.c
> index 741ba32a43ac..3239e75c148d 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -732,15 +732,18 @@ int __page_mapcount(struct page *page)
>  {
>         int ret;
>
> -       ret = atomic_read(&page->_mapcount) + 1;
> +       if (PageHuge(page))
> +               return compound_mapcount(page);
>         /*
>          * For file THP page->_mapcount contains total number of mapping
>          * of the page: no need to look into compound_mapcount.
>          */
> -       if (!PageAnon(page) && !PageHuge(page))
> -               return ret;
> +       if (!PageAnon(page))
> +               return atomic_read(&page->_mapcount) + 1;
> +
> +       ret = atomic_read(&page->_mapcount) + 1;
>         page = compound_head(page);
> -       ret += atomic_read(compound_mapcount_ptr(page)) + 1;
> +       ret += head_compound_mapcount(page);
>         if (PageDoubleMap(page))
>                 ret--;
>         return ret;
> --
> 2.31.1
>
Kirill A. Shutemov Dec. 18, 2021, 2:31 p.m. UTC | #6 
On Fri, Dec 17, 2021 at 12:30:41PM +0100, David Hildenbrand wrote:
> Let's return early for hugetlb, which really only relies on the compound
> mapcount so far and does not support PageDoubleMap() yet. Use the chance
> to cleanup the file-THP case to make it easier to grasp. While at it, use
> head_compound_mapcount().
> 
> This is a preparation for further changes.
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  mm/util.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/util.c b/mm/util.c
> index 741ba32a43ac..3239e75c148d 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -732,15 +732,18 @@ int __page_mapcount(struct page *page)
>  {
>  	int ret;
>  
> -	ret = atomic_read(&page->_mapcount) + 1;
> +	if (PageHuge(page))
> +		return compound_mapcount(page);

It would be nice to make PageHuge() inlinable first. It's a shame the we
need to have to do a function call for PageHuge() check.

Otherwise, looks good:

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
diff mbox series

Patch

diff --git a/mm/util.c b/mm/util.c
index 741ba32a43ac..3239e75c148d 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -732,15 +732,18 @@  int __page_mapcount(struct page *page)
 {
 	int ret;
 
-	ret = atomic_read(&page->_mapcount) + 1;
+	if (PageHuge(page))
+		return compound_mapcount(page);
 	/*
 	 * For file THP page->_mapcount contains total number of mapping
 	 * of the page: no need to look into compound_mapcount.
 	 */
-	if (!PageAnon(page) && !PageHuge(page))
-		return ret;
+	if (!PageAnon(page))
+		return atomic_read(&page->_mapcount) + 1;
+
+	ret = atomic_read(&page->_mapcount) + 1;
 	page = compound_head(page);
-	ret += atomic_read(compound_mapcount_ptr(page)) + 1;
+	ret += head_compound_mapcount(page);
 	if (PageDoubleMap(page))
 		ret--;
 	return ret;