Messages in this thread Patch in this message |  | | Date | Thu, 10 Apr 2025 17:14:53 +0530 | | From | Aditya Gupta <> | | Subject | Re: [REPORT] Softlockups on PowerNV with upstream |
| |
Hi,
On 25/04/10 07:25AM, Oscar Salvador wrote: > On Wed, Apr 09, 2025 at 11:33:44PM +0530, Aditya Gupta wrote: > > Hi, > > > > While booting current upstream kernel, I consistently get "softlockups", on IBM PowerNV system. > > > > I have tested it only on PowerNV systems. But some architectures/platforms also > > might have it. PSeries systems don't have this issue though. > > > > Bisect points to the following commit: > > > > commit 61659efdb35ce6c6ac7639342098f3c4548b794b > > Author: Gavin Shan <gshan@redhat.com> > > Date: Wed Mar 12 09:30:43 2025 +1000 > > > > drivers/base/memory: improve add_boot_memory_block() > > > ... > > Console log > > ----------- > > > > [ 2.783371] smp: Brought up 4 nodes, 256 CPUs > > [ 2.783475] numa: Node 0 CPUs: 0-63 > > [ 2.783537] numa: Node 2 CPUs: 64-127 > > [ 2.783591] numa: Node 4 CPUs: 128-191 > > [ 2.783653] numa: Node 6 CPUs: 192-255 > > [ 2.804945] Memory: 735777792K/738197504K available (17536K kernel code, 5760K rwdata, 15232K rodata, 6528K init, 2517K bss, 1369664K reserved, 0K cma-reserved) > > If I am not mistaken this is ~700GB, and PowerNV uses 16MB as section size, > and sections_per_block == 1 (I think).
Yes, the memory is around 700G:
# lsmem RANGE SIZE STATE REMOVABLE BLOCK 0x0000000000000000-0x0000001fffffffff 128G online yes 0-127 0x0000400000000000-0x0000400fffffffff 64G online yes 65536-65599 0x0000800000000000-0x0000803fffffffff 256G online yes 131072-131327 0x0000c00000000000-0x0000c03fffffffff 256G online yes 196608-196863 Memory block size: 1G Total online memory: 704G Total offline memory: 0B
I don't know about the sections_per_block.
> > The code before the mentioned commit, was something like: > > for (nr = base_section_nr; nr < base_section_nr + sections_per_block; nr++) > if (present_section_nr(nr)) > section_count++; > > if (section_count == 0) > return 0; > return add_memory_block() > > So, in case of PowerNV , we will just check one section at a time and > either return or call add_memory_block depending whether it is present. > > Now, with the current code that is something different. > We now have > > memory_dev_init: > for(nr = 0, nr <= __highest_present_section_nr; nr += 1) > ret = add_boot_memory_block > > add_boot_memory_block: > for_each_present_section_nr(base_section_nr, nr) { > if (nr >= (base_section_nr + sections_per_block)) > break; > > return add_memory_block(); > } > return 0; > > The thing is that next_present_section_nr() (which is called in > for_each_present_section_nr()) will loop until we find a present > section. > And then we will check whether the found section is beyond > base_section_nr + sections_per_block (where sections_per_block = 1). > If so, we skip add_memory_block. > > Now, I think that the issue comes from for_each_present_section_nr > having to loop a lot until we find a present section. > And then the loop in memory_dev_init increments only by 1, which means > that the next iteration we might have to loop a lot again to find the > another present section. And so on and so forth. > > Maybe we can fix this by making memory_dev_init() remember in which > section add_boot_memory_block returns. > Something like the following (only compile-tested) > > diff --git a/drivers/base/memory.c b/drivers/base/memory.c > index 8f3a41d9bfaa..d97635cbfd1d 100644 > --- a/drivers/base/memory.c > +++ b/drivers/base/memory.c > @@ -816,18 +816,25 @@ static int add_memory_block(unsigned long block_id, unsigned long state, > return 0; > } > > -static int __init add_boot_memory_block(unsigned long base_section_nr) > +static int __init add_boot_memory_block(unsigned long *base_section_nr) > { > + int ret; > unsigned long nr; > > - for_each_present_section_nr(base_section_nr, nr) { > - if (nr >= (base_section_nr + sections_per_block)) > + for_each_present_section_nr(*base_section_nr, nr) { > + if (nr >= (*base_section_nr + sections_per_block)) > break; > > - return add_memory_block(memory_block_id(base_section_nr), > - MEM_ONLINE, NULL, NULL); > + ret = add_memory_block(memory_block_id(*base_section_nr), > + MEM_ONLINE, NULL, NULL); > + *base_section = nr; > + return ret; > } > > + if (nr == -1) > + *base_section = __highest_present_section_nr + 1; > + else > + *base_section = nr; > return 0; > } > > @@ -973,9 +980,9 @@ void __init memory_dev_init(void) > * Create entries for memory sections that were found > * during boot and have been initialized > */ > - for (nr = 0; nr <= __highest_present_section_nr; > - nr += sections_per_block) { > - ret = add_boot_memory_block(nr); > + nr = first_present_section_nr(); > + for (; nr <= __highest_present_section_nr; nr += sections_per_block) { > + ret = add_boot_memory_block(&nr); > if (ret) > panic("%s() failed to add memory block: %d\n", __func__, > ret); >
Makes sense, thanks for the nice explanation.
> > @Aditya: can you please give it a try? >
Yes, will try it now.
Thanks, - Aditya G
> > > -- > Oscar Salvador > SUSE Labs
|  |