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

[David Hildenbrand <david@xxxxxxxxxx>]

> > > 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. :)
> I meant while keeping the existing shape of the API but yes fair enough!
Time to do it properly I guess :)

[...]

> > Assume we store in the task_struct
> >
> > uint8_t lazy_mmu_enabled_count;
> > bool lazy_mmu_paused;
> I didn't think of that approach! I can't immediately see any problem
> with it, assuming we're fine with storing arch-specific context in
> thread_struct (which seems to be the case as things stand).
Right, just to complete the picture:

a) We will have some CONFIG_ARCH_LAZY_MMU

b) Without that config, all lazy_mmu_*() functions are a nop and no 
lazy_mmu_state is stored in task_struct
struct lazy_mmu_state {
        uint8_t enabled_count;
        bool paused;
}

c) With that config, common-code lazy_mmu_*() functions implement the 
updating of the lazy_mmu_state in task_struct and call into arch code
on the transition from 0->1, 1->0 etc.

Maybe that can be done through exiting 
arch_enter_lazy_mmu_mode()/arch_leave_lazy_mmu_mode() callbacks, maybe 
we need more. I feel like
we might be able to implement that through the existing helpers.

> > 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.
> These are good points - and this is only possible with such global
> state. (Similarly we can check that the counter never underflows.)
Exactly.

[..]

> Overall what you're proposing seems sensible to me, the additional
> fields in task_struct don't take much space and we can keep the API
> unchanged in most cases. It is also good to have the option to check
> that the API is used correctly. I'll reply to the cover letter to let
> anyone who didn't follow this thread chip in, before I go ahead and try
> out that new approach.
And on top of the proposal above we will have some

struct arch_lazy_mmu_state;

define by the architecture (could be an empty struct on most).

We can store that inside "struct lazy_mmu_state;" or if we ever have to, 
start returning only that from the enable/disable etc. functions.
For now, I'd say just store it in the task struct in the lazy_mmu_state. 
But we can always adjust later if required.
In the first (this) series we probably don't even have to introduce 
arch_lazy_mmu_state.
--
Cheers

David / dhildenb