[PATCH v2 -next 0/3] mm/hugetlb_cgroup: rework on cftypes

[PATCH v2 -next 0/3] mm/hugetlb_cgroup: rework on cftypes

[Xiu Jianfeng]

Hi,

This patchset provides an intuitive view of the control files through
static templates of cftypes, improve the readability of the code.

Changes from v1:
 - Fix smatch warnings of 'tmpl->name'

Xiu Jianfeng (3):
  mm/hugetlb_cgroup: identify the legacy using cgroup_subsys_on_dfl()
  mm/hugetlb_cgroup: prepare cftypes based on template
  mm/hugetlb_cgroup: switch to the new cftypes

 include/linux/hugetlb.h |   5 -
 mm/hugetlb_cgroup.c     | 302 ++++++++++++++++++++++------------------
 2 files changed, 164 insertions(+), 143 deletions(-)

-- 
2.34.1

[PATCH v2 -next 1/3] mm/hugetlb_cgroup: identify the legacy using cgroup_subsys_on_dfl()

[Xiu Jianfeng]

Currently the numa_stat file encodes 1 into private using the micro
MEMFILE_PRIVATE() to identify the legacy. Actually, we can use
cgroup_subsys_on_dfl() instead. This is helpful to handle .private
in the static array of the comming patch.

Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 mm/hugetlb_cgroup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
index e20339a346b9..45f94a869776 100644
--- a/mm/hugetlb_cgroup.c
+++ b/mm/hugetlb_cgroup.c
@@ -460,7 +460,7 @@ static int hugetlb_cgroup_read_numa_stat(struct seq_file *seq, void *dummy)
 	int nid;
 	struct cftype *cft = seq_cft(seq);
 	int idx = MEMFILE_IDX(cft->private);
-	bool legacy = MEMFILE_ATTR(cft->private);
+	bool legacy = !cgroup_subsys_on_dfl(hugetlb_cgrp_subsys);
 	struct hugetlb_cgroup *h_cg = hugetlb_cgroup_from_css(seq_css(seq));
 	struct cgroup_subsys_state *css;
 	unsigned long usage;
@@ -839,7 +839,7 @@ static void __init __hugetlb_cgroup_file_legacy_init(int idx)
 	/* Add the numa stat file */
 	cft = &h->cgroup_files_legacy[8];
 	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.numa_stat", buf);
-	cft->private = MEMFILE_PRIVATE(idx, 1);
+	cft->private = MEMFILE_PRIVATE(idx, 0);
 	cft->seq_show = hugetlb_cgroup_read_numa_stat;
 
 	/* NULL terminate the last cft */
-- 
2.34.1

[PATCH v2 -next 2/3] mm/hugetlb_cgroup: prepare cftypes based on template

[Xiu Jianfeng]

Unlike other cgroup subsystems, the hugetlb cgroup does not provide
a static array of cftype that explicitly displays the properties,
handling functions, etc., of each file. Instead, it dynamically creates
the attribute of cftypes based on the hstate during the startup
procedure. This reduces the readability of the code.

To fix this issue, introduce two templates of cftypes, and rebuild the
attributes according to the hstate to make it ready to be added to
cgroup framework.

Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 mm/hugetlb_cgroup.c | 155 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 155 insertions(+)

diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
index 45f94a869776..56876b61027d 100644
--- a/mm/hugetlb_cgroup.c
+++ b/mm/hugetlb_cgroup.c
@@ -27,7 +27,16 @@
 #define MEMFILE_IDX(val)	(((val) >> 16) & 0xffff)
 #define MEMFILE_ATTR(val)	((val) & 0xffff)
 
+#define MEMFILE_OFFSET(t, m)	(((offsetof(t, m) << 16) | sizeof_field(t, m)))
+#define MEMFILE_OFFSET0(val)	(((val) >> 16) & 0xffff)
+#define MEMFILE_FIELD_SIZE(val)	((val) & 0xffff)
+
+#define DFL_TMPL_SIZE		ARRAY_SIZE(hugetlb_dfl_tmpl)
+#define LEGACY_TMPL_SIZE	ARRAY_SIZE(hugetlb_legacy_tmpl)
+
 static struct hugetlb_cgroup *root_h_cgroup __read_mostly;
+static struct cftype *dfl_files;
+static struct cftype *legacy_files;
 
 static inline struct page_counter *
 __hugetlb_cgroup_counter_from_cgroup(struct hugetlb_cgroup *h_cg, int idx,
@@ -702,12 +711,142 @@ static int hugetlb_events_local_show(struct seq_file *seq, void *v)
 	return __hugetlb_events_show(seq, true);
 }
 
