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:34:44AM +0200, David Hildenbrand (Arm) wrote:
> 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?

Why do you think so?

>From Documentation/RCU/Design/Requirements/Requirements.rst


Note well that involuntary context switches are *not* Tasks-RCU quiescent
states.  After all, in preemptible kernels, a task executing code in a
trampoline might be preempted.  In this case, the Tasks-RCU grace period
clearly cannot end until that task resumes and its execution leaves that
trampoline. 



-- 
MST