Re: [PATCH v8 8/8] memblock: Unpreserve memory in case of error

[Mike Rapoport <rppt@kernel.org>]

On Sun, Oct 26, 2025 at 01:41:30PM -0400, Pasha Tatashin wrote:
> On Sun, Oct 26, 2025 at 12:29 PM Mike Rapoport <rppt@kernel.org> wrote:
> >
> > > @@ -2462,12 +2463,14 @@ static int __init prepare_kho_fdt(void)
> > >
> > >       err |= fdt_begin_node(fdt, "");
> > >       err |= fdt_property_string(fdt, "compatible", MEMBLOCK_KHO_NODE_COMPATIBLE);
> > > -     for (i = 0; i < reserved_mem_count; i++) {
> > > +     for (i = 0; !err && i < reserved_mem_count; i++) {
> > >               struct reserve_mem_table *map = &reserved_mem_table[i];
> > >               struct page *page = phys_to_page(map->start);
> > >               unsigned int nr_pages = map->size >> PAGE_SHIFT;
> > >
> > > -             err |= kho_preserve_pages(page, nr_pages);
> > > +             err = kho_preserve_pages(page, nr_pages);
> > > +             if (err)
> > > +                     break;
> >
> > Please
> >
> >         goto err_unpreserve;
> 
> While we can do that, we loose some symmetry of not performing
> fdt_end_node() and fdt_finish() if fdt lib ever adds some debugging
> facility to make sure that open nodes/trees are properly clodes, this
> is going to flag that. I prefer my current implementation.

Why do we care about fdt that we are never going to use and that's freed a
few lines below?

> > >               err |= fdt_begin_node(fdt, map->name);
> > >               err |= fdt_property_string(fdt, "compatible", RESERVE_MEM_KHO_NODE_COMPATIBLE);
> > >               err |= fdt_property(fdt, "start", &map->start, sizeof(map->start));
> >
> >         if (err)
> >                 goto err_unpreserve;
> >
> > and drop !err from the loop condition.
> 
> That is going to miss one 'nr_preserved++' . We cannot do that, we
> could move it to the beginning of the loop, but I prefer keeping err
> right in the condition.
 
I very much dislike the error handling after this patch. From a single

	if (err)
		put_page()

it grew into a complex beast with special variables just for the sake of
it.

What I'd like to see is something like

err_unpreserve_fdt:
	kho_unpreserve_folio(page_folio(fdt_page));
err_unpreserve_mems:
	for (unsigned int i = 0; i < nr_preserved; i++) {
		/* unpreserve mem[i] */
	}
err_free_fdt:
	put_page(fdt_page);
	return err;

-- 
Sincerely yours,
Mike.