+static struct cftype hugetlb_dfl_tmpl[] = {
+	{
+		.name = "max",
+		.private = RES_LIMIT,
+		.seq_show = hugetlb_cgroup_read_u64_max,
+		.write = hugetlb_cgroup_write_dfl,
+		.flags = CFTYPE_NOT_ON_ROOT,
+	},
+	{
+		.name = "rsvd.max",
+		.private = RES_RSVD_LIMIT,
+		.seq_show = hugetlb_cgroup_read_u64_max,
+		.write = hugetlb_cgroup_write_dfl,
+		.flags = CFTYPE_NOT_ON_ROOT,
+	},
+	{
+		.name = "current",
+		.private = RES_USAGE,
+		.seq_show = hugetlb_cgroup_read_u64_max,
+		.flags = CFTYPE_NOT_ON_ROOT,
+	},
+	{
+		.name = "rsvd.current",
+		.private = RES_RSVD_USAGE,
+		.seq_show = hugetlb_cgroup_read_u64_max,
+		.flags = CFTYPE_NOT_ON_ROOT,
+	},
+	{
+		.name = "events",
+		.seq_show = hugetlb_events_show,
+		.file_offset = MEMFILE_OFFSET(struct hugetlb_cgroup, events_file[0]),
+		.flags = CFTYPE_NOT_ON_ROOT,
+	},
+	{
+		.name = "events.local",
+		.seq_show = hugetlb_events_local_show,
+		.file_offset = MEMFILE_OFFSET(struct hugetlb_cgroup, events_local_file[0]),
+		.flags = CFTYPE_NOT_ON_ROOT,
+	},
+	{
+		.name = "numa_stat",
+		.seq_show = hugetlb_cgroup_read_numa_stat,
+		.flags = CFTYPE_NOT_ON_ROOT,
+	},
+	/* don't need terminator here */
+};
+
+static struct cftype hugetlb_legacy_tmpl[] = {
+	{
+		.name = "limit_in_bytes",
+		.private = RES_LIMIT,
+		.read_u64 = hugetlb_cgroup_read_u64,
+		.write = hugetlb_cgroup_write_legacy,
+	},
+	{
+		.name = "rsvd.limit_in_bytes",
+		.private = RES_RSVD_LIMIT,
+		.read_u64 = hugetlb_cgroup_read_u64,
+		.write = hugetlb_cgroup_write_legacy,
+	},
+	{
+		.name = "usage_in_bytes",
+		.private = RES_USAGE,
+		.read_u64 = hugetlb_cgroup_read_u64,
+	},
+	{
+		.name = "rsvd.usage_in_bytes",
+		.private = RES_RSVD_USAGE,
+		.read_u64 = hugetlb_cgroup_read_u64,
+	},
+	{
+		.name = "max_usage_in_bytes",
+		.private = RES_MAX_USAGE,
+		.write = hugetlb_cgroup_reset,
+		.read_u64 = hugetlb_cgroup_read_u64,
+	},
+	{
+		.name = "rsvd.max_usage_in_bytes",
+		.private = RES_RSVD_MAX_USAGE,
+		.write = hugetlb_cgroup_reset,
+		.read_u64 = hugetlb_cgroup_read_u64,
+	},
+	{
+		.name = "failcnt",
+		.private = RES_FAILCNT,
+		.write = hugetlb_cgroup_reset,
+		.read_u64 = hugetlb_cgroup_read_u64,
+	},
+	{
+		.name = "rsvd.failcnt",
+		.private = RES_RSVD_FAILCNT,
+		.write = hugetlb_cgroup_reset,
+		.read_u64 = hugetlb_cgroup_read_u64,
+	},
+	{
+		.name = "numa_stat",
+		.seq_show = hugetlb_cgroup_read_numa_stat,
+	},
+	/* don't need terminator here */
+};
+
+static void __init
+hugetlb_cgroup_cfttypes_init(struct hstate *h, struct cftype *cft,
+			     struct cftype *tmpl, int tmpl_size)
+{
+	char buf[32];
+	int i, idx = hstate_index(h);
+
+	/* format the size */
+	mem_fmt(buf, sizeof(buf), huge_page_size(h));
+
+	for (i = 0; i < tmpl_size; cft++, tmpl++, i++) {
+		*cft = *tmpl;
+		/* rebuild the name */
+		snprintf(cft->name, MAX_CFTYPE_NAME, "%s.%s", buf, tmpl->name);
+		/* rebuild the private */
+		if (tmpl->private)
+			cft->private = MEMFILE_PRIVATE(idx, tmpl->private);
+		/* rebuild the file_offset */
+		if (tmpl->file_offset) {
+			unsigned int offset = tmpl->file_offset;
+			cft->file_offset = MEMFILE_OFFSET0(offset) +
+					   MEMFILE_FIELD_SIZE(offset) * idx;
+		}
+	}
+}
+
 static void __init __hugetlb_cgroup_file_dfl_init(int idx)
 {
 	char buf[32];
 	struct cftype *cft;
 	struct hstate *h = &hstates[idx];
 
+	hugetlb_cgroup_cfttypes_init(h, dfl_files + idx * DFL_TMPL_SIZE,
+				     hugetlb_dfl_tmpl, DFL_TMPL_SIZE);
+
 	/* format the size */
 	mem_fmt(buf, sizeof(buf), huge_page_size(h));
 
@@ -779,6 +918,9 @@ static void __init __hugetlb_cgroup_file_legacy_init(int idx)
 	struct cftype *cft;
 	struct hstate *h = &hstates[idx];
 
+	hugetlb_cgroup_cfttypes_init(h, legacy_files + idx * LEGACY_TMPL_SIZE,
+				     hugetlb_legacy_tmpl, LEGACY_TMPL_SIZE);
+
 	/* format the size */
 	mem_fmt(buf, sizeof(buf), huge_page_size(h));
 
@@ -856,10 +998,23 @@ static void __init __hugetlb_cgroup_file_init(int idx)
 	__hugetlb_cgroup_file_legacy_init(idx);
 }
 
