On Mon, Jun 13, 2022 at 02:35:08PM +0800, Muchun Song wrote:
> We hold an another reference to hugetlb_optimize_vmemmap_key when
> making vmemmap_optimize_mode on, because we use static_key to tell
> memory_hotplug that memory_hotplug.memmap_on_memory should be
> overridden. However, this rule has gone when we have introduced
> SECTION_CANNOT_OPTIMIZE_VMEMMAP. Therefore, we could simplify
> vmemmap_optimize_mode handling by not holding an another reference
> to hugetlb_optimize_vmemmap_key.
>
> Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
LGTM, and it looks way nicer, so
Reviewed-by: Oscar Salvador <osalvador@xxxxxxx>
One question below though
> -static enum vmemmap_optimize_mode vmemmap_optimize_mode =
> +static bool vmemmap_optimize_enabled =
> IS_ENABLED(CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON);
So, by default vmemmap_optimize_enabled will be on if we have
CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON, but we can always override that
via cmdline, as below, right?
>
> -static void vmemmap_optimize_mode_switch(enum vmemmap_optimize_mode to)
> -{
> - if (vmemmap_optimize_mode == to)
> - return;
> -
> - if (to == VMEMMAP_OPTIMIZE_OFF)
> - static_branch_dec(&hugetlb_optimize_vmemmap_key);
> - else
> - static_branch_inc(&hugetlb_optimize_vmemmap_key);
> - WRITE_ONCE(vmemmap_optimize_mode, to);
> -}
> -
> static int __init hugetlb_vmemmap_early_param(char *buf)
> {
> - bool enable;
> - enum vmemmap_optimize_mode mode;
> -
> - if (kstrtobool(buf, &enable))
> - return -EINVAL;
> -
> - mode = enable ? VMEMMAP_OPTIMIZE_ON : VMEMMAP_OPTIMIZE_OFF;
> - vmemmap_optimize_mode_switch(mode);
> -
> - return 0;
> + return kstrtobool(buf, &vmemmap_optimize_enabled);
> }
> early_param("hugetlb_free_vmemmap", hugetlb_vmemmap_early_param);
>
> @@ -103,7 +76,7 @@ static unsigned int optimizable_vmemmap_pages(struct hstate *h,
> unsigned long pfn = page_to_pfn(head);
> unsigned long end = pfn + pages_per_huge_page(h);
>
> - if (READ_ONCE(vmemmap_optimize_mode) == VMEMMAP_OPTIMIZE_OFF)
> + if (!READ_ONCE(vmemmap_optimize_enabled))
> return 0;
>
> for (; pfn < end; pfn += PAGES_PER_SECTION) {
> @@ -155,7 +128,6 @@ void __init hugetlb_vmemmap_init(struct hstate *h)
>
> if (!is_power_of_2(sizeof(struct page))) {
> pr_warn_once("cannot optimize vmemmap pages because \"struct page\" crosses page boundaries\n");
> - static_branch_disable(&hugetlb_optimize_vmemmap_key);
> return;
> }
>
> @@ -176,36 +148,13 @@ void __init hugetlb_vmemmap_init(struct hstate *h)
> }
>
> #ifdef CONFIG_PROC_SYSCTL
> -static int hugetlb_optimize_vmemmap_handler(struct ctl_table *table, int write,
> - void *buffer, size_t *length,
> - loff_t *ppos)
> -{
> - int ret;
> - enum vmemmap_optimize_mode mode;
> - static DEFINE_MUTEX(sysctl_mutex);
> -
> - if (write && !capable(CAP_SYS_ADMIN))
> - return -EPERM;
> -
> - mutex_lock(&sysctl_mutex);
> - mode = vmemmap_optimize_mode;
> - table->data = &mode;
> - ret = proc_dointvec_minmax(table, write, buffer, length, ppos);
> - if (write && !ret)
> - vmemmap_optimize_mode_switch(mode);
> - mutex_unlock(&sysctl_mutex);
> -
> - return ret;
> -}
> -
> static struct ctl_table hugetlb_vmemmap_sysctls[] = {
> {
> .procname = "hugetlb_optimize_vmemmap",
> - .maxlen = sizeof(enum vmemmap_optimize_mode),
> + .data = &vmemmap_optimize_enabled,
> + .maxlen = sizeof(int),
> .mode = 0644,
> - .proc_handler = hugetlb_optimize_vmemmap_handler,
> - .extra1 = SYSCTL_ZERO,
> - .extra2 = SYSCTL_ONE,
> + .proc_handler = proc_dobool,
> },
> { }
> };
> --
> 2.11.0
>
>
--
Oscar Salvador
SUSE Labs
