Re: [PATCH v5 7/9] drivers/base/memory: count inherited poisoned frames into the block

[Breno Leitao <leitao@debian.org>]

On Fri, Sep 18, 2026 at 02:18:03PM +0200, David Hildenbrand (Arm) wrote:
> On 9/17/26 15:01, Breno Leitao wrote:
> > On Wed, Sep 16, 2026 at 04:51:40PM +0200, David Hildenbrand (Arm) wrote:
> >> On 9/16/26 11:35, Breno Leitao wrote:
> >>>
> >>> hmmm, I am not sure I see it that way.
> >>>
> >>> The loop only runs for a block the bitmap marks. On a machine with nothing
> >>> recorded the bitmap is all zeros, the range_contains_poisoned_memory() check
> >>> right above it returns false, and the loop never executes.
> >>>
> >>> What every boot does pay is that check, once per block. The stub installs
> >>> the table whether or not anything was ever recorded in it, so this is not a
> >>> NULL test: it is two 64-bit divisions by the unit size plus a
> >>> find_next_bit() over the single word a 128M block covers at one bit per 2M.
> >>>
> >>> The real cost (that "for loop above"), comes when you kexec (not on cold
> >>> boot -- given the bitmap is empty), and you are trying to init
> >>> a memory block that has poisoned pages into it. Which seems the right
> >>> trade-off, no?
> >>>
> >>> That said, can we do better? Yes. The silly win is to let the table say
> >>> whether anything was ever recorded in it, something like a
> >>> linux_efi_poisoned_memory->empty that the first recorded frame clears,
> >>> and return on that before the bitmap is reached at all.
> >>>
> >>> Is this what you are looking for, or something more drastic?
> >>
> >> Ah, that magical "range_contains_poisoned_memory" does a bitmap scan?
> >>
> >> I'm sorry, but that is absolutely confusing.
> >>
> >> There is no way someone will figure out that range_contains_poisoned_memory()
> >> queries some efi specific bitmap that won't even be able to represent any memory
> >> outside of it's range.
> >>
> >> I don't really have time to give a better solution, but starting with the
> >> naming, range_contains_poisoned_memory() is just absolutely misleading.
> > 
> > Fair point, I'll clean up the naming in the next revision.
> > 
> > I'll also add that ->empty field, which should help locate this bit
> > faster and may let us skip the bitmap query entirely on the happy path.
> > 
> > Anything else you'd like addressed?
> > 
> > Good to know this moved the needle from "David hates this feature" to
> > "David only hates the naming" -- I'll take that as progress.  :-P
> 
> ;)
> 
> I think the crucial part is to find a way to cleanly distinguish our source of
> information, and also how the source does only apply to some memory.

Right, we have two source for poisoned page information, today.

1) LINUX_EFI_POISONED_MEMORY: Used to track memory block that got
   poisioned, and will be passed around during kexec.
2) PG_hwpoison on struct page: Used by the memory subsystem to avoid
   touching it.

And I understand that this design is fine, and we want to be easy to
identify what we are querying on function name. For instance,
I understand you confusion in range_contains_poisoned_memory() came
from:

range_contains_poisoned_memory():
   * What the caller reads: 
	* "is any memory in this range hardware poisoned?"
   * What actually runs:
	* "is any bit set in an EFI table that a PREVIOUS kernel wrote,
	   at 2 MiB granularity, for this memory region?"

So, I think think this is a naming issue, and I need to think more about
it. Maybe appending efiposioned (on data that is coming from EFI config
table). Let me think more about it.

> Regarding this patch here, I'd assume it's sufficient.
> 
> But I do wonder why we are walking pages when we have a bitmap to walk/process
> at hand?

Because the counter has to agree with the page flag, and the page flag is
not the bitmap.

PG_hwpoison in a block is the union of every source that poisoned a
frame; the inherited EFI table is one of them, and it is the coarse and
partial one.

You might ask why I do not just count the bits set in the bitmape and
multiply by the frames a unit covers.

That was my first try. It over-counts: a bit stands for a whole 2M unit,
but only the frames that reach __free_pages_core() get flagged -- CMA
comes back through __free_pages(), the initrd and __init memory through
free_reserved_pages(), and KHO-preserved frames never get an initialised
struct page at all.

And the over-count cannot be undone later. 

If the walk itself is what bothers you, the way out is not the bitmap
but counting as we flag: hwpoison_boot_page() already knows the pfn, so
it can bump a per-block-id counter in a small memblock array that
memblk_nr_poison_init() then just reads.

Exact by construction, no walk. Happy to go that way instead if you
prefer it.