+static void __init __hugetlb_cgroup_file_pre_init(void)
+{
+	int cft_count;
+
+	cft_count = hugetlb_max_hstate * DFL_TMPL_SIZE + 1; /* add terminator */
+	dfl_files = kcalloc(cft_count, sizeof(struct cftype), GFP_KERNEL);
+	BUG_ON(!dfl_files);
+	cft_count = hugetlb_max_hstate * LEGACY_TMPL_SIZE + 1; /* add terminator */
+	legacy_files = kcalloc(cft_count, sizeof(struct cftype), GFP_KERNEL);
+	BUG_ON(!legacy_files);
+}
+
 void __init hugetlb_cgroup_file_init(void)
 {
 	struct hstate *h;
 
+	__hugetlb_cgroup_file_pre_init();
 	for_each_hstate(h)
 		__hugetlb_cgroup_file_init(hstate_index(h));
 }
-- 
2.34.1

[PATCH v2 -next 3/3] mm/hugetlb_cgroup: switch to the new cftypes

[Xiu Jianfeng]

The previous patch has already reconstructed the cftype attributes
based on the templates and saved them in dfl_cftypes and legacy_cftypes.
then remove the old procedure and switch to the new cftypes.

Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 include/linux/hugetlb.h |   5 --
 mm/hugetlb_cgroup.c     | 163 +++++-----------------------------------
 2 files changed, 17 insertions(+), 151 deletions(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 279aca379b95..a951c0d06061 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -686,11 +686,6 @@ struct hstate {
 	unsigned int nr_huge_pages_node[MAX_NUMNODES];
 	unsigned int free_huge_pages_node[MAX_NUMNODES];
 	unsigned int surplus_huge_pages_node[MAX_NUMNODES];
-#ifdef CONFIG_CGROUP_HUGETLB
-	/* cgroup control files */
-	struct cftype cgroup_files_dfl[8];
-	struct cftype cgroup_files_legacy[10];
-#endif
 	char name[HSTATE_NAME_LEN];
 };
 
diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
index 56876b61027d..11806736ab62 100644
--- a/mm/hugetlb_cgroup.c
+++ b/mm/hugetlb_cgroup.c
@@ -838,164 +838,26 @@ hugetlb_cgroup_cfttypes_init(struct hstate *h, struct cftype *cft,
 	}
 }
 
-static void __init __hugetlb_cgroup_file_dfl_init(int idx)
+static void __init __hugetlb_cgroup_file_dfl_init(struct hstate *h)
 {
-	char buf[32];
-	struct cftype *cft;
-	struct hstate *h = &hstates[idx];
+	int idx = hstate_index(h);
 
 	hugetlb_cgroup_cfttypes_init(h, dfl_files + idx * DFL_TMPL_SIZE,
 				     hugetlb_dfl_tmpl, DFL_TMPL_SIZE);
-
-	/* format the size */
-	mem_fmt(buf, sizeof(buf), huge_page_size(h));
-
-	/* Add the limit file */
-	cft = &h->cgroup_files_dfl[0];
-	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.max", buf);
-	cft->private = MEMFILE_PRIVATE(idx, RES_LIMIT);
-	cft->seq_show = hugetlb_cgroup_read_u64_max;
-	cft->write = hugetlb_cgroup_write_dfl;
-	cft->flags = CFTYPE_NOT_ON_ROOT;
-
-	/* Add the reservation limit file */
-	cft = &h->cgroup_files_dfl[1];
-	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.rsvd.max", buf);
-	cft->private = MEMFILE_PRIVATE(idx, RES_RSVD_LIMIT);
-	cft->seq_show = hugetlb_cgroup_read_u64_max;
-	cft->write = hugetlb_cgroup_write_dfl;
-	cft->flags = CFTYPE_NOT_ON_ROOT;
-
-	/* Add the current usage file */
-	cft = &h->cgroup_files_dfl[2];
-	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.current", buf);
-	cft->private = MEMFILE_PRIVATE(idx, RES_USAGE);
-	cft->seq_show = hugetlb_cgroup_read_u64_max;
-	cft->flags = CFTYPE_NOT_ON_ROOT;
-
-	/* Add the current reservation usage file */
-	cft = &h->cgroup_files_dfl[3];
-	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.rsvd.current", buf);
-	cft->private = MEMFILE_PRIVATE(idx, RES_RSVD_USAGE);
-	cft->seq_show = hugetlb_cgroup_read_u64_max;
-	cft->flags = CFTYPE_NOT_ON_ROOT;
-
-	/* Add the events file */
-	cft = &h->cgroup_files_dfl[4];
-	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.events", buf);
-	cft->private = MEMFILE_PRIVATE(idx, 0);
-	cft->seq_show = hugetlb_events_show;
-	cft->file_offset = offsetof(struct hugetlb_cgroup, events_file[idx]);
-	cft->flags = CFTYPE_NOT_ON_ROOT;
-
-	/* Add the events.local file */
-	cft = &h->cgroup_files_dfl[5];
-	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.events.local", buf);
-	cft->private = MEMFILE_PRIVATE(idx, 0);
-	cft->seq_show = hugetlb_events_local_show;
-	cft->file_offset = offsetof(struct hugetlb_cgroup,
-				    events_local_file[idx]);
-	cft->flags = CFTYPE_NOT_ON_ROOT;
-
-	/* Add the numa stat file */
-	cft = &h->cgroup_files_dfl[6];
-	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.numa_stat", buf);
-	cft->private = MEMFILE_PRIVATE(idx, 0);
-	cft->seq_show = hugetlb_cgroup_read_numa_stat;
-	cft->flags = CFTYPE_NOT_ON_ROOT;
-
-	/* NULL terminate the last cft */
-	cft = &h->cgroup_files_dfl[7];
-	memset(cft, 0, sizeof(*cft));
-
-	WARN_ON(cgroup_add_dfl_cftypes(&hugetlb_cgrp_subsys,
-				       h->cgroup_files_dfl));
 }
 
