Re: [PATCH v4 2/4] lazy tlb: allow lazy tlb mm refcounting to be configurable

Excerpts from Andy Lutomirski's message of June 9, 2021 2:20 am:

On 6/4/21 6:42 PM, Nicholas Piggin wrote:

quoted

Add CONFIG_MMU_TLB_REFCOUNT which enables refcounting of the lazy tlb mm
when it is context switched. This can be disabled by architectures that
don't require this refcounting if they clean up lazy tlb mms when the
last refcount is dropped. Currently this is always enabled, which is
what existing code does, so the patch is effectively a no-op.

Rename rq->prev_mm to rq->prev_lazy_mm, because that's what it is.

I am in favor of this approach, but I would be a lot more comfortable
with the resulting code if task->active_mm were at least better
documented and possibly even guarded by ifdefs.

active_mm is fairly well documented in Documentation/active_mm.rst IMO.
I don't think anything has changed in 20 years, I don't know what more
is needed, but if you can add to documentation that would be nice. Maybe
moving a bit of that into .c and .h files?

x86 bare metal currently does not need the core lazy mm refcounting, and
x86 bare metal *also* does not need ->active_mm.  Under the x86 scheme,
if lazy mm refcounting were configured out, ->active_mm could become a
dangling pointer, and this makes me extremely uncomfortable.

So I tend to think that, depending on config, the core code should
either keep ->active_mm [1] alive or get rid of it entirely.

I don't actually know what you mean.

core code needs the concept of an "active_mm". This is the mm that your 
kernel threads are using, even in the unmerged CONFIG_LAZY_TLB=n patch,
active_mm still points to init_mm for kernel threads.

We could hide that idea behind an active_mm() function that would always 
return &init_mm if mm==NULL, but you still have the concept of an active
mm and a pointer that callers must not access after free (because some
cases will be CONFIG_LAZY_TLB=y).

[1] I don't really think it belongs in task_struct at all.  It's not a
property of the task.  It's the *per-cpu* mm that the core code is
keeping alive for lazy purposes.  How about consolidating it with the
copy in rq?

I agree it's conceptually a per-cpu property. I don't know why it was 
done this way, maybe it was just convenient and works well for mm and 
active_mm to be adjacent. Linus might have a better insight.

I guess the short summary of my opinion is that I like making this
configurable, but I do not like the state of the code.

I don't think I'd object to moving active_mm to rq and converting all
usages to active_mm() while we're there, it would make things a bit
more configurable. But I don't see it making core code fundamentally
less complex... if you're referring to the x86 mm switching monstrosity,
then that's understandable, but I admit I haven't spent enough time
looking at it to make a useful comment. A patch would be enlightening,
I have the leftover CONFIG_LAZY_TLB=n patch if you were thinking of 
building on that I can send it to you.

Thanks,
Nick