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