Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH v2 11/11] mm,sched: conditionally skip lazy TLB mm refcounting
From:   Andy Lutomirski <luto@amacapital.net>
In-Reply-To: <1532987170.28585.52.camel@surriel.com>
Date:   Mon, 30 Jul 2018 15:00:40 -0700
Cc:     Andy Lutomirski <luto@kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Peter Zijlstra <peterz@infradead.org>,
        LKML <linux-kernel@vger.kernel.org>,
        kernel-team <kernel-team@fb.com>, X86 ML <x86@kernel.org>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Ingo Molnar <mingo@kernel.org>, Mike Galbraith <efault@gmx.de>,
        Dave Hansen <dave.hansen@intel.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <91F4157A-8004-46EC-AE04-F2F338B0D5BB@amacapital.net>
References: <20180728215357.3249-1-riel@surriel.com> <20180728215357.3249-11-riel@surriel.com> <CALCETrWS4pE5Tv3OErM3AD-E=CGG3U+2CBVecRF3N0c+g42vEg@mail.gmail.com> <20180729155452.37eddc11@imladris.surriel.com> <20180730095502.GG2494@hirez.programming.kicks-ass.net> <1532961011.28585.30.camel@surriel.com> <20180730162653.GM2494@hirez.programming.kicks-ass.net> <1532978146.28585.32.camel@surriel.com> <CALCETrXZPN6Lt55siHM__YW4jFfoy5w+Hd2aJtEi8qhsro6RMA@mail.gmail.com> <1532979368.28585.33.camel@surriel.com> <CALCETrXnG9nPzJKOtd=65BWeoTs2y=mVUAEvNGmFrD8fvg-EfQ@mail.gmail.com> <1532987170.28585.52.camel@surriel.com>
To:     Rik van Riel <riel@surriel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Jul 30, 2018, at 2:46 PM, Rik van Riel <riel@surriel.com> wrote:
>=20
>> On Mon, 2018-07-30 at 12:49 -0700, Andy Lutomirski wrote:
>>=20
>>=20
>> I think it's a big step in the right direction, but it still makes be
>> nervous.  I'd be more comfortable with it if you at least had a
>> functional set of patches that result in active_mm being gone,
>> because
>> that will mean that you actually audited the whole mess and fixed
>> anything that might rely on active_mm pointing somewhere or that
>> might
>> be putting a value you didn't take into account into active_mm.  IOW
>> I'm not totally thrilled by applying the patches as is if we're still
>> a bit unsure as to what might have gotten missed.
>>=20
>> I don't think it's at all necessary to redo the patches.
>>=20
>> Does that seem reasonable?
>=20
> Absolutely. I tried to keep ->active_mm very similar
> to before for exactly that reason.
>=20
> Lets go through all the places where it is used, in
> x86 and architecture independent code. I have not
> checked other architectures.
>=20
> It looks like we should be able to get rid of
> ->active_mm at some point, but a lot of it depends
> on other architecture maintainers.
>=20
>=20
> arch/x86/events/core.c:
> - get_segment_base: get current->active_mm->context.ldt,
>  this appears to be for TIF_IA32 user programs only, so
>  we should be able to use current->mm here

->mm sounds more correct anyway

>=20
> arch/x86/kernel/cpu/common.c:
> - current task's ->active_mm assigned in two places,
>  never read
>=20
> arch/x86/lib/insn-eval.c:
> - get_desc() gets current->active_mm->context.ldt, this
>  appears to be only for user space programs

Same as above

>=20
> arch/x86/mm/tlb.c:
> - this series adds two places where current->active_mm is
>  written, it is never read
>=20
> arch/x86/platform/efi/efi_64.c:
> - current->active_mm is set to efi_mm for a little bit,
>  with irqs disabled, and then changed back, with irqs still
>  disabled; we should be able to get rid of ->active_mm here
> - in the init code, ->active_mm is set to efi_mm as well,
>  presumably the kernel automatically switches that back on
>  the next context switch; this may be buggy, since preemption
>  is enabled and a GFP_KERNEL allocation is just a few lines
>  below

Ick. This should mostly go away soon =E2=80=94 most EFI code will move to a r=
eal thread.

>=20
> arch/x86/power/cpu.c:
> - fix_processor_context() calls load_mm_ldt(current->active_mm);,
>  we should be able to use cpu_tlbstate.loaded_mm instead

Agreed

>=20
> drivers/cpufreq/pmac32-cpufreq.c:
> - pmu_set_cpu_speed() restores current->active_mm - don't know if
>  anyone still cares about 32 bit PPC :)
>=20

I think we should only remove active_mm when the new #define is set. So this=
 doesn=E2=80=99t need to change.

> drivers/firmware/efi/arm-runtime.c:
> - efi_virtmap_unload switches back the pgd to current->active_mm
>  from &efi_mm; that mm could be stored elsewhere if we excised
>  ->active_mm everywhere

Ditto

IOW the active_mm refcounting may be genuinely useful for architectures that=
 are not able to efficiently shoot down remote lazy mm references in exit_mm=
ap(). I suspect that ARM64 may be in that category.