Re: [PATCH v3 2/2] mm: Tidy up shmem mTHP controls and stats
From: Baolin Wang
Date: Wed Sep 04 2024 - 20:45:56 EST
On 2024/9/4 18:47, Ryan Roberts wrote:
Hi Andrew,
On 09/08/2024 09:31, Baolin Wang wrote:
On 2024/8/8 19:18, Ryan Roberts wrote:
Previously we had a situation where shmem mTHP controls and stats were
not exposed for some supported sizes and were exposed for some
unsupported sizes. So let's clean that up.
Anon mTHP can support all large orders [2, PMD_ORDER]. But shmem can
support all large orders [1, MAX_PAGECACHE_ORDER]. However, per-size
shmem controls and stats were previously being exposed for all the anon
mTHP orders, meaning order-1 was not present, and for arm64 64K base
pages, orders 12 and 13 were exposed but were not supported internally.
Tidy this all up by defining ctrl and stats attribute groups for anon
and file separately. Anon ctrl and stats groups are populated for all
orders in THP_ORDERS_ALL_ANON and file ctrl and stats groups are
populated for all orders in THP_ORDERS_ALL_FILE_DEFAULT.
Additionally, create "any" ctrl and stats attribute groups which are
populated for all orders in (THP_ORDERS_ALL_ANON |
THP_ORDERS_ALL_FILE_DEFAULT). swpout stats use this since they apply to
anon and shmem.
The side-effect of all this is that different hugepage-*kB directories
contain different sets of controls and stats, depending on which memory
types support that size. This approach is preferred over the
alternative, which is to populate dummy controls and stats for memory
types that do not support a given size.
Signed-off-by: Ryan Roberts <ryan.roberts@xxxxxxx>
---
mm/huge_memory.c | 144 +++++++++++++++++++++++++++++++++++++----------
1 file changed, 114 insertions(+), 30 deletions(-)
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 0c3075ee00012..082d86b7c6c2f 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -482,8 +482,8 @@ static void thpsize_release(struct kobject *kobj);
static DEFINE_SPINLOCK(huge_anon_orders_lock);
static LIST_HEAD(thpsize_list);
-static ssize_t thpsize_enabled_show(struct kobject *kobj,
- struct kobj_attribute *attr, char *buf)
+static ssize_t anon_enabled_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
{
int order = to_thpsize(kobj)->order;
const char *output;
@@ -500,9 +500,9 @@ static ssize_t thpsize_enabled_show(struct kobject *kobj,
return sysfs_emit(buf, "%s\n", output);
}
-static ssize_t thpsize_enabled_store(struct kobject *kobj,
- struct kobj_attribute *attr,
- const char *buf, size_t count)
+static ssize_t anon_enabled_store(struct kobject *kobj,
+ struct kobj_attribute *attr,
+ const char *buf, size_t count)
{
int order = to_thpsize(kobj)->order;
ssize_t ret = count;
@@ -544,19 +544,35 @@ static ssize_t thpsize_enabled_store(struct kobject *kobj,
return ret;
}
-static struct kobj_attribute thpsize_enabled_attr =
- __ATTR(enabled, 0644, thpsize_enabled_show, thpsize_enabled_store);
+static struct kobj_attribute anon_enabled_attr =
+ __ATTR(enabled, 0644, anon_enabled_show, anon_enabled_store);
-static struct attribute *thpsize_attrs[] = {
- &thpsize_enabled_attr.attr,
+static struct attribute *anon_ctrl_attrs[] = {
+ &anon_enabled_attr.attr,
+ NULL,
+};
+
+static const struct attribute_group anon_ctrl_attr_grp = {
+ .attrs = anon_ctrl_attrs,
+};
+
+static struct attribute *file_ctrl_attrs[] = {
#ifdef CONFIG_SHMEM
&thpsize_shmem_enabled_attr.attr,
#endif
NULL,
};
-static const struct attribute_group thpsize_attr_group = {
- .attrs = thpsize_attrs,
+static const struct attribute_group file_ctrl_attr_grp = {
+ .attrs = file_ctrl_attrs,
+};
+
+static struct attribute *any_ctrl_attrs[] = {
+ NULL,
+};
+
+static const struct attribute_group any_ctrl_attr_grp = {
+ .attrs = any_ctrl_attrs,
};
I wonder why adding a NULL group?
static const struct kobj_type thpsize_ktype = {
@@ -595,64 +611,132 @@ DEFINE_MTHP_STAT_ATTR(anon_fault_fallback,
MTHP_STAT_ANON_FAULT_FALLBACK);
DEFINE_MTHP_STAT_ATTR(anon_fault_fallback_charge,
MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
DEFINE_MTHP_STAT_ATTR(swpout, MTHP_STAT_SWPOUT);
DEFINE_MTHP_STAT_ATTR(swpout_fallback, MTHP_STAT_SWPOUT_FALLBACK);
+#ifdef CONFIG_SHMEM
DEFINE_MTHP_STAT_ATTR(shmem_alloc, MTHP_STAT_SHMEM_ALLOC);
DEFINE_MTHP_STAT_ATTR(shmem_fallback, MTHP_STAT_SHMEM_FALLBACK);
DEFINE_MTHP_STAT_ATTR(shmem_fallback_charge, MTHP_STAT_SHMEM_FALLBACK_CHARGE);
+#endif
DEFINE_MTHP_STAT_ATTR(split, MTHP_STAT_SPLIT);
DEFINE_MTHP_STAT_ATTR(split_failed, MTHP_STAT_SPLIT_FAILED);
DEFINE_MTHP_STAT_ATTR(split_deferred, MTHP_STAT_SPLIT_DEFERRED);
-static struct attribute *stats_attrs[] = {
+static struct attribute *anon_stats_attrs[] = {
&anon_fault_alloc_attr.attr,
&anon_fault_fallback_attr.attr,
&anon_fault_fallback_charge_attr.attr,
+#ifndef CONFIG_SHMEM
&swpout_attr.attr,
&swpout_fallback_attr.attr,
- &shmem_alloc_attr.attr,
- &shmem_fallback_attr.attr,
- &shmem_fallback_charge_attr.attr,
+#endif
&split_attr.attr,
&split_failed_attr.attr,
&split_deferred_attr.attr,
NULL,
};
-static struct attribute_group stats_attr_group = {
+static struct attribute_group anon_stats_attr_grp = {
+ .name = "stats",
+ .attrs = anon_stats_attrs,
+};
+
+static struct attribute *file_stats_attrs[] = {
+#ifdef CONFIG_SHMEM
+ &shmem_alloc_attr.attr,
+ &shmem_fallback_attr.attr,
+ &shmem_fallback_charge_attr.attr,
+#endif
+ NULL,
+};
+
+static struct attribute_group file_stats_attr_grp = {
+ .name = "stats",
+ .attrs = file_stats_attrs,
+};
+
+static struct attribute *any_stats_attrs[] = {
+#ifdef CONFIG_SHMEM
+ &swpout_attr.attr,
+ &swpout_fallback_attr.attr,
+#endif
Sorry I did not point it out in early version. I think file pages and shmem can
also be split, while 'split_deferred' is only for anonymous page. So I think the
any_stats_attrs should be:
static struct attribute *any_stats_attrs[] = {
#ifdef CONFIG_SHMEM
&swpout_attr.attr,
&swpout_fallback_attr.attr,
#endif
&split_attr.attr,
&split_failed_attr.attr,
NULL,
};
Could you please squash the following into this patch, which is already in
mm-unstable? I'm hoping this sufficient and I don't need to send a whole new
revision since there are changes on top of this in mm-unstable, which makes
things tricky.
----8<-----
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 382938e46f96f..5905957b1f70d 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -631,8 +631,6 @@ static struct attribute *anon_stats_attrs[] = {
&swpout_attr.attr,
&swpout_fallback_attr.attr,
#endif
- &split_attr.attr,
- &split_failed_attr.attr,
&split_deferred_attr.attr,
&nr_anon_attr.attr,
&nr_anon_partially_mapped_attr.attr,
@@ -663,6 +661,8 @@ static struct attribute *any_stats_attrs[] = {
&swpout_attr.attr,
&swpout_fallback_attr.attr,
#endif
+ &split_attr.attr,
+ &split_failed_attr.attr,
NULL,
};
The change looks good to me. Feel free to add
Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>