-static void __init __hugetlb_cgroup_file_legacy_init(int idx)
+static void __init __hugetlb_cgroup_file_legacy_init(struct hstate *h)
 {
-	char buf[32];
-	struct cftype *cft;
-	struct hstate *h = &hstates[idx];
+	int idx = hstate_index(h);
 
 	hugetlb_cgroup_cfttypes_init(h, legacy_files + idx * LEGACY_TMPL_SIZE,
 				     hugetlb_legacy_tmpl, LEGACY_TMPL_SIZE);
-
-	/* format the size */
-	mem_fmt(buf, sizeof(buf), huge_page_size(h));
-
-	/* Add the limit file */
-	cft = &h->cgroup_files_legacy[0];
-	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.limit_in_bytes", buf);
-	cft->private = MEMFILE_PRIVATE(idx, RES_LIMIT);
-	cft->read_u64 = hugetlb_cgroup_read_u64;
-	cft->write = hugetlb_cgroup_write_legacy;
-
-	/* Add the reservation limit file */
-	cft = &h->cgroup_files_legacy[1];
-	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.rsvd.limit_in_bytes", buf);
-	cft->private = MEMFILE_PRIVATE(idx, RES_RSVD_LIMIT);
-	cft->read_u64 = hugetlb_cgroup_read_u64;
-	cft->write = hugetlb_cgroup_write_legacy;
-
-	/* Add the usage file */
-	cft = &h->cgroup_files_legacy[2];
-	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.usage_in_bytes", buf);
-	cft->private = MEMFILE_PRIVATE(idx, RES_USAGE);
-	cft->read_u64 = hugetlb_cgroup_read_u64;
-
-	/* Add the reservation usage file */
-	cft = &h->cgroup_files_legacy[3];
-	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.rsvd.usage_in_bytes", buf);
-	cft->private = MEMFILE_PRIVATE(idx, RES_RSVD_USAGE);
-	cft->read_u64 = hugetlb_cgroup_read_u64;
-
-	/* Add the MAX usage file */
-	cft = &h->cgroup_files_legacy[4];
-	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.max_usage_in_bytes", buf);
-	cft->private = MEMFILE_PRIVATE(idx, RES_MAX_USAGE);
-	cft->write = hugetlb_cgroup_reset;
-	cft->read_u64 = hugetlb_cgroup_read_u64;
-
-	/* Add the MAX reservation usage file */
-	cft = &h->cgroup_files_legacy[5];
-	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.rsvd.max_usage_in_bytes", buf);
-	cft->private = MEMFILE_PRIVATE(idx, RES_RSVD_MAX_USAGE);
-	cft->write = hugetlb_cgroup_reset;
-	cft->read_u64 = hugetlb_cgroup_read_u64;
-
-	/* Add the failcntfile */
-	cft = &h->cgroup_files_legacy[6];
-	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.failcnt", buf);
-	cft->private = MEMFILE_PRIVATE(idx, RES_FAILCNT);
-	cft->write = hugetlb_cgroup_reset;
-	cft->read_u64 = hugetlb_cgroup_read_u64;
-
-	/* Add the reservation failcntfile */
-	cft = &h->cgroup_files_legacy[7];
-	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.rsvd.failcnt", buf);
-	cft->private = MEMFILE_PRIVATE(idx, RES_RSVD_FAILCNT);
-	cft->write = hugetlb_cgroup_reset;
-	cft->read_u64 = hugetlb_cgroup_read_u64;
-
-	/* Add the numa stat file */
-	cft = &h->cgroup_files_legacy[8];
-	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.numa_stat", buf);
-	cft->private = MEMFILE_PRIVATE(idx, 0);
-	cft->seq_show = hugetlb_cgroup_read_numa_stat;
-
-	/* NULL terminate the last cft */
-	cft = &h->cgroup_files_legacy[9];
-	memset(cft, 0, sizeof(*cft));
-
-	WARN_ON(cgroup_add_legacy_cftypes(&hugetlb_cgrp_subsys,
-					  h->cgroup_files_legacy));
 }
 
-static void __init __hugetlb_cgroup_file_init(int idx)
+static void __init __hugetlb_cgroup_file_init(struct hstate *h)
 {
-	__hugetlb_cgroup_file_dfl_init(idx);
-	__hugetlb_cgroup_file_legacy_init(idx);
+	__hugetlb_cgroup_file_dfl_init(h);
+	__hugetlb_cgroup_file_legacy_init(h);
 }
 
 static void __init __hugetlb_cgroup_file_pre_init(void)
@@ -1010,13 +872,22 @@ static void __init __hugetlb_cgroup_file_pre_init(void)
 	BUG_ON(!legacy_files);
 }
 
+static void __init __hugetlb_cgroup_file_post_init(void)
+{
+	WARN_ON(cgroup_add_dfl_cftypes(&hugetlb_cgrp_subsys,
+				       dfl_files));
+	WARN_ON(cgroup_add_legacy_cftypes(&hugetlb_cgrp_subsys,
+					  legacy_files));
+}
+
 void __init hugetlb_cgroup_file_init(void)
 {
 	struct hstate *h;
 
 	__hugetlb_cgroup_file_pre_init();
 	for_each_hstate(h)
-		__hugetlb_cgroup_file_init(hstate_index(h));
+		__hugetlb_cgroup_file_init(h);
+	__hugetlb_cgroup_file_post_init();
 }
 
 /*
-- 
2.34.1

Re: [PATCH v2 -next 0/3] mm/hugetlb_cgroup: rework on cftypes

[Oscar Salvador]

On Wed, Jun 05, 2024 at 07:01:30AM +0000, Xiu Jianfeng wrote:
> Hi,
> 
> This patchset provides an intuitive view of the control files through
> static templates of cftypes, improve the readability of the code.

Just dropping by to say that I plan to have a look into this today/tomorrow.


-- 
Oscar Salvador
SUSE Labs

Re: [PATCH v2 -next 1/3] mm/hugetlb_cgroup: identify the legacy using cgroup_subsys_on_dfl()

[Oscar Salvador]

On Wed, Jun 05, 2024 at 07:01:31AM +0000, Xiu Jianfeng wrote:
> Currently the numa_stat file encodes 1 into private using the micro
> MEMFILE_PRIVATE() to identify the legacy. Actually, we can use
> cgroup_subsys_on_dfl() instead. This is helpful to handle .private
> in the static array of the comming patch.
> 
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>

Reviewed-by: Oscar Salvador <osalvador@suse.de>

Nit below:

> ---
>  mm/hugetlb_cgroup.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
> index e20339a346b9..45f94a869776 100644
> --- a/mm/hugetlb_cgroup.c
> +++ b/mm/hugetlb_cgroup.c
> @@ -460,7 +460,7 @@ static int hugetlb_cgroup_read_numa_stat(struct seq_file *seq, void *dummy)
>  	int nid;
>  	struct cftype *cft = seq_cft(seq);
>  	int idx = MEMFILE_IDX(cft->private);
> -	bool legacy = MEMFILE_ATTR(cft->private);
> +	bool legacy = !cgroup_subsys_on_dfl(hugetlb_cgrp_subsys);
>  	struct hugetlb_cgroup *h_cg = hugetlb_cgroup_from_css(seq_css(seq));
>  	struct cgroup_subsys_state *css;
>  	unsigned long usage;
> @@ -839,7 +839,7 @@ static void __init __hugetlb_cgroup_file_legacy_init(int idx)
>  	/* Add the numa stat file */
>  	cft = &h->cgroup_files_legacy[8];
>  	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.numa_stat", buf);
> -	cft->private = MEMFILE_PRIVATE(idx, 1);
> +	cft->private = MEMFILE_PRIVATE(idx, 0);

