| Message ID | 20220609040342.2703-1-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: sysctl: fix missing numa_stat when !CONFIG_HUGETLB_PAGE | expand |
On Thu 09-06-22 12:03:42, Muchun Song wrote: > "numa_stat" should not be included in the scope of CONFIG_HUGETLB_PAGE, > if CONFIG_HUGETLB_PAGE is not configured even if CONFIG_NUMA is configured, > "numa_stat" is missed form /proc. Remove it out of CONFIG_HUGETLB_PAGE > and move numa_stat sysctl handling to mm/vmstat.c. > > Fixes: 4518085e127d ("mm, sysctl: make NUMA stats configurable") > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > Cc: <stable@vger.kernel.org> It really looks like numa_stat is misplaced but I am wondering why the fix cannot be as simple as diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 830aaf8ca08e..b9c2cd9ed3a2 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -2463,6 +2463,17 @@ static struct ctl_table vm_table[] = { .extra1 = SYSCTL_ZERO, .extra2 = SYSCTL_TWO_HUNDRED, }, +#ifdef CONFIG_NUMA + { + .procname = "numa_stat", + .data = &sysctl_vm_numa_stat, + .maxlen = sizeof(int), + .mode = 0644, + .proc_handler = sysctl_vm_numa_stat_handler, + .extra1 = SYSCTL_ZERO, + .extra2 = SYSCTL_ONE, + }, +#endif #ifdef CONFIG_HUGETLB_PAGE { .procname = "nr_hugepages", @@ -2479,15 +2490,6 @@ static struct ctl_table vm_table[] = { .mode = 0644, .proc_handler = &hugetlb_mempolicy_sysctl_handler, }, - { - .procname = "numa_stat", - .data = &sysctl_vm_numa_stat, - .maxlen = sizeof(int), - .mode = 0644, - .proc_handler = sysctl_vm_numa_stat_handler, - .extra1 = SYSCTL_ZERO, - .extra2 = SYSCTL_ONE, - }, #endif { .procname = "hugetlb_shm_group",
On Thu, Jun 09, 2022 at 10:45:56AM +0200, Michal Hocko wrote: > On Thu 09-06-22 12:03:42, Muchun Song wrote: > > "numa_stat" should not be included in the scope of CONFIG_HUGETLB_PAGE, > > if CONFIG_HUGETLB_PAGE is not configured even if CONFIG_NUMA is configured, > > "numa_stat" is missed form /proc. Remove it out of CONFIG_HUGETLB_PAGE > > and move numa_stat sysctl handling to mm/vmstat.c. > > > > Fixes: 4518085e127d ("mm, sysctl: make NUMA stats configurable") > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > Cc: <stable@vger.kernel.org> > > It really looks like numa_stat is misplaced but I am wondering why the > fix cannot be as simple as > Your changes LGTM. I think I have overdone it. I really should seprate this into two separate patches, one is bug fix (just like your changes), another is simplify the code further. I'll rework this patch. Thanks.
diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h index bfe38869498d..1297a6b8ba23 100644 --- a/include/linux/vmstat.h +++ b/include/linux/vmstat.h @@ -13,12 +13,7 @@ extern int sysctl_stat_interval; #ifdef CONFIG_NUMA -#define ENABLE_NUMA_STAT 1 -#define DISABLE_NUMA_STAT 0 -extern int sysctl_vm_numa_stat; DECLARE_STATIC_KEY_TRUE(vm_numa_stat_key); -int sysctl_vm_numa_stat_handler(struct ctl_table *table, int write, - void *buffer, size_t *length, loff_t *ppos); #endif struct reclaim_stat { diff --git a/kernel/sysctl.c b/kernel/sysctl.c index e52b6e372c60..3d6f36f230bb 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -2107,15 +2107,6 @@ static struct ctl_table vm_table[] = { .mode = 0644, .proc_handler = &hugetlb_mempolicy_sysctl_handler, }, - { - .procname = "numa_stat", - .data = &sysctl_vm_numa_stat, - .maxlen = sizeof(int), - .mode = 0644, - .proc_handler = sysctl_vm_numa_stat_handler, - .extra1 = SYSCTL_ZERO, - .extra2 = SYSCTL_ONE, - }, #endif { .procname = "hugetlb_shm_group", diff --git a/mm/vmstat.c b/mm/vmstat.c index 373d2730fcf2..e10afbee888e 100644 --- a/mm/vmstat.c +++ b/mm/vmstat.c @@ -33,8 +33,6 @@ #include "internal.h" #ifdef CONFIG_NUMA -int sysctl_vm_numa_stat = ENABLE_NUMA_STAT; - /* zero numa counters within a zone */ static void zero_zone_numa_counters(struct zone *zone) { @@ -73,35 +71,37 @@ static void invalid_numa_statistics(void) zero_global_numa_counters(); } -static DEFINE_MUTEX(vm_numa_stat_lock); - -int sysctl_vm_numa_stat_handler(struct ctl_table *table, int write, - void *buffer, size_t *length, loff_t *ppos) +static int sysctl_numa_stat_handler(struct ctl_table *table, int write, + void *buffer, size_t *length, loff_t *ppos) { - int ret, oldval; + int ret; + struct static_key *key = table->data; + static DEFINE_MUTEX(lock); - mutex_lock(&vm_numa_stat_lock); - if (write) - oldval = sysctl_vm_numa_stat; - ret = proc_dointvec_minmax(table, write, buffer, length, ppos); - if (ret || !write) - goto out; - - if (oldval == sysctl_vm_numa_stat) - goto out; - else if (sysctl_vm_numa_stat == ENABLE_NUMA_STAT) { - static_branch_enable(&vm_numa_stat_key); - pr_info("enable numa statistics\n"); - } else { - static_branch_disable(&vm_numa_stat_key); + mutex_lock(&lock); + ret = proc_do_static_key(table, write, buffer, length, ppos); + if (!ret && write && !static_key_enabled(key)) invalid_numa_statistics(); - pr_info("disable numa statistics, and clear numa counters\n"); - } - -out: - mutex_unlock(&vm_numa_stat_lock); + mutex_unlock(&lock); return ret; } + +static struct ctl_table numa_stat_sysctl[] = { + { + .procname = "numa_stat", + .data = &vm_numa_stat_key.key, + .mode = 0644, + .proc_handler = sysctl_numa_stat_handler, + }, + { } +}; + +static int __init numa_stat_sysctl_init(void) +{ + register_sysctl_init("vm", numa_stat_sysctl); + return 0; +} +late_initcall(numa_stat_sysctl_init); #endif #ifdef CONFIG_VM_EVENT_COUNTERS
"numa_stat" should not be included in the scope of CONFIG_HUGETLB_PAGE, if CONFIG_HUGETLB_PAGE is not configured even if CONFIG_NUMA is configured, "numa_stat" is missed form /proc. Remove it out of CONFIG_HUGETLB_PAGE and move numa_stat sysctl handling to mm/vmstat.c. Fixes: 4518085e127d ("mm, sysctl: make NUMA stats configurable") Signed-off-by: Muchun Song <songmuchun@bytedance.com> Cc: <stable@vger.kernel.org> --- include/linux/vmstat.h | 5 ----- kernel/sysctl.c | 9 --------- mm/vmstat.c | 52 +++++++++++++++++++++++++------------------------- 3 files changed, 26 insertions(+), 40 deletions(-)