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

[David Hildenbrand <david@xxxxxxxxxx>]

> Somewhat, but in the regular case where enter() is called followed by
> leave() there is really no complexity for the caller, just an extra
> local variable.
>
> There are complications where we want to exit lazy_mmu temporarily, as
> in mm/kasan/shadow.c [1k], but this is in fact unavoidable. Chatting
> with Mark Rutland, I realised that to truly support nested sections,
> this must be handled in a special way in any case. To be clear, I am
> referring to this situation:
>
> __kasan_populate_vmalloc:
>      apply_to_page_range:
>          arch_enter_lazy_mmu_mode() {1}
>
>          kasan_populate_vmalloc_pte:
>              arch_leave_lazy_mmu_mode() {2}
>              arch_enter_lazy_mmu_mode() {3}
>
>          arch_leave_lazy_mmu_mode() {4}
>
> With the approach this series takes, call {2} is made safe by passing a
> special parameter (say LAZY_MMU_FLUSH) that forces lazy_mmu to be fully
> exited - and call {3} will then re-enter lazy_mmu. This works regardless
> of whether __kasan_populate_vmalloc() has been called with lazy_mmu
> already enabled (i.e. calls {1} and {4} can be nested).
>
> On the other hand, with a pagefault_disabled-like approach, there is no
> way to instruct call {3} to fully exit lazy_mmu regardless of the
> nesting level.
Sure there is, with a better API. See below. :)

> It would be possible to make both approaches work by introducing a new
> API, along the lines of:
> - int arch_disable_save_lazy_mmu_mode() (the return value indicates the
> nesting level)
> - void arch_restore_lazy_mmu_mode(int state) (re-enter lazy_mmu at the
> given nesting level)
Yes, I think we really need a proper API.

> This is arguably more self-documenting than passing LAZY_MMU_FLUSH in
> call {2}. This API is however no simpler when using a
> pagefault_disabled-like approach (and less consistent than when always
> saving state on the stack).
Yes, a proper API is warranted. In particular, thinking about the following:

arch_enter_lazy_mmu_mode() {1}
        arch_enter_lazy_mmu_mode() {2}

        kasan_populate_vmalloc_pte:
                arch_leave_lazy_mmu_mode() {3}
                arch_enter_lazy_mmu_mode() {4}

        arch_leave_lazy_mmu_mode() {5}
arch_leave_lazy_mmu_mode() {6}


Imagine if we have the following API instead:

lazy_mmu_enable() {1}
        lazy_mmu_enable() {2}

        kasan_populate_vmalloc_pte:
                lazy_mmu_pause() {3}
                lazy_mmu_continue() {4}

        lazy_mmu_disable() {5}
lazy_mmu_disable() {6}


I think it is crucial that after lazy_mmu_save/lazy_mmu_restore, no more 
nesting must happen.
Assume we store in the task_struct

uint8_t lazy_mmu_enabled_count;
bool lazy_mmu_paused;

We can do things like

a) Sanity check that while we are paused that we get no more 
enable/disable requests
b) Sanity check that while we are paused that we get no more pause requests.

[...]

> > If LAZY_MMU_DEFAULT etc. are not for common code, then please
> > maintain them for the individual archs as well, just like you do with the
> > opaque type.
> I see your point - having them defined in <linux/mm_types.h> could be
> misleading. I just wanted to avoid all 4 architectures defining the same
> macros. Maybe call them __LAZY_MMU_* to suggest they're not supposed to
> be used in generic code?
Maybe look into avoiding them completely :) Let's agree on the API first 
and then figure out how to pass the information we need to pass.
[...]

> > Worse, it does not
> > > truly enable states to be nested: it allows the outermost section to
> > > store some state, but nested sections cannot allocate extra space. This
> > > is really what the stack is for.
> > If it's really just 8 bytes I don't really see the problem. So likely
> > there is
> > more to it?
> I suppose 8 extra bytes per task is acceptable, but some architectures
> may want to add more state there.
Just for reference: we currently perform an order-2 allocation, 
effectively leaving ~4KiB "unused".
If there are any real such case on the horizon where we need to store 
significantly more (in which case storing it on the stack might probably 
also bad), please let me know.
> The one case that is truly problematic (though not required at this
> point) is where each (nested) section needs to store its own state. With
> this series it works just fine as there is a lazy_mmu_state_t for each
> section, however if we use task_struct/thread_struct there can be only
> one member shared by all nested sections.
Do we have a use case for that on the horizon? If so, I fully agree, we 
have to store information per level. How/what information we have to 
store would be another question.
--
Cheers

David / dhildenb