Messages in this thread | | | Date | Thu, 9 Nov 2023 14:36:01 +0000 | From | Matthew Wilcox <> | Subject | Re: [v4 2/3] mm: Defer TLB flush by keeping both src and dst folios at migration |
| |
On Thu, Nov 09, 2023 at 01:59:07PM +0900, Byungchul Park wrote: > +++ b/include/linux/page-flags.h > @@ -136,6 +136,7 @@ enum pageflags { > PG_arch_2, > PG_arch_3, > #endif > + PG_migrc, /* Page is under migrc's control */ > __NR_PAGEFLAGS,
Yeah; no. We're out of page flags. And CXL is insufficiently compelling to add more. If you use CXL, you don't care about performance, by definition.
> @@ -589,6 +590,9 @@ TESTCLEARFLAG(Young, young, PF_ANY) > PAGEFLAG(Idle, idle, PF_ANY) > #endif > > +TESTCLEARFLAG(Migrc, migrc, PF_ANY) > +__PAGEFLAG(Migrc, migrc, PF_ANY)
Why PF_ANY?
> +/* > + * Initialize the page when allocated from buddy allocator. > + */ > +static inline void migrc_init_page(struct page *p) > +{ > + __ClearPageMigrc(p); > +}
This flag should already be clear ... ?
> +/* > + * Check if the folio is pending for TLB flush and then clear the flag. > + */ > +static inline bool migrc_unpend_if_pending(struct folio *f) > +{ > + return folio_test_clear_migrc(f); > +}
If you named the flag better, you wouldn't need this wrapper.
> +static void migrc_mark_pending(struct folio *fsrc, struct folio *fdst) > +{ > + folio_get(fsrc); > + __folio_set_migrc(fsrc); > + __folio_set_migrc(fdst); > +}
This is almost certainly unsafe. By using the non-atomic bit ops, you stand the risk of losing a simultaneous update to any other bit in this word. Like, say, someone trying to lock the folio?
> +++ b/mm/page_alloc.c > @@ -1535,6 +1535,9 @@ inline void post_alloc_hook(struct page *page, unsigned int order, > > set_page_owner(page, order, gfp_flags); > page_table_check_alloc(page, order); > + > + for (i = 0; i != 1 << order; ++i) > + migrc_init_page(page + i);
No.
| |