| Message ID | 20250203225604.44742-5-usamaarif642@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/damon: add support for hugepages | expand |
On Mon, 3 Feb 2025 22:55:31 +0000 Usama Arif <usamaarif642@gmail.com> wrote: > This is to gather statistics to check if memory regions of specific > access tempratures are backed by hugepages of a size in a specific > range nit. A period is missed? > This filter can help to observe and prove the effectivenes of > different schemes for shrinking/collapsing hugepages. > > Signed-off-by: Usama Arif <usamaarif642@gmail.com> > --- > include/linux/damon.h | 2 ++ > mm/damon/paddr.c | 7 +++++++ > mm/damon/sysfs-schemes.c | 1 + > 3 files changed, 10 insertions(+) > > diff --git a/include/linux/damon.h b/include/linux/damon.h > index 6f30ceeff215..5ba6c2114e3f 100644 > --- a/include/linux/damon.h > +++ b/include/linux/damon.h > @@ -336,6 +336,7 @@ struct damos_stat { > * @DAMOS_FILTER_TYPE_ANON: Anonymous pages. > * @DAMOS_FILTER_TYPE_MEMCG: Specific memcg's pages. > * @DAMOS_FILTER_TYPE_YOUNG: Recently accessed pages. > + * @DAMOS_FILTER_TYPE_HUGEPAGE: Page is part of a hugepage. What about "DAMOS_FILTER_TYPE_HUGEPAGE_SIZE: hugepages of a given size range."? > * @DAMOS_FILTER_TYPE_ADDR: Address range. > * @DAMOS_FILTER_TYPE_TARGET: Data Access Monitoring target. > * @NR_DAMOS_FILTER_TYPES: Number of filter types. > @@ -355,6 +356,7 @@ enum damos_filter_type { > DAMOS_FILTER_TYPE_ANON, > DAMOS_FILTER_TYPE_MEMCG, > DAMOS_FILTER_TYPE_YOUNG, > + DAMOS_FILTER_TYPE_HUGEPAGE, > DAMOS_FILTER_TYPE_ADDR, > DAMOS_FILTER_TYPE_TARGET, > NR_DAMOS_FILTER_TYPES, > diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c > index 3f59a3fdc391..34fe1eb664cc 100644 > --- a/mm/damon/paddr.c > +++ b/mm/damon/paddr.c > @@ -227,6 +227,7 @@ static bool damos_pa_filter_match(struct damos_filter *filter, > { > bool matched = false; > struct mem_cgroup *memcg; > + size_t folio_sz; > > switch (filter->type) { > case DAMOS_FILTER_TYPE_ANON: > @@ -246,6 +247,12 @@ static bool damos_pa_filter_match(struct damos_filter *filter, > if (matched) > damon_folio_mkold(folio); > break; > +#if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES) I think we don't really need this macro? > + case DAMOS_FILTER_TYPE_HUGEPAGE: > + folio_sz = folio_size(folio); > + matched = filter->folio_size.min <= folio_sz && folio_sz <= filter->folio_size.max; We should also return 'false' if the folio is not a large folio (folio_sz == PAGE_SIZE), if we agreed to my suggestion on the previous version of this patch series? I'd also prefer calling filter->folio_siz as sz_range or size_range, so that we can reuse it for future filter types of size range. > + break; > +#endif > default: > break; > } > diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c > index bc7ca43ca9c4..76aee3ab277e 100644 > --- a/mm/damon/sysfs-schemes.c > +++ b/mm/damon/sysfs-schemes.c > @@ -330,6 +330,7 @@ static const char * const damon_sysfs_scheme_filter_type_strs[] = { > "anon", > "memcg", > "young", > + "hugepage", hugepage_size? > "addr", > "target", > }; > -- > 2.43.5 Thanks, SJ
On 04/02/2025 23:12, SeongJae Park wrote: > On Mon, 3 Feb 2025 22:55:31 +0000 Usama Arif <usamaarif642@gmail.com> wrote: > >> This is to gather statistics to check if memory regions of specific >> access tempratures are backed by hugepages of a size in a specific >> range > > nit. A period is missed? > >> This filter can help to observe and prove the effectivenes of >> different schemes for shrinking/collapsing hugepages. >> >> Signed-off-by: Usama Arif <usamaarif642@gmail.com> >> --- >> include/linux/damon.h | 2 ++ >> mm/damon/paddr.c | 7 +++++++ >> mm/damon/sysfs-schemes.c | 1 + >> 3 files changed, 10 insertions(+) >> >> diff --git a/include/linux/damon.h b/include/linux/damon.h >> index 6f30ceeff215..5ba6c2114e3f 100644 >> --- a/include/linux/damon.h >> +++ b/include/linux/damon.h >> @@ -336,6 +336,7 @@ struct damos_stat { >> * @DAMOS_FILTER_TYPE_ANON: Anonymous pages. >> * @DAMOS_FILTER_TYPE_MEMCG: Specific memcg's pages. >> * @DAMOS_FILTER_TYPE_YOUNG: Recently accessed pages. >> + * @DAMOS_FILTER_TYPE_HUGEPAGE: Page is part of a hugepage. > > What about "DAMOS_FILTER_TYPE_HUGEPAGE_SIZE: hugepages of a given size range."? > >> * @DAMOS_FILTER_TYPE_ADDR: Address range. >> * @DAMOS_FILTER_TYPE_TARGET: Data Access Monitoring target. >> * @NR_DAMOS_FILTER_TYPES: Number of filter types. >> @@ -355,6 +356,7 @@ enum damos_filter_type { >> DAMOS_FILTER_TYPE_ANON, >> DAMOS_FILTER_TYPE_MEMCG, >> DAMOS_FILTER_TYPE_YOUNG, >> + DAMOS_FILTER_TYPE_HUGEPAGE, >> DAMOS_FILTER_TYPE_ADDR, >> DAMOS_FILTER_TYPE_TARGET, >> NR_DAMOS_FILTER_TYPES, >> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c >> index 3f59a3fdc391..34fe1eb664cc 100644 >> --- a/mm/damon/paddr.c >> +++ b/mm/damon/paddr.c >> @@ -227,6 +227,7 @@ static bool damos_pa_filter_match(struct damos_filter *filter, >> { >> bool matched = false; >> struct mem_cgroup *memcg; >> + size_t folio_sz; >> >> switch (filter->type) { >> case DAMOS_FILTER_TYPE_ANON: >> @@ -246,6 +247,12 @@ static bool damos_pa_filter_match(struct damos_filter *filter, >> if (matched) >> damon_folio_mkold(folio); >> break; >> +#if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES) > > I think we don't really need this macro? > >> + case DAMOS_FILTER_TYPE_HUGEPAGE: >> + folio_sz = folio_size(folio); >> + matched = filter->folio_size.min <= folio_sz && folio_sz <= filter->folio_size.max; > > We should also return 'false' if the folio is not a large folio (folio_sz == > PAGE_SIZE), if we agreed to my suggestion on the previous version of this patch > series? I think we could let users get PAGE_SIZE as well incase they wanted it? more on it below.. > > I'd also prefer calling filter->folio_siz as sz_range or size_range, so that we > can reuse it for future filter types of size range. > >> + break; >> +#endif >> default: >> break; >> } >> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c >> index bc7ca43ca9c4..76aee3ab277e 100644 >> --- a/mm/damon/sysfs-schemes.c >> +++ b/mm/damon/sysfs-schemes.c >> @@ -330,6 +330,7 @@ static const char * const damon_sysfs_scheme_filter_type_strs[] = { >> "anon", >> "memcg", >> "young", >> + "hugepage", > > hugepage_size? > So from https://lore.kernel.org/all/20250121200510.43645-1-sj@kernel.org/, my understanding was you didn't have a preference between "hugepage" and "hugepage_size"? I do prefer hugepage over hugepage_size. This is because this is a type of filter, I would say hugepage_size is not a type, but hugepage is. For e.g. we have anon as type and not anon_number. And the "size" part is clear from the min and max which is documented in patch 5 and 6. TBH my preference would be "folio". What I think we should do is let users input any value for min and max, as long as max > min, even if its just 4K page. If the user just wants to know how much of the region is 4K page, they will get it with this. But as David said, folio is a kernel concept so might not be best to expose it as sysfs to userspace? As folio might not be an option, my preference is going with hugepage, but as its not an implementation detail and just naming, I am ok to change it to hugepage_size if you have a strong preference for it. >> "addr", >> "target", >> }; >> -- >> 2.43.5 > > > Thanks, > SJ
On Wed, 5 Feb 2025 13:52:37 +0000 Usama Arif <usamaarif642@gmail.com> wrote: > > > On 04/02/2025 23:12, SeongJae Park wrote: > > On Mon, 3 Feb 2025 22:55:31 +0000 Usama Arif <usamaarif642@gmail.com> wrote: > > > >> This is to gather statistics to check if memory regions of specific > >> access tempratures are backed by hugepages of a size in a specific > >> range > > > > nit. A period is missed? > > > >> This filter can help to observe and prove the effectivenes of > >> different schemes for shrinking/collapsing hugepages. > >> > >> Signed-off-by: Usama Arif <usamaarif642@gmail.com> > >> --- [...] > >> --- a/mm/damon/sysfs-schemes.c > >> +++ b/mm/damon/sysfs-schemes.c > >> @@ -330,6 +330,7 @@ static const char * const damon_sysfs_scheme_filter_type_strs[] = { > >> "anon", > >> "memcg", > >> "young", > >> + "hugepage", > > > > hugepage_size? > > > > So from https://lore.kernel.org/all/20250121200510.43645-1-sj@kernel.org/, my understanding > was you didn't have a preference between "hugepage" and "hugepage_size"? You're right. And sorry, I changed my mind. > > I do prefer hugepage over hugepage_size. This is because this is a type of filter, I would > say hugepage_size is not a type, but hugepage is. For e.g. we have anon as type and not > anon_number. And the "size" part is clear from the min and max which is documented in patch > 5 and 6. I agree your points. The meaning of "min" and "max" is well documented. But, not everyone reads the documentation, and human's memory is volatile. Also we might want to further extend DAMOS fitler for other hugepage properties, say, zero-filled hugepage-internal base pages ratio. So now I prefer "hugepage_size". > > TBH my preference would be "folio". What I think we should do is let users input any value > for min and max, as long as max > min, even if its just 4K page. If the user just wants > to know how much of the region is 4K page, they will get it with this. We can still do this, since DAMOS filters work for not only matching memory but also non-matching memory. It's not very intuitive, but at least makes a sense in my opinion. > But as David said, > folio is a kernel concept so might not be best to expose it as sysfs to userspace? > > As folio might not be an option, my preference is going with hugepage, but as its not > an implementation detail and just naming, I am ok to change it to hugepage_size if > you have a strong preference for it. Yes, I'd suggest calling it "hugepage_size". I don't find a problem it will cause, other than the length. Please let me know if I'm missing something, though! Again, sorry for changing my mind after the last discussion. Thanks, SJ [...]
On 04/02/2025 23:12, SeongJae Park wrote: > On Mon, 3 Feb 2025 22:55:31 +0000 Usama Arif <usamaarif642@gmail.com> wrote: > > >> + case DAMOS_FILTER_TYPE_HUGEPAGE: >> + folio_sz = folio_size(folio); >> + matched = filter->folio_size.min <= folio_sz && folio_sz <= filter->folio_size.max; > > We should also return 'false' if the folio is not a large folio (folio_sz == > PAGE_SIZE), if we agreed to my suggestion on the previous version of this patch > series? > Thanks SJ for the reviews. I will address all of the changes in the next revision. The only thing over here is, I would like the filter to work for folios of all sizes, even PAGE_SIZE. It would give us a more convenient way of knowing if hot regions are mainly being backed by 4K pages, and improve observability. Let me know if thats ok?
On Fri, 7 Feb 2025 18:22:04 +0000 Usama Arif <usamaarif642@gmail.com> wrote: > > > On 04/02/2025 23:12, SeongJae Park wrote: > > On Mon, 3 Feb 2025 22:55:31 +0000 Usama Arif <usamaarif642@gmail.com> wrote: > > > > > >> + case DAMOS_FILTER_TYPE_HUGEPAGE: > >> + folio_sz = folio_size(folio); > >> + matched = filter->folio_size.min <= folio_sz && folio_sz <= filter->folio_size.max; > > > > We should also return 'false' if the folio is not a large folio (folio_sz == > > PAGE_SIZE), if we agreed to my suggestion on the previous version of this patch > > series? > > > > Thanks SJ for the reviews. The pleasure is mine! > > I will address all of the changes in the next revision. Looking forward to :) > The only thing over here is, I would > like the filter to work for folios of all sizes, even PAGE_SIZE. It would give us a more convenient > way of knowing if hot regions are mainly being backed by 4K pages, and improve observability. > Let me know if thats ok? I have no strong opinion here. Unless others have concerns, I'm ok with your approach. Thanks, SJ
diff --git a/include/linux/damon.h b/include/linux/damon.h index 6f30ceeff215..5ba6c2114e3f 100644 --- a/include/linux/damon.h +++ b/include/linux/damon.h @@ -336,6 +336,7 @@ struct damos_stat { * @DAMOS_FILTER_TYPE_ANON: Anonymous pages. * @DAMOS_FILTER_TYPE_MEMCG: Specific memcg's pages. * @DAMOS_FILTER_TYPE_YOUNG: Recently accessed pages. + * @DAMOS_FILTER_TYPE_HUGEPAGE: Page is part of a hugepage. * @DAMOS_FILTER_TYPE_ADDR: Address range. * @DAMOS_FILTER_TYPE_TARGET: Data Access Monitoring target. * @NR_DAMOS_FILTER_TYPES: Number of filter types. @@ -355,6 +356,7 @@ enum damos_filter_type { DAMOS_FILTER_TYPE_ANON, DAMOS_FILTER_TYPE_MEMCG, DAMOS_FILTER_TYPE_YOUNG, + DAMOS_FILTER_TYPE_HUGEPAGE, DAMOS_FILTER_TYPE_ADDR, DAMOS_FILTER_TYPE_TARGET, NR_DAMOS_FILTER_TYPES, diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c index 3f59a3fdc391..34fe1eb664cc 100644 --- a/mm/damon/paddr.c +++ b/mm/damon/paddr.c @@ -227,6 +227,7 @@ static bool damos_pa_filter_match(struct damos_filter *filter, { bool matched = false; struct mem_cgroup *memcg; + size_t folio_sz; switch (filter->type) { case DAMOS_FILTER_TYPE_ANON: @@ -246,6 +247,12 @@ static bool damos_pa_filter_match(struct damos_filter *filter, if (matched) damon_folio_mkold(folio); break; +#if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES) + case DAMOS_FILTER_TYPE_HUGEPAGE: + folio_sz = folio_size(folio); + matched = filter->folio_size.min <= folio_sz && folio_sz <= filter->folio_size.max; + break; +#endif default: break; } diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c index bc7ca43ca9c4..76aee3ab277e 100644 --- a/mm/damon/sysfs-schemes.c +++ b/mm/damon/sysfs-schemes.c @@ -330,6 +330,7 @@ static const char * const damon_sysfs_scheme_filter_type_strs[] = { "anon", "memcg", "young", + "hugepage", "addr", "target", };
This is to gather statistics to check if memory regions of specific access tempratures are backed by hugepages of a size in a specific range This filter can help to observe and prove the effectivenes of different schemes for shrinking/collapsing hugepages. Signed-off-by: Usama Arif <usamaarif642@gmail.com> --- include/linux/damon.h | 2 ++ mm/damon/paddr.c | 7 +++++++ mm/damon/sysfs-schemes.c | 1 + 3 files changed, 10 insertions(+)