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

[David Hildenbrand (Arm)]

>> Cheers,
>> 
>> David
> 
> Yay. I did that + dropped the extra lock/unlock and now it's in the noise in
> my testing. needs much more testing of course.

Cool. I'd expect that latency-sensitive workloads (PREEMPT_RT) would not want to
have hwpoison handling either way, so using the no_resched variants at these
places might be doable.

> 
> If you want me to post (including addressing your other feedback) let me
> know.
> 

Let's first discuss the options. We essentially have the following one so far:

1) Ignore the problem

It's been there forever ... but I am not quite happy about that.

2) Use atomics everywhere

The easiest+cleanest, but as measured, the performance hit is real.

3) Keep retrying for a couple of times

The big problem is "how long". A CPU in a hypervisor might be stalled for quite
a while (20s? can be longer).

4) Disable preemption around non-atomic updates + synchronize_rcu() loop

I think it should work, but I don't like the possibly endless retry loop. (well,
it would never be an endless loop in practice)

Is there a problem with synchronize_rcu() latency, given that it can take in bad
scenarios a couple of seconds? (grace periods can be large ... but also very short)

4) disable local interrupts around non-atomic updates + let all CPUs perform
   atomic setting/clearing of the bit through smp_call_function_many().

Disabling local interrupts is way too expensive. :(

5) disable preemption around non-atomic updates + let all CPUs perform
   atomic setting/clearing of the bit through schedule_on_each_cpu()

Mixture of 3) + 4), but schedule_on_each_cpu() can also possibly take a long
time (as long as we cannot schedule on a CPU ...).

We could likely build something that remembers all pages with bits-to-set /
bits-to-clear, to then kick off a per-cpu work ... and once all CPUs processed a
page it was fully processed ... needs much more thought.

6) stop_machine()

Big hammer. I think we'd still need to disable preemption around non-atomic updates.

7) Move hwpoison bit out of page->flags

Use a sparse bitmap. Quite invasive, and any hwpoison bit checking code would
get more expensive.



Did I mess something up / forget something?

> Or look into call_rcu_tasks/call_rcu_tasks_rude which might work without 
> changes in mm.

Looking into call_rcu_tasks() (the first time) the semantics are interesting
("involuntary preemption is not a Tasks RCU quiescent state").

I'm curious how this interacts with random page allocations (on the IRQ path?),
and which design you envision given that we only get a single callback (who
prevents code to re-enter).

> 
> Or if someone else is gonnu work on this, absolutely fine too.

I can likely let someone work on that, but I think we should first figure out
what can really be done. (and likely when we send it out, it should be an RFC)

-- 
Cheers,

David