Received-SPF: pass (google.com: domain of linux-kernel+bounces-209183-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <ff7f7d2d-d5fc-2839-cbdb-9bab767bc00f@huawei.com>
Date: Tue, 11 Jun 2024 10:29:29 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.1
Subject: Re: [PATCH v2 -next 2/3] mm/hugetlb_cgroup: prepare cftypes based on
 template
Content-Language: en-US
From: xiujianfeng <xiujianfeng@huawei.com>
To: <akpm@linux-foundation.org>, <muchun.song@linux.dev>, Oscar Salvador
	<osalvador@suse.de>
CC: <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>
References: <20240605070133.1941677-1-xiujianfeng@huawei.com>
 <20240605070133.1941677-3-xiujianfeng@huawei.com>
In-Reply-To: <20240605070133.1941677-3-xiujianfeng@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit


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