On Thu, Nov 28, 2024 at 11:53 AM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
>
> On Thu, Nov 28, 2024 at 2:26 AM David Wang <00107082@xxxxxxx> wrote:
> >
> > The initial solution for codetag adjustment during page migration
> > uses three kinds of low level plumbings, those steps can be replaced
> > by swapping tags, which only needs one kind of low level plumbing,
> > and code is more clear.
>
> This description does not explain the real issue. I would suggest
> something like:
>
> 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@xxxxxxx>
> > Link: https://lore.kernel.org/lkml/20241124074318.399027-1-00107082@xxxxxxx/
> This above Link: seems unusual. Maybe uses Closes instead like this:
>
> Closed: https://lore.kernel.org/lkml/20241124074318.399027-1-00107082@xxxxxxx/
Sorry, fat fingered. Should have been:
This above Link: seems unusual. Maybe use Closes instead like this:
Closes: https://lore.kernel.org/lkml/20241124074318.399027-1-00107082@xxxxxxx/
>
> > ---
> > include/linux/pgalloc_tag.h | 4 ++--
> > lib/alloc_tag.c | 35 +++++++++++++++++++----------------
> > mm/migrate.c | 2 +-
> > 3 files changed, 22 insertions(+), 19 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..b45efde50c40 100644
> > --- a/lib/alloc_tag.c
> > +++ b/lib/alloc_tag.c
> > @@ -189,26 +189,29 @@ 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 handles[2];
> > + union codetag_ref refs[2];
> > + struct alloc_tag *tags[2];
> > + struct folio *folios[2] = {new, old};
> > + int i;
> >
> > - tag = pgalloc_tag_get(&old->page);
> > - if (!tag)
> > - return;
> > + for (i = 0; i < 2; i++) {
> > + tags[i] = pgalloc_tag_get(&folios[i]->page);
> > + if (!tags[i])
> > + return;
> > + if (!get_page_tag_ref(&folios[i]->page, &refs[i], &handles[i]))
> > + return;
>
> If any of the above getters fail on the second cycle, you will miss
> calling put_page_tag_ref(handles[0]) and releasing the page_tag_ref
> you obtained on the first cycle. It might be cleaner to drop the use
> of arrays and use new/old.
>
> > + }
> >
> > - if (!get_page_tag_ref(&new->page, &ref, &handle))
> > - return;
> > + swap(tags[0], tags[1]);
> >
> > - /* 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);
> > + for (i = 0; i < 2; i++) {
> > + __alloc_tag_ref_set(&refs[i], tags[i]);
> > + update_page_tag_ref(handles[i], &refs[i]);
> > + put_page_tag_ref(handles[i]);
> > + }
> > }
> >
> > 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
> >
