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

[Michael S. Tsirkin]

On Tue, Jun 30, 2026 at 08:17:42AM +0200, David Hildenbrand (Arm) wrote:
> 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

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.

I can test if you want but it seems clear.


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?

-- 
MST