| Message ID | 20201020085940.13875-6-sjpark@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce Data Access MONitor (DAMON) | expand |
On Tue, Oct 20, 2020 at 2:04 AM SeongJae Park <sjpark@amazon.com> wrote: > > From: SeongJae Park <sjpark@amazon.de> > > PG_idle and PG_young allows the two PTE Accessed bit users, > IDLE_PAGE_TRACKING and the reclaim logic concurrently work while don't > interfere each other. That is, when they need to clear the Accessed > bit, they set PG_young Only PG_young bit > and PG_idle to represent the previous state of > the bit, respectively. And when they need to read the bit, if the bit > is cleared, they further read the PG_young Again only PG_young bit. PG_idle bit is only read (and set) by the page idle tracking code and it can be cleared by others (reclaim or file access). > and PG_idle, respectively, to > know whether the other has cleared the bit meanwhile or not. > > We could add another page flag and extend the mechanism to use the flag > if we need to add another concurrent PTE Accessed bit user subsystem. > However, it would be only waste the space. Instead, if the new > subsystem is mutually exclusive with IDLE_PAGE_TRACKING, it could simply > reuse the PG_idle flag. However, it's impossible because the flags are > dependent on IDLE_PAGE_TRACKING. > > To allow such reuse of the flags, this commit separates the PG_young and > PG_idle flag logic from IDLE_PAGE_TRACKING and introduces new kernel > config, 'PAGE_IDLE_FLAG'. Hence, if !IDLE_PAGE_TRACKING and > IDLE_PAGE_FLAG, a new subsystem would be able to reuse PG_idle. > > In the next commit, DAMON's reference implementation of the virtual > memory address space monitoring primitives will use it. > > Signed-off-by: SeongJae Park <sjpark@amazon.de> > --- > include/linux/page-flags.h | 4 ++-- > include/linux/page_ext.h | 2 +- > include/linux/page_idle.h | 6 +++--- > include/trace/events/mmflags.h | 2 +- > mm/Kconfig | 8 ++++++++ > mm/page_ext.c | 12 +++++++++++- > mm/page_idle.c | 10 ---------- > 7 files changed, 26 insertions(+), 18 deletions(-) > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > index 6be1aa559b1e..7736d290bb61 100644 > --- a/include/linux/page-flags.h > +++ b/include/linux/page-flags.h > @@ -132,7 +132,7 @@ enum pageflags { > #ifdef CONFIG_MEMORY_FAILURE > PG_hwpoison, /* hardware poisoned page. Don't touch */ > #endif > -#if defined(CONFIG_IDLE_PAGE_TRACKING) && defined(CONFIG_64BIT) > +#if defined(CONFIG_PAGE_IDLE_FLAG) && defined(CONFIG_64BIT) > PG_young, > PG_idle, > #endif > @@ -432,7 +432,7 @@ static inline bool set_hwpoison_free_buddy_page(struct page *page) > #define __PG_HWPOISON 0 > #endif > > -#if defined(CONFIG_IDLE_PAGE_TRACKING) && defined(CONFIG_64BIT) > +#if defined(CONFIG_PAGE_IDLE_FLAG) && defined(CONFIG_64BIT) > TESTPAGEFLAG(Young, young, PF_ANY) > SETPAGEFLAG(Young, young, PF_ANY) > TESTCLEARFLAG(Young, young, PF_ANY) > diff --git a/include/linux/page_ext.h b/include/linux/page_ext.h > index cfce186f0c4e..c9cbc9756011 100644 > --- a/include/linux/page_ext.h > +++ b/include/linux/page_ext.h > @@ -19,7 +19,7 @@ struct page_ext_operations { > enum page_ext_flags { > PAGE_EXT_OWNER, > PAGE_EXT_OWNER_ALLOCATED, > -#if defined(CONFIG_IDLE_PAGE_TRACKING) && !defined(CONFIG_64BIT) > +#if defined(CONFIG_PAGE_IDLE_FLAG) && !defined(CONFIG_64BIT) > PAGE_EXT_YOUNG, > PAGE_EXT_IDLE, > #endif > diff --git a/include/linux/page_idle.h b/include/linux/page_idle.h > index 1e894d34bdce..d8a6aecf99cb 100644 > --- a/include/linux/page_idle.h > +++ b/include/linux/page_idle.h > @@ -6,7 +6,7 @@ > #include <linux/page-flags.h> > #include <linux/page_ext.h> > > -#ifdef CONFIG_IDLE_PAGE_TRACKING > +#ifdef CONFIG_PAGE_IDLE_FLAG > > #ifdef CONFIG_64BIT > static inline bool page_is_young(struct page *page) > @@ -106,7 +106,7 @@ static inline void clear_page_idle(struct page *page) > } > #endif /* CONFIG_64BIT */ > > -#else /* !CONFIG_IDLE_PAGE_TRACKING */ > +#else /* !CONFIG_PAGE_IDLE_FLAG */ > > static inline bool page_is_young(struct page *page) > { > @@ -135,6 +135,6 @@ static inline void clear_page_idle(struct page *page) > { > } > > -#endif /* CONFIG_IDLE_PAGE_TRACKING */ > +#endif /* CONFIG_PAGE_IDLE_FLAG */ > > #endif /* _LINUX_MM_PAGE_IDLE_H */ > diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h > index 5fb752034386..4d182c32071b 100644 > --- a/include/trace/events/mmflags.h > +++ b/include/trace/events/mmflags.h > @@ -73,7 +73,7 @@ > #define IF_HAVE_PG_HWPOISON(flag,string) > #endif > > -#if defined(CONFIG_IDLE_PAGE_TRACKING) && defined(CONFIG_64BIT) > +#if defined(CONFIG_PAGE_IDLE_FLAG) && defined(CONFIG_64BIT) > #define IF_HAVE_PG_IDLE(flag,string) ,{1UL << flag, string} > #else > #define IF_HAVE_PG_IDLE(flag,string) > diff --git a/mm/Kconfig b/mm/Kconfig > index 19fe2251c87a..044317ef9143 100644 > --- a/mm/Kconfig > +++ b/mm/Kconfig > @@ -761,10 +761,18 @@ config DEFERRED_STRUCT_PAGE_INIT > lifetime of the system until these kthreads finish the > initialisation. > > +config PAGE_IDLE_FLAG > + bool "Add PG_idle and PG_young flags" > + help > + This feature adds PG_idle and PG_young flags in 'struct page'. PTE > + Accessed bit writers can set the state of the bit in the flags to let > + other PTE Accessed bit readers don't disturbed. > + > config IDLE_PAGE_TRACKING > bool "Enable idle page tracking" > depends on SYSFS && MMU > select PAGE_EXTENSION if !64BIT > + select PAGE_IDLE_FLAG > help > This feature allows to estimate the amount of user pages that have > not been touched during a given period of time. This information can > diff --git a/mm/page_ext.c b/mm/page_ext.c > index a3616f7a0e9e..f9a6ff65ac0a 100644 > --- a/mm/page_ext.c > +++ b/mm/page_ext.c > @@ -58,11 +58,21 @@ > * can utilize this callback to initialize the state of it correctly. > */ > Is there a need to move the following code in this patch? > +#if defined(CONFIG_PAGE_IDLE_FLAG) && !defined(CONFIG_64BIT) > +static bool need_page_idle(void) > +{ > + return true; > +} > +struct page_ext_operations page_idle_ops = { > + .need = need_page_idle, > +}; > +#endif > + > static struct page_ext_operations *page_ext_ops[] = { > #ifdef CONFIG_PAGE_OWNER > &page_owner_ops, > #endif > -#if defined(CONFIG_IDLE_PAGE_TRACKING) && !defined(CONFIG_64BIT) > +#if defined(CONFIG_PAGE_IDLE_FLAG) && !defined(CONFIG_64BIT) > &page_idle_ops, > #endif > }; > diff --git a/mm/page_idle.c b/mm/page_idle.c > index 057c61df12db..144fb4ed961d 100644 > --- a/mm/page_idle.c > +++ b/mm/page_idle.c > @@ -211,16 +211,6 @@ static const struct attribute_group page_idle_attr_group = { > .name = "page_idle", > }; > > -#ifndef CONFIG_64BIT > -static bool need_page_idle(void) > -{ > - return true; > -} > -struct page_ext_operations page_idle_ops = { > - .need = need_page_idle, > -}; > -#endif > - > static int __init page_idle_init(void) > { > int err; > -- > 2.17.1 > Overall this patch looks good to me.
On Wed, 25 Nov 2020 07:30:06 -0800 Shakeel Butt <shakeelb@google.com> wrote: > On Tue, Oct 20, 2020 at 2:04 AM SeongJae Park <sjpark@amazon.com> wrote: > > > > From: SeongJae Park <sjpark@amazon.de> > > > > PG_idle and PG_young allows the two PTE Accessed bit users, > > IDLE_PAGE_TRACKING and the reclaim logic concurrently work while don't > > interfere each other. That is, when they need to clear the Accessed > > bit, they set PG_young > > Only PG_young bit Oops, right. Maybe I was out of my mind while writing this. Thank you for correcting this. > > > and PG_idle to represent the previous state of > > the bit, respectively. And when they need to read the bit, if the bit > > is cleared, they further read the PG_young > > Again only PG_young bit. Sure. > > PG_idle bit is only read (and set) by the page idle tracking code and > it can be cleared by others (reclaim or file access). > > > and PG_idle, respectively, to > > know whether the other has cleared the bit meanwhile or not. > > > > We could add another page flag and extend the mechanism to use the flag > > if we need to add another concurrent PTE Accessed bit user subsystem. > > However, it would be only waste the space. Instead, if the new > > subsystem is mutually exclusive with IDLE_PAGE_TRACKING, it could simply > > reuse the PG_idle flag. However, it's impossible because the flags are > > dependent on IDLE_PAGE_TRACKING. > > > > To allow such reuse of the flags, this commit separates the PG_young and > > PG_idle flag logic from IDLE_PAGE_TRACKING and introduces new kernel > > config, 'PAGE_IDLE_FLAG'. Hence, if !IDLE_PAGE_TRACKING and > > IDLE_PAGE_FLAG, a new subsystem would be able to reuse PG_idle. > > > > In the next commit, DAMON's reference implementation of the virtual > > memory address space monitoring primitives will use it. > > > > Signed-off-by: SeongJae Park <sjpark@amazon.de> > > --- > > include/linux/page-flags.h | 4 ++-- > > include/linux/page_ext.h | 2 +- > > include/linux/page_idle.h | 6 +++--- > > include/trace/events/mmflags.h | 2 +- > > mm/Kconfig | 8 ++++++++ > > mm/page_ext.c | 12 +++++++++++- > > mm/page_idle.c | 10 ---------- > > 7 files changed, 26 insertions(+), 18 deletions(-) > > > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > > index 6be1aa559b1e..7736d290bb61 100644 > > --- a/include/linux/page-flags.h > > +++ b/include/linux/page-flags.h > > @@ -132,7 +132,7 @@ enum pageflags { > > #ifdef CONFIG_MEMORY_FAILURE > > PG_hwpoison, /* hardware poisoned page. Don't touch */ > > #endif > > -#if defined(CONFIG_IDLE_PAGE_TRACKING) && defined(CONFIG_64BIT) > > +#if defined(CONFIG_PAGE_IDLE_FLAG) && defined(CONFIG_64BIT) > > PG_young, > > PG_idle, > > #endif > > @@ -432,7 +432,7 @@ static inline bool set_hwpoison_free_buddy_page(struct page *page) > > #define __PG_HWPOISON 0 > > #endif > > > > -#if defined(CONFIG_IDLE_PAGE_TRACKING) && defined(CONFIG_64BIT) > > +#if defined(CONFIG_PAGE_IDLE_FLAG) && defined(CONFIG_64BIT) > > TESTPAGEFLAG(Young, young, PF_ANY) > > SETPAGEFLAG(Young, young, PF_ANY) > > TESTCLEARFLAG(Young, young, PF_ANY) > > diff --git a/include/linux/page_ext.h b/include/linux/page_ext.h > > index cfce186f0c4e..c9cbc9756011 100644 > > --- a/include/linux/page_ext.h > > +++ b/include/linux/page_ext.h > > @@ -19,7 +19,7 @@ struct page_ext_operations { > > enum page_ext_flags { > > PAGE_EXT_OWNER, > > PAGE_EXT_OWNER_ALLOCATED, > > -#if defined(CONFIG_IDLE_PAGE_TRACKING) && !defined(CONFIG_64BIT) > > +#if defined(CONFIG_PAGE_IDLE_FLAG) && !defined(CONFIG_64BIT) > > PAGE_EXT_YOUNG, > > PAGE_EXT_IDLE, > > #endif > > diff --git a/include/linux/page_idle.h b/include/linux/page_idle.h > > index 1e894d34bdce..d8a6aecf99cb 100644 > > --- a/include/linux/page_idle.h > > +++ b/include/linux/page_idle.h > > @@ -6,7 +6,7 @@ > > #include <linux/page-flags.h> > > #include <linux/page_ext.h> > > > > -#ifdef CONFIG_IDLE_PAGE_TRACKING > > +#ifdef CONFIG_PAGE_IDLE_FLAG > > > > #ifdef CONFIG_64BIT > > static inline bool page_is_young(struct page *page) > > @@ -106,7 +106,7 @@ static inline void clear_page_idle(struct page *page) > > } > > #endif /* CONFIG_64BIT */ > > > > -#else /* !CONFIG_IDLE_PAGE_TRACKING */ > > +#else /* !CONFIG_PAGE_IDLE_FLAG */ > > > > static inline bool page_is_young(struct page *page) > > { > > @@ -135,6 +135,6 @@ static inline void clear_page_idle(struct page *page) > > { > > } > > > > -#endif /* CONFIG_IDLE_PAGE_TRACKING */ > > +#endif /* CONFIG_PAGE_IDLE_FLAG */ > > > > #endif /* _LINUX_MM_PAGE_IDLE_H */ > > diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h > > index 5fb752034386..4d182c32071b 100644 > > --- a/include/trace/events/mmflags.h > > +++ b/include/trace/events/mmflags.h > > @@ -73,7 +73,7 @@ > > #define IF_HAVE_PG_HWPOISON(flag,string) > > #endif > > > > -#if defined(CONFIG_IDLE_PAGE_TRACKING) && defined(CONFIG_64BIT) > > +#if defined(CONFIG_PAGE_IDLE_FLAG) && defined(CONFIG_64BIT) > > #define IF_HAVE_PG_IDLE(flag,string) ,{1UL << flag, string} > > #else > > #define IF_HAVE_PG_IDLE(flag,string) > > diff --git a/mm/Kconfig b/mm/Kconfig > > index 19fe2251c87a..044317ef9143 100644 > > --- a/mm/Kconfig > > +++ b/mm/Kconfig > > @@ -761,10 +761,18 @@ config DEFERRED_STRUCT_PAGE_INIT > > lifetime of the system until these kthreads finish the > > initialisation. > > > > +config PAGE_IDLE_FLAG > > + bool "Add PG_idle and PG_young flags" > > + help > > + This feature adds PG_idle and PG_young flags in 'struct page'. PTE > > + Accessed bit writers can set the state of the bit in the flags to let > > + other PTE Accessed bit readers don't disturbed. > > + > > config IDLE_PAGE_TRACKING > > bool "Enable idle page tracking" > > depends on SYSFS && MMU > > select PAGE_EXTENSION if !64BIT > > + select PAGE_IDLE_FLAG > > help > > This feature allows to estimate the amount of user pages that have > > not been touched during a given period of time. This information can > > diff --git a/mm/page_ext.c b/mm/page_ext.c > > index a3616f7a0e9e..f9a6ff65ac0a 100644 > > --- a/mm/page_ext.c > > +++ b/mm/page_ext.c > > @@ -58,11 +58,21 @@ > > * can utilize this callback to initialize the state of it correctly. > > */ > > > > Is there a need to move the following code in this patch? After this patchset, someone would turn CONFIG_PAGE_IDLE_FLAG on but CONFIG_IDLE_PAGE_TRACKING. In that case, the build will fail because page_idle.c will not be compiled. Because below code is used by page_ext.c only, I think moving into here is ok. > > > > +#if defined(CONFIG_PAGE_IDLE_FLAG) && !defined(CONFIG_64BIT) > > +static bool need_page_idle(void) > > +{ > > + return true; > > +} > > +struct page_ext_operations page_idle_ops = { > > + .need = need_page_idle, > > +}; > > +#endif > > + > > static struct page_ext_operations *page_ext_ops[] = { > > #ifdef CONFIG_PAGE_OWNER > > &page_owner_ops, > > #endif > > -#if defined(CONFIG_IDLE_PAGE_TRACKING) && !defined(CONFIG_64BIT) > > +#if defined(CONFIG_PAGE_IDLE_FLAG) && !defined(CONFIG_64BIT) > > &page_idle_ops, > > #endif > > }; > > diff --git a/mm/page_idle.c b/mm/page_idle.c > > index 057c61df12db..144fb4ed961d 100644 > > --- a/mm/page_idle.c > > +++ b/mm/page_idle.c > > @@ -211,16 +211,6 @@ static const struct attribute_group page_idle_attr_group = { > > .name = "page_idle", > > }; > > > > -#ifndef CONFIG_64BIT > > -static bool need_page_idle(void) > > -{ > > - return true; > > -} > > -struct page_ext_operations page_idle_ops = { > > - .need = need_page_idle, > > -}; > > -#endif > > - > > static int __init page_idle_init(void) > > { > > int err; > > -- > > 2.17.1 > > > > Overall this patch looks good to me. Appreciate! Thanks, SeongJae Park
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h index 6be1aa559b1e..7736d290bb61 100644 --- a/include/linux/page-flags.h +++ b/include/linux/page-flags.h @@ -132,7 +132,7 @@ enum pageflags { #ifdef CONFIG_MEMORY_FAILURE PG_hwpoison, /* hardware poisoned page. Don't touch */ #endif -#if defined(CONFIG_IDLE_PAGE_TRACKING) && defined(CONFIG_64BIT) +#if defined(CONFIG_PAGE_IDLE_FLAG) && defined(CONFIG_64BIT) PG_young, PG_idle, #endif @@ -432,7 +432,7 @@ static inline bool set_hwpoison_free_buddy_page(struct page *page) #define __PG_HWPOISON 0 #endif -#if defined(CONFIG_IDLE_PAGE_TRACKING) && defined(CONFIG_64BIT) +#if defined(CONFIG_PAGE_IDLE_FLAG) && defined(CONFIG_64BIT) TESTPAGEFLAG(Young, young, PF_ANY) SETPAGEFLAG(Young, young, PF_ANY) TESTCLEARFLAG(Young, young, PF_ANY) diff --git a/include/linux/page_ext.h b/include/linux/page_ext.h index cfce186f0c4e..c9cbc9756011 100644 --- a/include/linux/page_ext.h +++ b/include/linux/page_ext.h @@ -19,7 +19,7 @@ struct page_ext_operations { enum page_ext_flags { PAGE_EXT_OWNER, PAGE_EXT_OWNER_ALLOCATED, -#if defined(CONFIG_IDLE_PAGE_TRACKING) && !defined(CONFIG_64BIT) +#if defined(CONFIG_PAGE_IDLE_FLAG) && !defined(CONFIG_64BIT) PAGE_EXT_YOUNG, PAGE_EXT_IDLE, #endif diff --git a/include/linux/page_idle.h b/include/linux/page_idle.h index 1e894d34bdce..d8a6aecf99cb 100644 --- a/include/linux/page_idle.h +++ b/include/linux/page_idle.h @@ -6,7 +6,7 @@ #include <linux/page-flags.h> #include <linux/page_ext.h> -#ifdef CONFIG_IDLE_PAGE_TRACKING +#ifdef CONFIG_PAGE_IDLE_FLAG #ifdef CONFIG_64BIT static inline bool page_is_young(struct page *page) @@ -106,7 +106,7 @@ static inline void clear_page_idle(struct page *page) } #endif /* CONFIG_64BIT */ -#else /* !CONFIG_IDLE_PAGE_TRACKING */ +#else /* !CONFIG_PAGE_IDLE_FLAG */ static inline bool page_is_young(struct page *page) { @@ -135,6 +135,6 @@ static inline void clear_page_idle(struct page *page) { } -#endif /* CONFIG_IDLE_PAGE_TRACKING */ +#endif /* CONFIG_PAGE_IDLE_FLAG */ #endif /* _LINUX_MM_PAGE_IDLE_H */ diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h index 5fb752034386..4d182c32071b 100644 --- a/include/trace/events/mmflags.h +++ b/include/trace/events/mmflags.h @@ -73,7 +73,7 @@ #define IF_HAVE_PG_HWPOISON(flag,string) #endif -#if defined(CONFIG_IDLE_PAGE_TRACKING) && defined(CONFIG_64BIT) +#if defined(CONFIG_PAGE_IDLE_FLAG) && defined(CONFIG_64BIT) #define IF_HAVE_PG_IDLE(flag,string) ,{1UL << flag, string} #else #define IF_HAVE_PG_IDLE(flag,string) diff --git a/mm/Kconfig b/mm/Kconfig index 19fe2251c87a..044317ef9143 100644 --- a/mm/Kconfig +++ b/mm/Kconfig @@ -761,10 +761,18 @@ config DEFERRED_STRUCT_PAGE_INIT lifetime of the system until these kthreads finish the initialisation. +config PAGE_IDLE_FLAG + bool "Add PG_idle and PG_young flags" + help + This feature adds PG_idle and PG_young flags in 'struct page'. PTE + Accessed bit writers can set the state of the bit in the flags to let + other PTE Accessed bit readers don't disturbed. + config IDLE_PAGE_TRACKING bool "Enable idle page tracking" depends on SYSFS && MMU select PAGE_EXTENSION if !64BIT + select PAGE_IDLE_FLAG help This feature allows to estimate the amount of user pages that have not been touched during a given period of time. This information can diff --git a/mm/page_ext.c b/mm/page_ext.c index a3616f7a0e9e..f9a6ff65ac0a 100644 --- a/mm/page_ext.c +++ b/mm/page_ext.c @@ -58,11 +58,21 @@ * can utilize this callback to initialize the state of it correctly. */ +#if defined(CONFIG_PAGE_IDLE_FLAG) && !defined(CONFIG_64BIT) +static bool need_page_idle(void) +{ + return true; +} +struct page_ext_operations page_idle_ops = { + .need = need_page_idle, +}; +#endif + static struct page_ext_operations *page_ext_ops[] = { #ifdef CONFIG_PAGE_OWNER &page_owner_ops, #endif -#if defined(CONFIG_IDLE_PAGE_TRACKING) && !defined(CONFIG_64BIT) +#if defined(CONFIG_PAGE_IDLE_FLAG) && !defined(CONFIG_64BIT) &page_idle_ops, #endif }; diff --git a/mm/page_idle.c b/mm/page_idle.c index 057c61df12db..144fb4ed961d 100644 --- a/mm/page_idle.c +++ b/mm/page_idle.c @@ -211,16 +211,6 @@ static const struct attribute_group page_idle_attr_group = { .name = "page_idle", }; -#ifndef CONFIG_64BIT -static bool need_page_idle(void) -{ - return true; -} -struct page_ext_operations page_idle_ops = { - .need = need_page_idle, -}; -#endif - static int __init page_idle_init(void) { int err;