Hi, Alexander
Alexander Gordeev <agordeev@xxxxxxxxxxxxx> wrote on Tuesday, 7 January 2025 23:02
>
> On Sun, Dec 22, 2024 at 07:15:37PM +0800, Guo Weikang wrote:
>
> Hi Guo,
>
> > Before SLUB initialization, various subsystems used memblock_alloc to
> > allocate memory. In most cases, when memory allocation fails, an immediate
> > panic is required. To simplify this behavior and reduce repetitive checks,
> > introduce `memblock_alloc_or_panic`. This function ensures that memory
> > allocation failures result in a panic automatically, improving code
> > readability and consistency across subsystems that require this behavior.
>
> I believe, you also want to make similar function against memblock_alloc_low().
>
About if we need add more interfaces like `or_panic` or `no_panic` , there has been a
discussion, temporarily only modify the most frequently used `memblock_alloc`.
- https://lore.kernel.org/lkml/CAOm6qnm4pN95kJK8bYfu7Z2Bp_6_Gn35v2WyMmYraQ+YMYQEdg@xxxxxxxxxxxxxx/T/#mf09ea8cb9c5e6cbb9a4b592625394b5639c73e1b
> Please, find s390 comments below.
>
Thanks. Since the patch has been merged into `mm-everything`, I will submit another
patch to fix your suggestion.
> ...
>
> > diff --git a/arch/s390/kernel/numa.c b/arch/s390/kernel/numa.c
> > index ddc1448ea2e1..a33e20f73330 100644
> > --- a/arch/s390/kernel/numa.c
> > +++ b/arch/s390/kernel/numa.c
> > @@ -22,10 +22,7 @@ void __init numa_setup(void)
> > node_set(0, node_possible_map);
> > node_set_online(0);
> > for (nid = 0; nid < MAX_NUMNODES; nid++) {
> > - NODE_DATA(nid) = memblock_alloc(sizeof(pg_data_t), 8);
> > - if (!NODE_DATA(nid))
> > - panic("%s: Failed to allocate %zu bytes align=0x%x\n",
> > - __func__, sizeof(pg_data_t), 8);
> > + NODE_DATA(nid) = memblock_alloc_or_panic(sizeof(pg_data_t), 8);
> > }
>
> Please, also remove the cycle body brackets.
Sure.
>
> > NODE_DATA(0)->node_spanned_pages = memblock_end_of_DRAM() >> PAGE_SHIFT;
> > NODE_DATA(0)->node_id = 0;
> > diff --git a/arch/s390/kernel/setup.c b/arch/s390/kernel/setup.c
> > index 0ce550faf073..1298f0860733 100644
> > --- a/arch/s390/kernel/setup.c
> > +++ b/arch/s390/kernel/setup.c
> > @@ -376,11 +376,7 @@ static unsigned long __init stack_alloc_early(void)
> > {
> > unsigned long stack;
> >
> > - stack = (unsigned long)memblock_alloc(THREAD_SIZE, THREAD_SIZE);
> > - if (!stack) {
> > - panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> > - __func__, THREAD_SIZE, THREAD_SIZE);
> > - }
> > + stack = (unsigned long)memblock_alloc_or_panic(THREAD_SIZE, THREAD_SIZE);
> > return stack;
> > }
> >
> > @@ -504,10 +500,7 @@ static void __init setup_resources(void)
> > bss_resource.end = __pa_symbol(__bss_stop) - 1;
> >
> > for_each_mem_range(i, &start, &end) {
> > - res = memblock_alloc(sizeof(*res), 8);
> > - if (!res)
> > - panic("%s: Failed to allocate %zu bytes align=0x%x\n",
> > - __func__, sizeof(*res), 8);
> > + res = memblock_alloc_or_panic(sizeof(*res), 8);
> > res->flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM;
> >
> > res->name = "System RAM";
> > @@ -526,10 +519,7 @@ static void __init setup_resources(void)
> > std_res->start > res->end)
> > continue;
> > if (std_res->end > res->end) {
> > - sub_res = memblock_alloc(sizeof(*sub_res), 8);
> > - if (!sub_res)
> > - panic("%s: Failed to allocate %zu bytes align=0x%x\n",
> > - __func__, sizeof(*sub_res), 8);
> > + sub_res = memblock_alloc_or_panic(sizeof(*sub_res), 8);
> > *sub_res = *std_res;
> > sub_res->end = res->end;
> > std_res->start = res->end + 1;
> > @@ -816,9 +806,7 @@ static void __init setup_randomness(void)
> > {
> > struct sysinfo_3_2_2 *vmms;
> >
> > - vmms = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> > - if (!vmms)
> > - panic("Failed to allocate memory for sysinfo structure\n");
> > + vmms = memblock_alloc_or_panic(PAGE_SIZE, PAGE_SIZE);
> > if (stsi(vmms, 3, 2, 2) == 0 && vmms->count)
> > add_device_randomness(&vmms->vm, sizeof(vmms->vm[0]) * vmms->count);
> > memblock_free(vmms, PAGE_SIZE);
> > diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
> > index 822d8e6f8717..d77aaefb59bd 100644
> > --- a/arch/s390/kernel/smp.c
> > +++ b/arch/s390/kernel/smp.c
> > @@ -611,9 +611,9 @@ void __init smp_save_dump_ipl_cpu(void)
> > if (!dump_available())
> > return;
> > sa = save_area_alloc(true);
> > - regs = memblock_alloc(512, 8);
> > - if (!sa || !regs)
> > + if (!sa)
> > panic("could not allocate memory for boot CPU save area\n");
>
> Please, replace memblock_alloc() with memblock_alloc_or_panic() in
> save_area_alloc() and remove the error handling here and also in
> smp_save_dump_secondary_cpus().
OK, I confirmed that all the places where save_area_alloc is called have panic handle.
>
> > + regs = memblock_alloc_or_panic(512, 8);
> > copy_oldmem_kernel(regs, __LC_FPREGS_SAVE_AREA, 512);
> > save_area_add_regs(sa, regs);
> > memblock_free(regs, 512);
> > @@ -792,10 +792,7 @@ void __init smp_detect_cpus(void)
> > u16 address;
> >
> > /* Get CPU information */
> > - info = memblock_alloc(sizeof(*info), 8);
> > - if (!info)
> > - panic("%s: Failed to allocate %zu bytes align=0x%x\n",
> > - __func__, sizeof(*info), 8);
> > + info = memblock_alloc_or_panic(sizeof(*info), 8);
> > smp_get_core_info(info, 1);
> > /* Find boot CPU type */
> > if (sclp.has_core_type) {
> > diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
> > index 0fd56a1cadbd..cf5ee6032c0b 100644
> > --- a/arch/s390/kernel/topology.c
> > +++ b/arch/s390/kernel/topology.c
> > @@ -548,10 +548,7 @@ static void __init alloc_masks(struct sysinfo_15_1_x *info,
> > nr_masks *= info->mag[TOPOLOGY_NR_MAG - offset - 1 - i];
> > nr_masks = max(nr_masks, 1);
> > for (i = 0; i < nr_masks; i++) {
> > - mask->next = memblock_alloc(sizeof(*mask->next), 8);
> > - if (!mask->next)
> > - panic("%s: Failed to allocate %zu bytes align=0x%x\n",
> > - __func__, sizeof(*mask->next), 8);
> > + mask->next = memblock_alloc_or_panic(sizeof(*mask->next), 8);
> > mask = mask->next;
> > }
> > }
> > @@ -569,10 +566,7 @@ void __init topology_init_early(void)
> > }
> > if (!MACHINE_HAS_TOPOLOGY)
> > goto out;
> > - tl_info = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> > - if (!tl_info)
> > - panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> > - __func__, PAGE_SIZE, PAGE_SIZE);
> > + tl_info = memblock_alloc_or_panic(PAGE_SIZE, PAGE_SIZE);
> > info = tl_info;
> > store_topology(info);
> > pr_info("The CPU configuration topology of the machine is: %d %d %d %d %d %d / %d\n",
>
> Thanks!
Alexander Gordeev <agordeev@xxxxxxxxxxxxx> wrote on Tuesday, 7 January 2025 23:02
>
> On Sun, Dec 22, 2024 at 07:15:37PM +0800, Guo Weikang wrote:
>
> Hi Guo,
>
> > Before SLUB initialization, various subsystems used memblock_alloc to
> > allocate memory. In most cases, when memory allocation fails, an immediate
> > panic is required. To simplify this behavior and reduce repetitive checks,
> > introduce `memblock_alloc_or_panic`. This function ensures that memory
> > allocation failures result in a panic automatically, improving code
> > readability and consistency across subsystems that require this behavior.
>
> I believe, you also want to make similar function against memblock_alloc_low().
>
About if we need add more interfaces like `or_panic` or `no_panic` , there has been a
discussion, temporarily only modify the most frequently used `memblock_alloc`.
- https://lore.kernel.org/lkml/CAOm6qnm4pN95kJK8bYfu7Z2Bp_6_Gn35v2WyMmYraQ+YMYQEdg@xxxxxxxxxxxxxx/T/#mf09ea8cb9c5e6cbb9a4b592625394b5639c73e1b
> Please, find s390 comments below.
>
Thanks. Since the patch has been merged into `mm-everything`, I will submit another
patch to fix your suggestion.
> ...
>
> > diff --git a/arch/s390/kernel/numa.c b/arch/s390/kernel/numa.c
> > index ddc1448ea2e1..a33e20f73330 100644
> > --- a/arch/s390/kernel/numa.c
> > +++ b/arch/s390/kernel/numa.c
> > @@ -22,10 +22,7 @@ void __init numa_setup(void)
> > node_set(0, node_possible_map);
> > node_set_online(0);
> > for (nid = 0; nid < MAX_NUMNODES; nid++) {
> > - NODE_DATA(nid) = memblock_alloc(sizeof(pg_data_t), 8);
> > - if (!NODE_DATA(nid))
> > - panic("%s: Failed to allocate %zu bytes align=0x%x\n",
> > - __func__, sizeof(pg_data_t), 8);
> > + NODE_DATA(nid) = memblock_alloc_or_panic(sizeof(pg_data_t), 8);
> > }
>
> Please, also remove the cycle body brackets.
Sure.
>
> > NODE_DATA(0)->node_spanned_pages = memblock_end_of_DRAM() >> PAGE_SHIFT;
> > NODE_DATA(0)->node_id = 0;
> > diff --git a/arch/s390/kernel/setup.c b/arch/s390/kernel/setup.c
> > index 0ce550faf073..1298f0860733 100644
> > --- a/arch/s390/kernel/setup.c
> > +++ b/arch/s390/kernel/setup.c
> > @@ -376,11 +376,7 @@ static unsigned long __init stack_alloc_early(void)
> > {
> > unsigned long stack;
> >
> > - stack = (unsigned long)memblock_alloc(THREAD_SIZE, THREAD_SIZE);
> > - if (!stack) {
> > - panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> > - __func__, THREAD_SIZE, THREAD_SIZE);
> > - }
> > + stack = (unsigned long)memblock_alloc_or_panic(THREAD_SIZE, THREAD_SIZE);
> > return stack;
> > }
> >
> > @@ -504,10 +500,7 @@ static void __init setup_resources(void)
> > bss_resource.end = __pa_symbol(__bss_stop) - 1;
> >
> > for_each_mem_range(i, &start, &end) {
> > - res = memblock_alloc(sizeof(*res), 8);
> > - if (!res)
> > - panic("%s: Failed to allocate %zu bytes align=0x%x\n",
> > - __func__, sizeof(*res), 8);
> > + res = memblock_alloc_or_panic(sizeof(*res), 8);
> > res->flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM;
> >
> > res->name = "System RAM";
> > @@ -526,10 +519,7 @@ static void __init setup_resources(void)
> > std_res->start > res->end)
> > continue;
> > if (std_res->end > res->end) {
> > - sub_res = memblock_alloc(sizeof(*sub_res), 8);
> > - if (!sub_res)
> > - panic("%s: Failed to allocate %zu bytes align=0x%x\n",
> > - __func__, sizeof(*sub_res), 8);
> > + sub_res = memblock_alloc_or_panic(sizeof(*sub_res), 8);
> > *sub_res = *std_res;
> > sub_res->end = res->end;
> > std_res->start = res->end + 1;
> > @@ -816,9 +806,7 @@ static void __init setup_randomness(void)
> > {
> > struct sysinfo_3_2_2 *vmms;
> >
> > - vmms = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> > - if (!vmms)
> > - panic("Failed to allocate memory for sysinfo structure\n");
> > + vmms = memblock_alloc_or_panic(PAGE_SIZE, PAGE_SIZE);
> > if (stsi(vmms, 3, 2, 2) == 0 && vmms->count)
> > add_device_randomness(&vmms->vm, sizeof(vmms->vm[0]) * vmms->count);
> > memblock_free(vmms, PAGE_SIZE);
> > diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
> > index 822d8e6f8717..d77aaefb59bd 100644
> > --- a/arch/s390/kernel/smp.c
> > +++ b/arch/s390/kernel/smp.c
> > @@ -611,9 +611,9 @@ void __init smp_save_dump_ipl_cpu(void)
> > if (!dump_available())
> > return;
> > sa = save_area_alloc(true);
> > - regs = memblock_alloc(512, 8);
> > - if (!sa || !regs)
> > + if (!sa)
> > panic("could not allocate memory for boot CPU save area\n");
>
> Please, replace memblock_alloc() with memblock_alloc_or_panic() in
> save_area_alloc() and remove the error handling here and also in
> smp_save_dump_secondary_cpus().
OK, I confirmed that all the places where save_area_alloc is called have panic handle.
>
> > + regs = memblock_alloc_or_panic(512, 8);
> > copy_oldmem_kernel(regs, __LC_FPREGS_SAVE_AREA, 512);
> > save_area_add_regs(sa, regs);
> > memblock_free(regs, 512);
> > @@ -792,10 +792,7 @@ void __init smp_detect_cpus(void)
> > u16 address;
> >
> > /* Get CPU information */
> > - info = memblock_alloc(sizeof(*info), 8);
> > - if (!info)
> > - panic("%s: Failed to allocate %zu bytes align=0x%x\n",
> > - __func__, sizeof(*info), 8);
> > + info = memblock_alloc_or_panic(sizeof(*info), 8);
> > smp_get_core_info(info, 1);
> > /* Find boot CPU type */
> > if (sclp.has_core_type) {
> > diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
> > index 0fd56a1cadbd..cf5ee6032c0b 100644
> > --- a/arch/s390/kernel/topology.c
> > +++ b/arch/s390/kernel/topology.c
> > @@ -548,10 +548,7 @@ static void __init alloc_masks(struct sysinfo_15_1_x *info,
> > nr_masks *= info->mag[TOPOLOGY_NR_MAG - offset - 1 - i];
> > nr_masks = max(nr_masks, 1);
> > for (i = 0; i < nr_masks; i++) {
> > - mask->next = memblock_alloc(sizeof(*mask->next), 8);
> > - if (!mask->next)
> > - panic("%s: Failed to allocate %zu bytes align=0x%x\n",
> > - __func__, sizeof(*mask->next), 8);
> > + mask->next = memblock_alloc_or_panic(sizeof(*mask->next), 8);
> > mask = mask->next;
> > }
> > }
> > @@ -569,10 +566,7 @@ void __init topology_init_early(void)
> > }
> > if (!MACHINE_HAS_TOPOLOGY)
> > goto out;
> > - tl_info = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> > - if (!tl_info)
> > - panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> > - __func__, PAGE_SIZE, PAGE_SIZE);
> > + tl_info = memblock_alloc_or_panic(PAGE_SIZE, PAGE_SIZE);
> > info = tl_info;
> > store_topology(info);
> > pr_info("The CPU configuration topology of the machine is: %d %d %d %d %d %d / %d\n",
>
> Thanks!
---
Guo
