Re: [PATCH 0/2] mm: memory-failure: fix HWPoison flag race with non-atomic page flag ops

[David Hildenbrand (Arm)]

On 6/30/26 01:34, Michael S. Tsirkin wrote:
> On Mon, Jun 29, 2026 at 11:43:32PM +0200, David Hildenbrand (Arm) wrote:
>> On 6/29/26 23:22, David Hildenbrand (Arm) wrote:
>>> [...]
>>>
>>>
>>> Fully agreed. I was hoping RCU was cheaper (I mean, we were once told that RCU
>>> read side locking is essentially for free ... well in some configs :) )
>>>
>>> The question if we could optimize it reasonably enough ...
>>>
>>>
>>> ... for example, by doing the rcu read lock + unlock around the
>>>
>>> 	for (i = 1; i < (1 << order); i++) {
>>>
>>> loop on the alloc path. But I suspect it's not going to make that much of a
>>> difference.
>>>
>>> I concluded, similar to Andi, that stop_machine() is too big of a hammer.
>>>
>>> I wonder if something could be built out of preempt_disable() and sync SMP
>>> calls. hmm :(
>>
>> Scrap that, shouldn't work I think ...
>>
> 
> Wait a sec, what about call_rcu_tasks? Use that and re-check the bit is
> still set?

So, in essence the idea I had yestarday when it was late was the following:

Assume we

1) Can have a way to guarantee that a function on a CPU cannot execute within
our critical section (while updating the flags)

2) We can request to execute a function on each CPU and wait for completion

I think we could just let each CPU execute our desired action (e.g., try setting
the bit).

E.g.,

	local_irq_save(flags);
	page->flags &= whatever;
	local_irq_restore(flags);

And assume we want to set the bit, do a

	SetPageHWPoison(page);
	smp_call_function(set_hwpoison_smp_sync, page, 1);

whereby

	static void set_hwpoison_smp_sync(void *info)
	{
		SetPageHWPoison(page);
	}


The idea is (that needs double checking) that a CPU will execute the
SetPageHWPoison() either before the local_irq_save() or after the
local_irq_restore(). So it's own non-atomic update cannot get interrupted.

Now, IIUC when it comes to "how expensive is this" I think we have (cheap to
expensive):

1) preempt_disable()
2) rcu_read_lock()
3) local_irq_save()


So the above wouldn't be better than an rcu-based approach we have right now.
We'd need something that relies on disabled preemption only.

Huh, but I read that "anything that disables preemption also marks an RCU-sched
read-side critical section including preempt_disable() and preempt_enable()".

So for our use case we should be able to use preempt_disable() instead of
local_irq_save(). That should already work for your existing implementation.

-- 
Cheers,

David