linux-kernel - Re: [PATCH v2] mm/codetag: swap tags when migrate pages

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]
Message-ID: <CAJuCfpGxU8u4X0x1n3Sen206Xpi1ubBK2gyCgYinPV_NFFJNPw@mail.gmail.com>
Date: Fri, 29 Nov 2024 14:59:26 -0800
From: Suren Baghdasaryan <surenb@...gle.com>
To: David Wang <00107082@....com>
Cc: yuzhao@...gle.com, kent.overstreet@...ux.dev, akpm@...ux-foundation.org, 
	linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] mm/codetag: swap tags when migrate pages

On Thu, Nov 28, 2024 at 6:52 PM David Wang <00107082@....com> wrote:
>
> Current solution to adjust codetag references during page migration is
> done in 3 steps:
> 1. sets the codetag reference of the old page as empty (not pointing
> to any codetag);
> 2. subtracts counters of the new page to compensate for its own allocation;
> 3. sets codetag reference of the new page to point to the codetag of
> the old page.
> This does not work if CONFIG_MEM_ALLOC_PROFILING_DEBUG=n because
> set_codetag_empty() becomes NOOP. Instead, let's simply swap codetag
> references so that the new page is referencing the old codetag and the
> old page is referencing the new codetag. This way accounting stays
> valid and the logic makes more sense.
>
> Fixes: e0a955bf7f61 ("mm/codetag: add pgalloc_tag_copy()")
> Signed-off-by: David Wang <00107082@....com>
> Closes: https://lore.kernel.org/lkml/20241124074318.399027-1-00107082@163.com/
> Acked-by: Suren Baghdasaryan <surenb@...gle.com>
> Suggested-by: Suren Baghdasaryan <surenb@...gle.com>

Thanks! This looks fine to me but IIRC I did not Ack this patch
before. In the future please do not add Acked-by until you get an
explicit Ack. That said,

Acked-by: Suren Baghdasaryan <surenb@...gle.com>

> ---
>  include/linux/pgalloc_tag.h |  4 ++--
>  lib/alloc_tag.c             | 36 ++++++++++++++++++++++--------------
>  mm/migrate.c                |  2 +-
>  3 files changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/pgalloc_tag.h b/include/linux/pgalloc_tag.h
> index 0e43ab653ab6..3469c4b20105 100644
> --- a/include/linux/pgalloc_tag.h
> +++ b/include/linux/pgalloc_tag.h
> @@ -231,7 +231,7 @@ static inline void pgalloc_tag_sub_pages(struct alloc_tag *tag, unsigned int nr)
>  }
>
>  void pgalloc_tag_split(struct folio *folio, int old_order, int new_order);
> -void pgalloc_tag_copy(struct folio *new, struct folio *old);
> +void pgalloc_tag_swap(struct folio *new, struct folio *old);
>
>  void __init alloc_tag_sec_init(void);
>
> @@ -245,7 +245,7 @@ static inline struct alloc_tag *pgalloc_tag_get(struct page *page) { return NULL
>  static inline void pgalloc_tag_sub_pages(struct alloc_tag *tag, unsigned int nr) {}
>  static inline void alloc_tag_sec_init(void) {}
>  static inline void pgalloc_tag_split(struct folio *folio, int old_order, int new_order) {}
> -static inline void pgalloc_tag_copy(struct folio *new, struct folio *old) {}
> +static inline void pgalloc_tag_swap(struct folio *new, struct folio *old) {}
>
>  #endif /* CONFIG_MEM_ALLOC_PROFILING */
>
> diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
> index 2414a7ee7ec7..35f7560a309a 100644
> --- a/lib/alloc_tag.c
> +++ b/lib/alloc_tag.c
> @@ -189,26 +189,34 @@ void pgalloc_tag_split(struct folio *folio, int old_order, int new_order)
>         }
>  }
>
> -void pgalloc_tag_copy(struct folio *new, struct folio *old)
> +void pgalloc_tag_swap(struct folio *new, struct folio *old)
>  {
> -       union pgtag_ref_handle handle;
> -       union codetag_ref ref;
> -       struct alloc_tag *tag;
> +       union pgtag_ref_handle handle_old, handle_new;
> +       union codetag_ref ref_old, ref_new;
> +       struct alloc_tag *tag_old, *tag_new;
>
> -       tag = pgalloc_tag_get(&old->page);
> -       if (!tag)
> +       tag_old = pgalloc_tag_get(&old->page);
> +       if (!tag_old)
> +               return;
> +       tag_new = pgalloc_tag_get(&new->page);
> +       if (!tag_new)
>                 return;
>
> -       if (!get_page_tag_ref(&new->page, &ref, &handle))
> +       if (!get_page_tag_ref(&old->page, &ref_old, &handle_old))
>                 return;
> +       if (!get_page_tag_ref(&new->page, &ref_new, &handle_new)) {
> +               put_page_tag_ref(handle_old);
> +               return;
> +       }
> +
> +       /* swap tags */
> +       __alloc_tag_ref_set(&ref_old, tag_new);
> +       update_page_tag_ref(handle_old, &ref_old);
> +       __alloc_tag_ref_set(&ref_new, tag_old);
> +       update_page_tag_ref(handle_new, &ref_new);
>
> -       /* Clear the old ref to the original allocation tag. */
> -       clear_page_tag_ref(&old->page);
> -       /* Decrement the counters of the tag on get_new_folio. */
> -       alloc_tag_sub(&ref, folio_size(new));
> -       __alloc_tag_ref_set(&ref, tag);
> -       update_page_tag_ref(handle, &ref);
> -       put_page_tag_ref(handle);
> +       put_page_tag_ref(handle_old);
> +       put_page_tag_ref(handle_new);
>  }
>
>  static void shutdown_mem_profiling(bool remove_file)
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 2ce6b4b814df..cc68583c86f9 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -745,7 +745,7 @@ void folio_migrate_flags(struct folio *newfolio, struct folio *folio)
>                 folio_set_readahead(newfolio);
>
>         folio_copy_owner(newfolio, folio);
> -       pgalloc_tag_copy(newfolio, folio);
> +       pgalloc_tag_swap(newfolio, folio);
>
>         mem_cgroup_migrate(folio, newfolio);
>  }
> --
> 2.39.2
>