Re: [PATCH v3 06/13] mm: introduce generic lazy_mmu helpers

[David Hildenbrand <david@xxxxxxxxxx>]

On 24.10.25 16:32, Kevin Brodsky wrote:
> On 24/10/2025 15:27, David Hildenbrand wrote:
> > On 24.10.25 14:13, Kevin Brodsky wrote:
> > > On 23/10/2025 21:52, David Hildenbrand wrote:
> > > > On 15.10.25 10:27, Kevin Brodsky wrote:
> > > > > [...]
> > > > >
> > > > > * madvise_*_pte_range() call arch_leave() in multiple paths, some
> > > > >      followed by an immediate exit/rescheduling and some followed by a
> > > > >      conditional exit. These functions assume that they are called
> > > > >      with lazy MMU disabled and we cannot simply use pause()/resume()
> > > > >      to address that. This patch leaves the situation unchanged by
> > > > >      calling enable()/disable() in all cases.
> > > > I'm confused, the function simply does
> > > >
> > > > (a) enables lazy mmu
> > > > (b) does something on the page table
> > > > (c) disables lazy mmu
> > > > (d) does something expensive (split folio -> take sleepable locks,
> > > >       flushes tlb)
> > > > (e) go to (a)
> > > That step is conditional: we exit right away if pte_offset_map_lock()
> > > fails. The fundamental issue is that pause() must always be matched with
> > > resume(), but as those functions look today there is no situation where
> > > a pause() would always be matched with a resume().
> > We have matches enable/disable, so my question is rather "why" you are
> > even thinking about using pause/resume?
> >
> > What would be the benefit of that? If there is no benefit then just
> > drop this from the patch description as it's more confusing than just
> > ... doing what the existing code does :)
> Ah sorry I misunderstood, I guess you originally meant: why would we use
> pause()/resume()?
>
> The issue is the one I mentioned in the commit message: using
> enable()/disable(), we assume that the functions are called with lazy
> MMU mode is disabled. Consider:
>
>    lazy_mmu_mode_enable()
>    madvise_cold_or_pageout_pte_range():
>      lazy_mmu_mode_enable()
>      ...
>      if (need_resched()) {
>        lazy_mmu_mode_disable()
>        cond_resched() // lazy MMU still enabled
>      }
>
> This will explode on architectures that do not allow sleeping while in
> lazy MMU mode. I'm not saying this is an actual problem - I don't see
> why those functions would be called with lazy MMU mode enabled. But it
> does go against the notion that nesting works everywhere.
I would tackle it from a different direction: if code calls with lazy 
MMU enabled into random other code that might sleep, that caller would 
be wrong.
It's not about changing functions like this to use pause/resume.

Maybe the rule is simple: if you enable the lazy MMU, don't call any 
functions that might sleep.
Maybe we could support that later by handling it before/after sleeping, 
if ever required?
Or am I missing something regarding your point on pause()/resume()?

--
Cheers

David / dhildenb