Do we still have to set it to '0'?
Is it not by default?

-- 
Oscar Salvador
SUSE Labs

Re: [PATCH v2 -next 1/3] mm/hugetlb_cgroup: identify the legacy using cgroup_subsys_on_dfl()

[xiujianfeng]

On 2024/6/10 13:03, Oscar Salvador wrote:
> On Wed, Jun 05, 2024 at 07:01:31AM +0000, Xiu Jianfeng wrote:
>> Currently the numa_stat file encodes 1 into private using the micro
>> MEMFILE_PRIVATE() to identify the legacy. Actually, we can use
>> cgroup_subsys_on_dfl() instead. This is helpful to handle .private
>> in the static array of the comming patch.
>>
>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> 
> Reviewed-by: Oscar Salvador <osalvador@suse.de>
> 
> Nit below:
> 
>> ---
>>  mm/hugetlb_cgroup.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
>> index e20339a346b9..45f94a869776 100644
>> --- a/mm/hugetlb_cgroup.c
>> +++ b/mm/hugetlb_cgroup.c
>> @@ -460,7 +460,7 @@ static int hugetlb_cgroup_read_numa_stat(struct seq_file *seq, void *dummy)
>>  	int nid;
>>  	struct cftype *cft = seq_cft(seq);
>>  	int idx = MEMFILE_IDX(cft->private);
>> -	bool legacy = MEMFILE_ATTR(cft->private);
>> +	bool legacy = !cgroup_subsys_on_dfl(hugetlb_cgrp_subsys);
>>  	struct hugetlb_cgroup *h_cg = hugetlb_cgroup_from_css(seq_css(seq));
>>  	struct cgroup_subsys_state *css;
>>  	unsigned long usage;
>> @@ -839,7 +839,7 @@ static void __init __hugetlb_cgroup_file_legacy_init(int idx)
>>  	/* Add the numa stat file */
>>  	cft = &h->cgroup_files_legacy[8];
>>  	snprintf(cft->name, MAX_CFTYPE_NAME, "%s.numa_stat", buf);
>> -	cft->private = MEMFILE_PRIVATE(idx, 1);
>> +	cft->private = MEMFILE_PRIVATE(idx, 0);
> 
> Do we still have to set it to '0'?
> Is it not by default?
> 

Hi Oscar,

Thanks for your review.

Not all files care about the value of the second argument of
MEMFILE_PRIVATE(), but all files care about the value of the first
argument 'idx', so the .private must be explicitly set, and passing 0 as
the second argument can be treated as 'not caring what its value is'.

Setting it to 0 or 1 does not affect the functionality, but it makes the
semantics more clear, IMO.

However, you've reminded me that the reassignment of .private in the
second patch is problematic, and I will make a comment on that patch.

Re: [PATCH v2 -next 2/3] mm/hugetlb_cgroup: prepare cftypes based on template

[xiujianfeng]

