Hi,
On 2024/6/18 20:55, Marek Szyprowski wrote:
> Dear All,
>
> On 12.06.2024 11:24, Xiu Jianfeng wrote:
>> 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@xxxxxxxxxx>
>
> This patch landed in yesterday's linux-next (next-20240617) as commit
> 11308a02a56cc ("mm/hugetlb_cgroup: switch to the new cftypes"). In my
> daily tests I found that it triggers the following lock dependency
> checker warning on most of my ARM64 test machines:
>
> BUG: key ffff000005c080d8 has not been registered!
> ------------[ cut here ]------------
> DEBUG_LOCKS_WARN_ON(1)
> WARNING: CPU: 1 PID: 1080 at kernel/locking/lockdep.c:4895
> lockdep_init_map_type+0x1cc/0x284
> Modules linked in: ipv6
> CPU: 1 PID: 1080 Comm: cgmanager Not tainted 6.10.0-rc3+ #1011
> Hardware name: linux,dummy-virt (DT)
> pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : lockdep_init_map_type+0x1cc/0x284
> lr : lockdep_init_map_type+0x1cc/0x284
> ...
> Call trace:
> lockdep_init_map_type+0x1cc/0x284
> __kernfs_create_file+0x7c/0x138
> cgroup_addrm_files+0x170/0x360
> css_populate_dir+0x70/0x174
> cgroup_apply_control_enable+0x128/0x378
> rebind_subsystems+0x384/0x504
> cgroup_setup_root+0x224/0x420
> cgroup1_get_tree+0x248/0x36c
> vfs_get_tree+0x28/0xe8
> path_mount+0x3e8/0xb78
> __arm64_sys_mount+0x1f0/0x2dc
> invoke_syscall+0x48/0x118
> el0_svc_common.constprop.0+0x40/0xe8
> do_el0_svc_compat+0x20/0x3c
> el0_svc_compat+0x44/0xe0
> el0t_32_sync_handler+0x98/0x148
> el0t_32_sync+0x194/0x198
> irq event stamp: 8225
> hardirqs last enabled at (8225): [<ffff800080136134>]
> console_unlock+0x164/0x190
> hardirqs last disabled at (8224): [<ffff800080136120>]
> console_unlock+0x150/0x190
> softirqs last enabled at (8214): [<ffff8000800ae1f4>]
> handle_softirqs+0x4dc/0x4f4
> softirqs last disabled at (8207): [<ffff8000800105d4>]
> __do_softirq+0x14/0x20
> ---[ end trace 0000000000000000 ]---
>
> It looks that something is not properly intialized. Reverting $subject
> on top of linux-next fixes this issue.
Yes, the root cause is that the cft->lockdep_key has not been registered
proactively when the CONFIG_DEBUG_LOCK_ALLOC is enabled.
Would you please try the following patch?
https://lore.kernel.org/all/20240618071922.2127289-1-xiujianfeng@xxxxxxxxxx/
>
> I've used defconfig with some debug options enabled and some drivers
> compiled-in (if this matters):
>
> make ARCH=arm64 defconfig
>
> ./scripts/config -e BLK_DEV_RAM --set-val BLK_DEV_RAM_COUNT 4 --set-val
> BLK_DEV_RAM_SIZE 81920 --set-val CMA_SIZE_MBYTES 96 -e PROVE_LOCKING -e
> DEBUG_ATOMIC_SLEEP -e STAGING -e I2C_GPIO -e PM_DEBUG -e
> PM_ADVANCED_DEBUG -e USB_GADGET -e USB_ETH -e CONFIG_DEVFREQ_THERMAL -e
> CONFIG_BRCMFMAC_PCIE -e CONFIG_NFC -d ARCH_SUNXI -d ARCH_ALPINE -d
> DRM_NOUVEAU -d ARCH_BCM_IPROC -d ARCH_BERLIN -d ARCH_BRCMSTB -d
> ARCH_LAYERSCAPE -d ARCH_LG1K -d ARCH_HISI -d ARCH_MEDIATEK -d ARCH_MVEBU
> -d ARCH_SEATTLE -d ARCH_SYNQUACER -d ARCH_RENESAS -d ARCH_STRATIX10 -d
> ARCH_TEGRA -d ARCH_SPRD -d ARCH_THUNDER -d ARCH_THUNDER2 -d
> ARCH_UNIPHIER -d ARCH_XGENE -d ARCH_ZX -d ARCH_ZYNQMP -d HIBERNATION -d
> CLK_SUNXI -d CONFIG_EFI -d CONFIG_TEE -e FW_CFG_SYSFS
>
>
>> ---
>> 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 43aae8f88d5f..2b899c4ae968 100644
>> --- a/mm/hugetlb_cgroup.c
>> +++ b/mm/hugetlb_cgroup.c
>> @@ -839,164 +839,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)
>> @@ -1011,13 +873,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();
>> }
>>
>> /*
>
> Best regards
