linux-kernel - Re: [PATCH v3 05/11] x86/mm: Track the TLB's tlb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 21 Jun 2017 08:11:15 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Andy Lutomirski <luto@...nel.org>, X86 ML <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Borislav Petkov <bp@...en8.de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mel Gorman <mgorman@...e.de>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        Nadav Amit <nadav.amit@...il.com>,
        Rik van Riel <riel@...hat.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Arjan van de Ven <arjan@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v3 05/11] x86/mm: Track the TLB's tlb_gen and update the
 flushing algorithm

On Wed, Jun 21, 2017 at 1:32 AM, Thomas Gleixner <tglx@...utronix.de> wrote:
> On Tue, 20 Jun 2017, Andy Lutomirski wrote:
>>  struct flush_tlb_info {
>> +     /*
>> +      * We support several kinds of flushes.
>> +      *
>> +      * - Fully flush a single mm.  flush_mm will be set, flush_end will be
>
> flush_mm is the *mm member in the struct, right? You might rename that as a
> preparatory step so comments and implementation match.

The comment is outdated.  Fixed now.

>
>> +      *   TLB_FLUSH_ALL, and new_tlb_gen will be the tlb_gen to which the
>> +      *   IPI sender is trying to catch us up.
>> +      *
>> +      * - Partially flush a single mm.  flush_mm will be set, flush_start
>> +      *   and flush_end will indicate the range, and new_tlb_gen will be
>> +      *   set such that the changes between generation new_tlb_gen-1 and
>> +      *   new_tlb_gen are entirely contained in the indicated range.
>> +      *
>> +      * - Fully flush all mms whose tlb_gens have been updated.  flush_mm
>> +      *   will be NULL, flush_end will be TLB_FLUSH_ALL, and new_tlb_gen
>> +      *   will be zero.
>> +      */
>>       struct mm_struct *mm;
>>       unsigned long start;
>>       unsigned long end;
>> +     u64 new_tlb_gen;
>
> Nit. While at it could you please make that struct tabular aligned as we
> usually do in x86?

Sure.

>
>>  static void flush_tlb_func_common(const struct flush_tlb_info *f,
>>                                 bool local, enum tlb_flush_reason reason)
>>  {
>> +     struct mm_struct *loaded_mm = this_cpu_read(cpu_tlbstate.loaded_mm);
>> +
>> +     /*
>> +      * Our memory ordering requirement is that any TLB fills that
>> +      * happen after we flush the TLB are ordered after we read
>> +      * active_mm's tlb_gen.  We don't need any explicit barrier
>> +      * because all x86 flush operations are serializing and the
>> +      * atomic64_read operation won't be reordered by the compiler.
>> +      */
>
> Can you please move the comment above the loaded_mm assignment?

I'll move it above the function entirely.  It's more of a general
comment about how the function works than any particular part of the
function.

>
>> +     u64 mm_tlb_gen = atomic64_read(&loaded_mm->context.tlb_gen);
>> +     u64 local_tlb_gen = this_cpu_read(cpu_tlbstate.ctxs[0].tlb_gen);
>> +
>>       /* This code cannot presently handle being reentered. */
>>       VM_WARN_ON(!irqs_disabled());
>>
>> +     VM_WARN_ON(this_cpu_read(cpu_tlbstate.ctxs[0].ctx_id) !=
>> +                loaded_mm->context.ctx_id);
>> +
>>       if (this_cpu_read(cpu_tlbstate.state) != TLBSTATE_OK) {
>> +             /*
>> +              * leave_mm() is adequate to handle any type of flush, and
>> +              * we would prefer not to receive further IPIs.
>
> While I know what you mean, it might be useful to have a more elaborate
> explanation why this prevents new IPIs.

Added, although it just gets deleted again later in the series.

>
>> +              */
>>               leave_mm(smp_processor_id());
>>               return;
>>       }
>>
>> -     if (f->end == TLB_FLUSH_ALL) {
>> -             local_flush_tlb();
>> -             if (local)
>> -                     count_vm_tlb_event(NR_TLB_LOCAL_FLUSH_ALL);
>> -             trace_tlb_flush(reason, TLB_FLUSH_ALL);
>> -     } else {
>> +     if (local_tlb_gen == mm_tlb_gen) {
>> +             /*
>> +              * There's nothing to do: we're already up to date.  This can
>> +              * happen if two concurrent flushes happen -- the first IPI to
>> +              * be handled can catch us all the way up, leaving no work for
>> +              * the second IPI to be handled.
>
> That not restricted to IPIs, right? A local flush / IPI combo can do that
> as well.

Indeed.  Comment fixed.

>
> Other than those nits;
>
> Reviewed-by: Thomas Gleixner <tglx@...utronix.de>