On Thu, Nov 09, 2023 at 02:36:01PM +0000, Matthew Wilcox wrote:
> 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
I should've forced migrc to work only for 64bit arch. I missed it while
I removed kconifg for it. However, lemme try to avoid the additonal page
flag anyway if possible.
> 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?
PF_HEAD looks more fit on the purpose. I will change it to PF_HEAD.
> > +/*
> > + * 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 ... ?
That should be initialized either on allocation or on free.
> > +/*
> > + * 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.
I will.
> > +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?
fdst is not exposed yet so safe to use non-atomic in here IMHO. While..
fsrc's PG_locked is owned by the migration context and the folio has
been successfully unmapped, so I thought it'd be safe but yeah I'm not
convinced if fsrc is safe here for real. I will change it to atomic.
> > +++ 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.
I will change it.
Appreciate your feedback.
Byungchul
