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

[David Hildenbrand (Arm)]

On 9/16/26 11:35, Breno Leitao wrote:
> Hello David,
> 
> First of all, thanks for your time looking at this patchset.
> 
> On Wed, Sep 16, 2026 at 08:48:56AM +0200, David Hildenbrand (Arm) wrote:
>> On 9/15/26 14:53, Breno Leitao wrote:
>>> +static void memblk_nr_poison_init(struct memory_block *mem)
>>> +{
>>> +	unsigned long pfn = section_nr_to_pfn(mem->start_section_nr);
>>> +	unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
>>> +	unsigned long i, nr_poison = 0;
>>> +
>>> +	/* A hotplugged block is created before its pages are online. */
>>> +	if (mem->state != MEM_ONLINE)
>>> +		return;
>>> +
>>> +	if (!range_contains_poisoned_memory(PFN_PHYS(pfn),
>>> +					    nr_pages << PAGE_SHIFT))
>>> +		return;
>>> +
>>> +	for (i = 0; i < nr_pages; i++) {
>>> +		struct page *page = pfn_to_online_page(pfn + i);
>>> +
>>> +		if (page && PageHWPoison(page))
>>> +			nr_poison++;
>>> +	}
>>
>> That just slows down boot unnecessarily on 99.9999999999999999999% of all
>> systems out there.
> 
> 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.

-- 
Cheers,

David