Re: [PATCH v2 2/7] mm: introduce local state for lazy_mmu sections

[Juergen Gross <jgross@xxxxxxxx>]

On 09.09.25 11:07, David Hildenbrand wrote:
> On 08.09.25 09:39, Kevin Brodsky wrote:
> > arch_{enter,leave}_lazy_mmu_mode() currently have a stateless API
> > (taking and returning no value). This is proving problematic in
> > situations where leave() needs to restore some context back to its
> > original state (before enter() was called). In particular, this
> > makes it difficult to support the nesting of lazy_mmu sections -
> > leave() does not know whether the matching enter() call occurred
> > while lazy_mmu was already enabled, and whether to disable it or
> > not.
> >
> > This patch gives all architectures the chance to store local state
> > while inside a lazy_mmu section by making enter() return some value,
> > storing it in a local variable, and having leave() take that value.
> > That value is typed lazy_mmu_state_t - each architecture defining
> > __HAVE_ARCH_ENTER_LAZY_MMU_MODE is free to define it as it sees fit.
> > For now we define it as int everywhere, which is sufficient to
> > support nesting.
> >
> > The diff is unfortunately rather large as all the API changes need
> > to be done atomically. Main parts:
> >
> > * Changing the prototypes of arch_{enter,leave}_lazy_mmu_mode()
> >    in generic and arch code, and introducing lazy_mmu_state_t.
> >
> > * Introducing LAZY_MMU_{DEFAULT,NESTED} for future support of
> >    nesting. enter() always returns LAZY_MMU_DEFAULT for now.
> >    (linux/mm_types.h is not the most natural location for defining
> >    those constants, but there is no other obvious header that is
> >    accessible where arch's implement the helpers.)
> >
> > * Changing all lazy_mmu sections to introduce a lazy_mmu_state
> >    local variable, having enter() set it and leave() take it. Most of
> >    these changes were generated using the following Coccinelle script:
> >
> > @@
> > @@
> > {
> > + lazy_mmu_state_t lazy_mmu_state;
> > ...
> > - arch_enter_lazy_mmu_mode();
> > + lazy_mmu_state = arch_enter_lazy_mmu_mode();
> > ...
> > - arch_leave_lazy_mmu_mode();
> > + arch_leave_lazy_mmu_mode(lazy_mmu_state);
> > ...
> > }
> >
> > * In a few cases (e.g. xen_flush_lazy_mmu()), a function knows that
> >    lazy_mmu is already enabled, and it temporarily disables it by
> >    calling leave() and then enter() again. Here we want to ensure
> >    that any operation between the leave() and enter() calls is
> >    completed immediately; for that reason we pass LAZY_MMU_DEFAULT to
> >    leave() to fully disable lazy_mmu. enter() will then re-enable it
> >    - this achieves the expected behaviour, whether nesting occurred
> >    before that function was called or not.
> >
> > Note: it is difficult to provide a default definition of
> > lazy_mmu_state_t for architectures implementing lazy_mmu, because
> > that definition would need to be available in
> > arch/x86/include/asm/paravirt_types.h and adding a new generic
> >   #include there is very tricky due to the existing header soup.
> Yeah, I was wondering about exactly that.
>
> In particular because LAZY_MMU_DEFAULT etc resides somewehere compeltely 
> different.
>
> Which raises the question: is using a new type really of any benefit here?
>
> Can't we just use an "enum lazy_mmu_state" and call it a day?
The comment about the "header soup" made me look into this problem:

It seems some of the "#include <asm/paravirt.h>" instances in the code
base can just be dropped.

For the remaining cases I'd like to suggest a reorg of the related headers:
Instead of having the non-paravirt definition in one header and the paravirt
ones in paravirt.h, maybe it would be better to have only the paravirt
generic definitions in paravirt.h and the specific functions in the header
defining the non-paravirt variant. This would probably resolve the problem
with the "soup", as paravirt.h wouldn't rely on so many other headers.

I'm just preparing a patch doing the removal of the not needed includes, but
I'd be willing to address the disentangling as noted above.

Thoughts?


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature