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@xxxxxxxxxx>
> ---
>  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));
>  }