On 2024/6/5 15:01, Xiu Jianfeng wrote:
> Unlike other cgroup subsystems, the hugetlb cgroup does not provide
> a static array of cftype that explicitly displays the properties,
> handling functions, etc., of each file. Instead, it dynamically creates
> the attribute of cftypes based on the hstate during the startup
> procedure. This reduces the readability of the code.
> 
> To fix this issue, introduce two templates of cftypes, and rebuild the
> attributes according to the hstate to make it ready to be added to
> cgroup framework.
> 
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> ---
>  mm/hugetlb_cgroup.c | 155 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 155 insertions(+)
> 
> diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
> index 45f94a869776..56876b61027d 100644
> --- a/mm/hugetlb_cgroup.c
> +++ b/mm/hugetlb_cgroup.c
> @@ -27,7 +27,16 @@
>  #define MEMFILE_IDX(val)	(((val) >> 16) & 0xffff)
>  #define MEMFILE_ATTR(val)	((val) & 0xffff)
>  
> +#define MEMFILE_OFFSET(t, m)	(((offsetof(t, m) << 16) | sizeof_field(t, m)))
> +#define MEMFILE_OFFSET0(val)	(((val) >> 16) & 0xffff)
> +#define MEMFILE_FIELD_SIZE(val)	((val) & 0xffff)
> +
> +#define DFL_TMPL_SIZE		ARRAY_SIZE(hugetlb_dfl_tmpl)
> +#define LEGACY_TMPL_SIZE	ARRAY_SIZE(hugetlb_legacy_tmpl)
> +
>  static struct hugetlb_cgroup *root_h_cgroup __read_mostly;
> +static struct cftype *dfl_files;
> +static struct cftype *legacy_files;
>  
>  static inline struct page_counter *
>  __hugetlb_cgroup_counter_from_cgroup(struct hugetlb_cgroup *h_cg, int idx,
> @@ -702,12 +711,142 @@ static int hugetlb_events_local_show(struct seq_file *seq, void *v)
>  	return __hugetlb_events_show(seq, true);
>  }
>  
> +static struct cftype hugetlb_dfl_tmpl[] = {
> +	{
> +		.name = "max",
> +		.private = RES_LIMIT,
> +		.seq_show = hugetlb_cgroup_read_u64_max,
> +		.write = hugetlb_cgroup_write_dfl,
> +		.flags = CFTYPE_NOT_ON_ROOT,
> +	},
> +	{
> +		.name = "rsvd.max",
> +		.private = RES_RSVD_LIMIT,
> +		.seq_show = hugetlb_cgroup_read_u64_max,
> +		.write = hugetlb_cgroup_write_dfl,
> +		.flags = CFTYPE_NOT_ON_ROOT,
> +	},
> +	{
> +		.name = "current",
> +		.private = RES_USAGE,
> +		.seq_show = hugetlb_cgroup_read_u64_max,
> +		.flags = CFTYPE_NOT_ON_ROOT,
> +	},
> +	{
> +		.name = "rsvd.current",
> +		.private = RES_RSVD_USAGE,
> +		.seq_show = hugetlb_cgroup_read_u64_max,
> +		.flags = CFTYPE_NOT_ON_ROOT,
> +	},
> +	{
> +		.name = "events",
> +		.seq_show = hugetlb_events_show,
> +		.file_offset = MEMFILE_OFFSET(struct hugetlb_cgroup, events_file[0]),
> +		.flags = CFTYPE_NOT_ON_ROOT,
> +	},
> +	{
> +		.name = "events.local",
> +		.seq_show = hugetlb_events_local_show,
> +		.file_offset = MEMFILE_OFFSET(struct hugetlb_cgroup, events_local_file[0]),
> +		.flags = CFTYPE_NOT_ON_ROOT,
> +	},
> +	{
> +		.name = "numa_stat",
> +		.seq_show = hugetlb_cgroup_read_numa_stat,
> +		.flags = CFTYPE_NOT_ON_ROOT,
> +	},
> +	/* don't need terminator here */
> +};
> +
> +static struct cftype hugetlb_legacy_tmpl[] = {
> +	{
> +		.name = "limit_in_bytes",
> +		.private = RES_LIMIT,
> +		.read_u64 = hugetlb_cgroup_read_u64,
> +		.write = hugetlb_cgroup_write_legacy,
> +	},
> +	{
> +		.name = "rsvd.limit_in_bytes",
> +		.private = RES_RSVD_LIMIT,
> +		.read_u64 = hugetlb_cgroup_read_u64,
> +		.write = hugetlb_cgroup_write_legacy,
> +	},
> +	{
> +		.name = "usage_in_bytes",
> +		.private = RES_USAGE,
> +		.read_u64 = hugetlb_cgroup_read_u64,
> +	},
> +	{
> +		.name = "rsvd.usage_in_bytes",
> +		.private = RES_RSVD_USAGE,
> +		.read_u64 = hugetlb_cgroup_read_u64,
> +	},
> +	{
> +		.name = "max_usage_in_bytes",
> +		.private = RES_MAX_USAGE,
> +		.write = hugetlb_cgroup_reset,
> +		.read_u64 = hugetlb_cgroup_read_u64,
> +	},
> +	{
> +		.name = "rsvd.max_usage_in_bytes",
> +		.private = RES_RSVD_MAX_USAGE,
> +		.write = hugetlb_cgroup_reset,
> +		.read_u64 = hugetlb_cgroup_read_u64,
> +	},
> +	{
> +		.name = "failcnt",
> +		.private = RES_FAILCNT,
> +		.write = hugetlb_cgroup_reset,
> +		.read_u64 = hugetlb_cgroup_read_u64,
> +	},
> +	{
> +		.name = "rsvd.failcnt",
> +		.private = RES_RSVD_FAILCNT,
> +		.write = hugetlb_cgroup_reset,
> +		.read_u64 = hugetlb_cgroup_read_u64,
> +	},
> +	{
> +		.name = "numa_stat",
> +		.seq_show = hugetlb_cgroup_read_numa_stat,
> +	},
> +	/* don't need terminator here */
> +};
> +
> +static void __init
> +hugetlb_cgroup_cfttypes_init(struct hstate *h, struct cftype *cft,
> +			     struct cftype *tmpl, int tmpl_size)
> +{
> +	char buf[32];
> +	int i, idx = hstate_index(h);
> +
> +	/* format the size */
> +	mem_fmt(buf, sizeof(buf), huge_page_size(h));
> +
> +	for (i = 0; i < tmpl_size; cft++, tmpl++, i++) {
> +		*cft = *tmpl;
> +		/* rebuild the name */
> +		snprintf(cft->name, MAX_CFTYPE_NAME, "%s.%s", buf, tmpl->name);
> +		/* rebuild the private */
> +		if (tmpl->private)
> +			cft->private = MEMFILE_PRIVATE(idx, tmpl->private);

I found there is a problem here, the if (tmpl->private) statment should
be dropped, and the cft->private should be reassigned unconditionally
according to the idx of the hstate, because the .private in the
templates only represents the second argument of the MEMFILE_PRIVATE().

But cft->file_offset is a bit different; it does not need to be assigned
unless it is required, its usage is as follows:

int cgroup_add_file(...)
{
...
if (cft->file_offset) {
    struct cgroup_file *cfile = (void *)css + cft->file_offset;
    ...
}
...
}

I will fix it in the next version.

> +		/* rebuild the file_offset */
> +		if (tmpl->file_offset) {
> +			unsigned int offset = tmpl->file_offset;
> +			cft->file_offset = MEMFILE_OFFSET0(offset) +
> +					   MEMFILE_FIELD_SIZE(offset) * idx;
> +		}
> +	}
> +}
> +
>  static void __init __hugetlb_cgroup_file_dfl_init(int idx)
>  {
>  	char buf[32];
>  	struct cftype *cft;
>  	struct hstate *h = &hstates[idx];
>  
> +	hugetlb_cgroup_cfttypes_init(h, dfl_files + idx * DFL_TMPL_SIZE,
> +				     hugetlb_dfl_tmpl, DFL_TMPL_SIZE);
> +
>  	/* format the size */
>  	mem_fmt(buf, sizeof(buf), huge_page_size(h));
>  
> @@ -779,6 +918,9 @@ static void __init __hugetlb_cgroup_file_legacy_init(int idx)
>  	struct cftype *cft;
>  	struct hstate *h = &hstates[idx];
>  
> +	hugetlb_cgroup_cfttypes_init(h, legacy_files + idx * LEGACY_TMPL_SIZE,
> +				     hugetlb_legacy_tmpl, LEGACY_TMPL_SIZE);
> +
>  	/* format the size */
>  	mem_fmt(buf, sizeof(buf), huge_page_size(h));
>  
> @@ -856,10 +998,23 @@ static void __init __hugetlb_cgroup_file_init(int idx)
>  	__hugetlb_cgroup_file_legacy_init(idx);
>  }
>  
> +static void __init __hugetlb_cgroup_file_pre_init(void)
> +{
> +	int cft_count;
> +
> +	cft_count = hugetlb_max_hstate * DFL_TMPL_SIZE + 1; /* add terminator */
> +	dfl_files = kcalloc(cft_count, sizeof(struct cftype), GFP_KERNEL);
> +	BUG_ON(!dfl_files);
> +	cft_count = hugetlb_max_hstate * LEGACY_TMPL_SIZE + 1; /* add terminator */
> +	legacy_files = kcalloc(cft_count, sizeof(struct cftype), GFP_KERNEL);
> +	BUG_ON(!legacy_files);
> +}
> +
>  void __init hugetlb_cgroup_file_init(void)
>  {
>  	struct hstate *h;
>  
> +	__hugetlb_cgroup_file_pre_init();
>  	for_each_hstate(h)
>  		__hugetlb_cgroup_file_init(hstate_index(h));
>  }