On Thu, Sep 12, 2024 at 01:41:28PM -0700, Yang Shi wrote:
> diff --git a/arch/arm64/mm/copypage.c b/arch/arm64/mm/copypage.c
> index a7bb20055ce0..c8687ccc2633 100644
> --- a/arch/arm64/mm/copypage.c
> +++ b/arch/arm64/mm/copypage.c
> @@ -18,17 +18,41 @@ void copy_highpage(struct page *to, struct page *from)
> {
> void *kto = page_address(to);
> void *kfrom = page_address(from);
> + struct folio *src = page_folio(from);
> + struct folio *dst = page_folio(to);
> + unsigned int i, nr_pages;
>
> copy_page(kto, kfrom);
>
> if (kasan_hw_tags_enabled())
> page_kasan_tag_reset(to);
>
> - if (system_supports_mte() && page_mte_tagged(from)) {
> - /* It's a new page, shouldn't have been tagged yet */
> - WARN_ON_ONCE(!try_page_mte_tagging(to));
> - mte_copy_page_tags(kto, kfrom);
> - set_page_mte_tagged(to);
> + if (system_supports_mte()) {
> + if (folio_test_hugetlb(src) &&
> + folio_test_hugetlb_mte_tagged(src)) {
> + if (!try_folio_hugetlb_mte_tagging(dst))
> + return;
> +
> + /*
> + * Populate tags for all subpages.
> + *
> + * Don't assume the first page is head page since
> + * huge page copy may start from any subpage.
> + */
> + nr_pages = folio_nr_pages(src);
> + for (i = 0; i < nr_pages; i++) {
> + kfrom = page_address(folio_page(src, i));
> + kto = page_address(folio_page(dst, i));
> + mte_copy_page_tags(kto, kfrom);
> + }
> + folio_set_hugetlb_mte_tagged(dst);
> + } else if (page_mte_tagged(from)) {
> + /* It's a new page, shouldn't have been tagged yet */
> + WARN_ON_ONCE(!try_page_mte_tagging(to));
> +
> + mte_copy_page_tags(kto, kfrom);
> + set_page_mte_tagged(to);
> + }
> }
> }
A nitpick here: I don't like that much indentation, so just do an early
return if !system_supports_mte() in this function.
Otherwise the patch looks fine to me. I agree with David's point on an
earlier version of this patch, the naming of these functions isn't
great. So, as per David's suggestion (at least for the first two):
folio_test_hugetlb_mte_tagged()
folio_set_hugetlb_mte_tagged()
folio_try_hugetlb_mte_tagging()
As for "try" vs "test_and_set_.*_lock", the original name was picked to
mimic spin_trylock() since this function is waiting/spinning. It's not
great but the alternative naming is closer to test_and_set_bit_lock().
This has different behaviour, it only sets a bit with acquire semantics,
no waiting/spinning.
--
Catalin
