lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 24 Apr 2024 15:32:11 -0700
From: Yang Shi <shy828301@...il.com>
To: Zi Yan <ziy@...dia.com>
Cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>, 
	"Matthew Wilcox (Oracle)" <willy@...radead.org>, Ryan Roberts <ryan.roberts@....com>, 
	Barry Song <21cnbao@...il.com>, David Hildenbrand <david@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] mm/rmap: do not add fully unmapped large folio to
 deferred split list

On Wed, Apr 24, 2024 at 2:10 PM Zi Yan <zi.yan@...t.com> wrote:
>
> From: Zi Yan <ziy@...dia.com>
>
> In __folio_remove_rmap(), a large folio is added to deferred split list
> if any page in a folio loses its final mapping. It is possible that
> the folio is unmapped fully, but it is unnecessary to add the folio
> to deferred split list at all. Fix it by checking folio->_nr_pages_mapped
> before adding a folio to deferred split list. If the folio is already
> on the deferred split list, it will be skipped.
>
> Commit 98046944a159 ("mm: huge_memory: add the missing
> folio_test_pmd_mappable() for THP split statistics") tried to exclude
> mTHP deferred split stats from THP_DEFERRED_SPLIT_PAGE, but it does not
> fix everything. A fully unmapped PTE-mapped order-9 THP was also added to
> deferred split list and counted as THP_DEFERRED_SPLIT_PAGE, since nr is
> 512 (non zero), level is RMAP_LEVEL_PTE, and inside deferred_split_folio()
> the order-9 folio is folio_test_pmd_mappable(). However, this miscount
> was present even earlier due to implementation, since PTEs are unmapped
> individually and first PTE unmapping adds the THP into the deferred split
> list.

Shall you mention the miscounting for mTHP too? There is another patch
series adding the counter support for mTHP.

>
> With commit b06dc281aa99 ("mm/rmap: introduce
> folio_remove_rmap_[pte|ptes|pmd]()"), kernel is able to unmap PTE-mapped
> folios in one shot without causing the miscount, hence this patch.
>
> Signed-off-by: Zi Yan <ziy@...dia.com>
> ---
>  mm/rmap.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/mm/rmap.c b/mm/rmap.c
> index a7913a454028..220ad8a83589 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1553,9 +1553,11 @@ static __always_inline void __folio_remove_rmap(struct folio *folio,
>                  * page of the folio is unmapped and at least one page
>                  * is still mapped.
>                  */
> -               if (folio_test_large(folio) && folio_test_anon(folio))
> -                       if (level == RMAP_LEVEL_PTE || nr < nr_pmdmapped)
> -                               deferred_split_folio(folio);
> +               if (folio_test_large(folio) && folio_test_anon(folio) &&
> +                   list_empty(&folio->_deferred_list) &&

Do we really need this check? deferred_split_folio() does the same
check too. Bailing out earlier sounds ok too, but there may not be too
much gain.

> +                   ((level == RMAP_LEVEL_PTE && atomic_read(mapped)) ||
> +                    (level == RMAP_LEVEL_PMD && nr < nr_pmdmapped)))

IIUC, this line is used to cover the case which has both partial
PTE-mapping and PMD-mapping, then PMD mapping is unmapped fully. IIRC
this case was not handled correctly before, the THP actually skipped
deferred split queue. If so please add some description in the commit
log.

Otherwise the patch looks good to me. Reviewed-by: Yang Shi
<shy828301@...il.com>

> +                       deferred_split_folio(folio);
>         }
>
>         /*
>
> base-commit: 2541ee5668b019c486dd3e815114130e35c1495d
> --
> 2.43.0
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