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

[David Hildenbrand (Arm)]

On 6/30/26 08:27, Michael S. Tsirkin wrote:
> On Tue, Jun 30, 2026 at 08:17:42AM +0200, David Hildenbrand (Arm) wrote:
>> On 6/30/26 01:34, Michael S. Tsirkin wrote:
>>>
>>> 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
> 
> We have:
> 
> #else /* #ifdef CONFIG_PREEMPT_RCU */
> 
> 
> static inline void __rcu_read_lock(void)
> {
>         preempt_disable();
> }
> 
> ...
> 
> 
> static __always_inline void rcu_read_lock(void)
>         __acquires_shared(RCU)
> {
>         __rcu_read_lock();
>         __acquire_shared(RCU);
>         rcu_lock_acquire(&rcu_lock_map);
>         RCU_LOCKDEP_WARN(!rcu_is_watching(),
>                          "rcu_read_lock() used illegally while idle");
> }
> 
> 
> 
> So on non-debug build witout CONFIG_PREEMPT_RCU (what I tested), rcu_lock
> is exactly same as preempt_disable.  It's relatively cheap but not free.
> 
> 
> preempt_disable is not going to be cheaper.

Well, it will be cheaper in the general case (CONFIG_PREEMPT_RCU) :)

But yes, not for this case.

> 
> I can test if you want but it seems clear.
> 

If you measured only !CONFIG_PREEMPT_RCU, then yes, it won't change a thing for
that scenario.

> 
> But IIUC task rcu might be cheaper - IIUC it does not need rcu
> lock/unlock at all, it relies on readers to invoke the scheduler
> instead.
> No?

I thought that still requires protection of sorts (preempt_disable /
rcu_read_lock), because it might fire whenever the task is preempted?

-- 
Cheers,

David